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.

First patch:

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.

Second patch:

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.

Third patch:

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.

Fourth patch

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 perfect good.

Fifth patch

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.

Sixth patch

This one was adding a mochitest.

Seventh patch

Ha, this is just fixing up my patch header information. Adding an email and updating the commit message.

Eighth patch

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.

Ninth patch

I updated my test, because I was getting random orange failures from try server from my own test. Now passes or fails reliably.

About these ads

Leave a Reply

Fill in your details below or click an icon to log in: Logo

You are commenting using your account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s


Get every new post delivered to your Inbox.

%d bloggers like this: