OSD700 project release 0.6

Continuing on from my OSD 700 release 0.5 I now I have the next logical release, as 0.6.

My project is to implement hash change media fragment refresh on media documents. Example: Open a media document in a new tab, change the hash tag in the URI to a new number, and hit enter. This is called a media fragment, and changes should update the video’s position. If you do not have my changes though, this will not do anything.

I have gone through a few iterations for this patch, and finally have something that is passing review. It has some nits that I am fixing and was hoping to have fixed for this release, but I hit some issues in the build process and could not wait any longer. Whenever I try to build, it fails saying files were modified externally during the build.

What I am doing in this final commit is similar to what I was trying in my last post. I am inserting a script into the media document’s head, that listens for a hash change and updates the video. If you look in my last post you will see the script I insert, but now it is a simple inline script:

window.addEventListener('hashchange',function(e){document.querySelector('audio,video').src=e.newURL;},false);

That’s it, I just set the media element’s source to the new URI in a hash change event. The advantages to this is we can use the parse media fragment c++ code and our changes are pretty modular enough that we are highly unlikely to break anything.

I had a lot of fun on this release. Felt more productive. During the last bit leading up to this release I got to go back and forth with Boris Zbarsky a couple times, getting some review minuses, and felt I was getting a better handle on the code, knew what he was talking about, and how to implement it. Slowly getting over the learning curve. Every project or codebase I have been in has this moment, and is pretty exciting!

I still need to figure out testing though, and figure out why my builds are failing. I am doing a clobber build now, so we’ll see soon.

First review of my first Firefox patch

So I had a my first Firefox patch 677122 reviewed. It was a review-needs-work, but that’s fine.

I had to get some help to find someone to review it. I asked in #introduction, and was told Boris Zbarsky was a good candidate, but also to check the log of the file I changed and look for previous reviewers. I saw Boris Zbarsky as a pretty active reviewer, only to cement this advice. I ran the command “hg log path/to/file > output.txt” to check the file for previous reviewers.

My patch failed because there is simply a better approach. I was reloading the page if we had a new hash in a synthetic document, but that would require the video to constantly be refreshed. He suggested I add a script to media documents to listen for haschange events, and do the work there. This is awesome to me, as JavaScript is something I am more familiar with, and I get to do some regular expression work, which I always enjoy.

I have two of the three requirements with this new method already done. Did not take much time at all to write some JavaScript to do this.

I have this:

window.addEventListener( "hashchange", function( event ) {

  var match = /#t=(\d*)(,)?(\d*)/.exec( event.target.location.hash );

  var start,
      end;

  var media = document.getElementsByTagName( "video" )[ 0 ] || document.getElementsByTagName( "audio" )[ 0 ];

  if ( !media || !match || ( match[ 2 ] && !match[ 3 ] ) ) {

    return;
  }

  start = match[ 1 ] || 0;
  end = match[ 3 ];

  media.currentTime = start;

  if ( end ) {

    var listener = function( event ) {

      console.log( this.currentTime, end );
      if ( this.currentTime >= end ) {

        this.pause();
        media.removeEventListener( "timeupdate", listener, false );
      }
    };

    media.addEventListener( "timeupdate", listener, false );
  }

  media.play();
}, false );

This is full of early return and regex awesomeness. It first sets up a listener for hash change events (I just noticed I may need to wait for the page to finish loading), next I check the hash and get a reference to the media element. I check that both of these are valid, if not, return. Then I apply the start time, and setup a listener for the end time. This may need tweaking as time goes on, but it is something to work with.

I also have the code needed to insert my script into the media document creation:

  nodeInfo = mNodeInfoManager->GetNodeInfo(nsGkAtoms::script, nsnull,
                                           kNameSpaceID_XHTML,
                                           nsIDOMNode::ELEMENT_NODE);
  NS_ENSURE_TRUE(nodeInfo, NS_ERROR_OUT_OF_MEMORY);

  nsRefPtr<nsGenericHTMLElement> script = NS_NewHTMLScriptElement(nodeInfo.forget());
  NS_ENSURE_TRUE(script, NS_ERROR_OUT_OF_MEMORY);

  head->AppendChildTo(script, false);

This gets the node info for a script node, creates a script with the node info, and appends it to the head element. This works, and am currently writing this post in my build.

The last part is to figure out how to get all my JavaScript into that script element.

Popcorn.js down the dark path of IE8 support

What I mean by support is that it will be possible, but it will be a “Your mileage may vary” situation. We’ll make it possible, and show how to do it, but we won’t officially support it or test against it. It will be a separate entity that the Popcorn code does not care about.

This is due to increasing demand for Popcorn.js to reach more audiences, and while I do believe this is a dark road, and one that detrimental to the fight to kill IE8, I also find it a unique challenge…

Personally, I believe the numbers in IE8′s market share to be inflated, and not really accurate. It is the enterprise environment that refuses to upgrade to the next version, and not the home user. I would say for each home computer, there is one or more users. And, for each enterprise install, there are three more sitting in the office next to it that are only used once a week. I know this, because we have a bunch of dead offices in our building, probably with Windows XP and IE8. So now this person can view Popcorn on Fox’s website one day a week, instead of doing work…

Anyway, we found a way to have our cake and eat it to.

First, we’re pulling players out of core, and into a module. We are doing this as players are mainly for youtube support, which is flash, which works on IE8. So by pulling it out, we can make the player module 100% support IE8. Then, we are going to create a shim, that when included before a clean popcorn.js file, will make it dirty. So now you include your shim, popcorn, and attach the player module, and you’re good to go. I also considered simply including the shim inside the player module, as without the player module, the shim is pretty pointless, but I am not sold on this yet; need more discussion.

Another note. This will not mean each and every one of our plugins will support IE8. Some might, some might not. But, plugins can be modified and created with IE8 in mind, and that is the idea of a plugin anyway.

OSD 700 project release 0.5

So, my release for this week is a much better fix than before to bug 677122, and it is now in review.

This release started with canceling anchor code via a search in a string for what I thought was a valid case, but it turns out a string like this “#t=50,100″ is, unfortunately, a valid anchor name.

I asked for help, and was pointed to a boolean variable MozSyntheticDocument. Ended up not being exactly what I needed, as it was a protected member variable, but it started me down the path to finding the public getter function IsSyntheticDocument.

I wired it all together by getting an instance of the document as a nsIDocument, and was able to call the IsSyntheticDocument from that.

I updated a comment regarding the code I was working on, to illustrate the changes. I fixed a typo while I was at it!

My whole fix is centered around one if statement, that checked if a variable doShortCircuitedLoad is true. Basically, doShortCircuitedLoad happens if:

a) we’re navigating between two different SHEntries which have the same document identifiers, or

b) we’re navigating to a new shentry whose URI differs from the current URI only in its hash, the new hash is non-empty, and we’re not doing a POST.

Previously, I would just add my check next to the doShortCircuitedLoad check, so looked like this “if (doShortCircuitedLoad && myCheck) {” now, I added some logic in the doShortCircuitedLoad declaration, so it’s all in one place. Much cleaner.

Finally, there was a variable doHashchange, that was defined outside this if statement, it was only ever referenced inside the if, and was only ever true if doShortCircuitedLoad was also true. I moved its declaration into the if, and dropped the dependency of doShortCircuitedLoad, as we’re only ever going to touch the variable if doShortCircuitedLoad is already true.

Then put it into review.

I suspect my next task is to find someone specific to review it.

My Firefox patch is now in review

Been working on Firefox ticket 677122, and I’m pretty happy with my current solution.

In my last post, I mention how I have a solution, and should have a patch up and another blog post by the next day.

I did have a patch up by the next day, but it took an extra day before I got around to the post (this post). Not a huge deal.

Anyway, I did a few other changes in the code I was touching.

I noticed a typo in a comment I was modifying, so I fixed it up.

Also, there was a boolean doHashChange being declared just outside an if block, but it was only referenced inside the if block. This was the if block I was stifling the execution of, so I also felt it was appropriate to move that boolean to live in the scope of the if. Part of what I am trying to fix in this bug is not to execute the code for anchor changes if we don’t need to. That boolean was only used for anchor changes. It was also only being set if the if statement was true, so by moving it into the if, we no longer need to check if that is valid, because if it’s false, we won’t even enter that scope.

I am pretty happy with myself at the moment. I feel this is the working, right, and fast way to fix this, but we’ll see.

Tests, and running tests are a whole new problem that I will admit still know very little about. I have to start thinking about how to test something that lives in a mediaDocument, something that has nowhere for JavaScript to live.

Update on my update to my Firefox bug.

An update to my last post, and my bug.

Turns out, it was an update issue. My code was simply out of date, and what I needed was so new it was still hot. A good and quick lesson.

Initially after the update, I could no longer build, but some more advice led me to doing a clobber build and it worked fine. Another good quick lesson.

Also, my fix is now working and much better than it was before.

Before, I would check if the anchor string looked like a media fragment, but a media fragment string could also be a valid anchor.

My new fix simply changes the same check to see if the document is a synthetic one. Looks like this “!document->IsSyntheticDocument()”.

A synthetic document is something like viewing an image, audio, or video file in a new tab. This is also good, because there is no point in entering the anchor logic if an anchor is not valid in the loading document.

I should have a patch up tomorrow, with a new blog.

Quick update on my Firefox bug

So, my “working” fix for Firefox bug 677122 will not actually work.

It will potentially break any anchor names that look like media fragment code.

Example “#t=50,100″ is, unfortunately, a valid anchor name. I am going to need to get more specific with my check, which is fine. I figure I would instead of doing a check on the string, do a check to see if the document is a MediaDocument, as MediaDocuments cannot (that I know of) have anchors.

I took hump’s advice, and asked for help in the #introduction channel on IRC.

I got a reply in less than two minutes from a “jdm”. I never did figure out his real name, but I thanked him as jdm.

Jdm’s advice was to check for MozSyntheticDocument. This makes sense for me. MozSyntheticDocument is a readonly boolean value attached to the nsIDOMDocument. I was able to get a reference to this document, breakpointed it, and it is confirmed to be what I want, except when I try to compile, I get an error that MozSyntheticDocument does not exist…

I searched MXR for SyntheticDocument, and found some other interesting bits. A mIsSyntheticDocument which is protected, but exposed via a IsSyntheticDocument function which is attached to the msIDocument.

I grabbed a reference to msIDocument, same deal, not there. Hmm.

Also tried a GetMozSyntheticDocument function, attached to nsDocument. Frustratingly enough, also not there…

Then it hit me what could be wrong… I have been using MXR as a reference, then go in and write my code, without double checking that MY code was up to date, and had the functionality I was seeing on MRX. Currently doing an update. Will post again soon with updates.