Reflecting on Firefox bug 677122
My patch for 677122 is in what I suspect is the final stage, about to land.
This is a bit of a summary of this bug’s history. Some personal reflecting for myself, to step back and look at it in a new way.
For starters I was shocked to see that I had 9 patches, 8 of which are obsolete. By looking at the patches you can see the evolution of the bug. I actually feel really good about it, looking at it like this.
I attempted a short circuit load only if the media fragment hash tag does not exist, otherwise we reload the whole page. From my experience, a short circuit load is an attempt to be smart and only load some of the page, that is needed.
This would not of worked because it may break anchor tags, and would be a potential disaster because it potentially touches other cases unrelated to media fragments, like anchors. Also, it reloaded the whole page, which is not ideal.
Understand more of what I was doing, and was able to describe it with “Don’t do a short-circuited load on synthetic documents”.
This is an attempt to make the previous patch better. I would solve the anchor issue, and only touch code that is doing a short circuited load, and did a general cleanup, but it still reloaded the whole page.
Scrapped my old attempt, and went with inserting a script tag in the mediaDocument head, that would listen to hash changes.
The problem with this was I went about it wrong. When I listened for hashchange, I would check the start and end values in the hash, and update the video accordingly, which worked, but was not taking advantage of the original media fragment parser. I did it myself in JavaScirpt.
I fixed the above by simply updating the src attribute to the new hash, on a hash change. This would take advantage of the current mediaFragment parser, and any additions or changes to it, and was super modular, or so I thought.
Cept for some review nits, it was almost
This one was just cleaning up the nits. I got burned on NS_ENSURE_TRUE returning true, when attempting to change the return type of the function to void, which was interesting. made sense once it was brought to my attention.
This was also the first to get a review plus. I wanted to get a test in though, to get the full knowledge. I am here to learn, after all.
This one was adding a mochitest.
Ha, this is just fixing up my patch header information. Adding an email and updating the commit message.
This is probably the one where I learned the most about the process. I started a fire, got try server access, and put out the fire.
The fire was caused because I was doing this on mediaDocument.cpp, which is misleading, and not just audio and video, but also images and add-ons. videoDocument.cpp is actually video and audio, which is what I wanted, with video just being an extension to audio.
I moved my script injection off mediaDocument and onto videoDocument. Solved the fire.
I updated my test, because I was getting random orange failures from try server from my own test. Now passes or fails reliably.