The work on the JS implementation of the webvtt parser is wrapping up (see https://github.com/andreasgal/vtt.js), and it would be good to move toward integrating it. Rick's agreed to work on this integration, and will need some guidance as he starts. I've cc'ed some folks who can hopefully answer questions, and I'm encouraging him to use #content on irc as well. Previously Rick had written a WebVTTLoadListener to handling downloading a .vtt file, managing parser instances, and handing the parsed data off to other parts of the media code. Likely much of this can be reused, with the parser getting created from a JS XPCOM service. Rick, some general resources to get you going: * https://developer.mozilla.org/en-US/docs/How_to_Build_an_XPCOM_Component_in_Javascript and especially https://developer.mozilla.org/en-US/docs/How_to_Build_an_XPCOM_Component_in_Javascript#Using_XPCOMUtils * You can see many examples of code that does this in-tree, http://mxr.mozilla.org/mozilla-central/search?string=XPCOMUtils.jsm, or look at examples like https://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIDroppedLinkHandler.idl (IDL), https://mxr.mozilla.org/mozilla-central/source/content/base/src/contentAreaDropListener.js (impl), https://mxr.mozilla.org/mozilla-central/source/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp#914 (caller). I'm not the right person to advise on the IDL design, but will help where I can with questions. You'll have to be able to support the callback-heavy API we have, pass a window object in, pass back Document Fragments, figure out the data structure you want to support for returned cues, regions, etc: var parser = new WebVTTParser(); parser.onregion = function (region) {} parser.oncue = function (cue) {} parser.onpartialcue = function (cue) {} parser.onflush = function () {} parser.parse(moreData); parser.parse(moreData); parser.flush(); parser.convertCueToDOMTree(window, cue); If those I've cc'ed know of good examples he should look at, please add links. I'd also ask that you give Rick (reyre on irc) advice if you see him wandering lost in the dark staircases of XPCOM.

I can mentor this.

something like this for cues: [scriptable, uuid(24590500-09fd-4d60-88be-86b6f750b793)] interface nsIWebVTTCue : nsISupports { readonly attribute nsIWebVTTRegion region; readonly attribute AString vertical; readonly attribute float line; readonly attribute float position; readonly attribute float size; readonly attribute AString align; }; [scriptable, uuid(fa41e4ab-48fd-4558-848f-3ad6181c3871)] interface nsIWebVTTRegion : nsISupports { readonly attribute nsAString id; readonly attribute float width; readonly attribute long lines; readonly attribute float regionAnchorX; readonly attribute float regionAnchorY; readonly attribute float viewportAnchorX; readonly attribute float viewportAnchorY; readonly attribute AString scroll; }; [scriptable, uuid(2953cf08-403e-4419-8d20-ce286aac026b)] interface nsIWebVTTListener : nsISupports { void oncue(in nsIWebVTTCue); }; [scriptable, uuid(1604a67f-3b72-4027-bcba-6dddd5be6b10)] interface nsIWebVTTParser : nsISupports { void parse(jsval moreData); void flush(); } [scriptable, uuid(1604a67f-3b72-4027-bcba-6dddd5be6b10)] interface nsIWebVTTParserService : nsISupports { nsIWebVTTParser newParser(); void convertCueToDOMTree(nsIDOMWindow window, nsIWebVTTCue cue); } Something like this, and then we will use JS to implement all of this. onregion should be caught internally here, and map that to nsIWebVTTRegion objects in the cue.

Thanks Andreas.

So just to make sure, the nsIWebVTTListener here would be implemented in C++, right? Is parse() called from c++ or from JS? I'm not sure why it's taking a jsval...

Thanks for mentoring me on this Andreas. I'll be wrapping up the tests in the next day or so and hope to get started on this by Friday or Monday of next week (if not the weekend).

The listener would be implemented in C++, yes. parse is called from C++. I thought jsval is easier. We want an array buffer on the vtt.js side. Happy to hear alternatives. This is called on necko streams.

> We want an array buffer on the vtt.js side. ... This is called on necko streams. So our basic options are as follows, I think: 1) Define it as jsval, have the C++ necko consumer read the necko stream, then create an arraybuffer and pass it in. This is somewhat rocket science for the necko consumer, especially if it wants to create the arraybuffer in the compartment of the JS component (and if it _doesn't_ do that, then your array buffer will entail cross-compartment wrappers and touching it from JS will suck. A lot). 2) Define it as a stream, have the C++ necko consumer just pass through the stream. Add a way to read data into a new arraybuffer to nsIScriptableInputStream and use that from JS; at that point the C++ callee will get a JSContext in the right compartment and just be able to allocate the arraybuffer there. The benefit of this approach is that you can just allocate the arraybuffer in the C++, then ReadSegments directly into it. The drawback is having to create an nsIScriptableInputStream per call. Though at first glance nothing prevents us from reinitializing a given instance with a new nsIInputStream, actually. Or alternately, we could do something where the "read data" function takes an array buffer passed from JS (probably of size available()) and writes the bytes into it... 3) Define it as a stream as in #2 but just readBytes on the nsIScriptableInputStream and then convert the resulting JS string to an array buffer via a for loop or whatnot. This involves more buffer copies: one when reading from the stream to the nsCString, one when converting the nsCString to a JSString, and one when converting the JSString to an ArrayBuffer. The benefit is not having to write C++ code that uses the JSAPI _and_ our stream interfaces at once. ;) I'm personally partial to option #2 in some flavor.

nsIWebVTTListener should probably not have oncue (onfoos are in general for event handlers only), and could be probably [function]

This is great, thanks for the quick input. A couple of things I noticed reading that IDL. * I think for nsIWebVTTParserService void convertCueToDOMTree(nsIDOMWindow window, nsIWebVTTCue cue); wants to be: nsIDOMDocumentFragment convertCueToDOMTree(nsIDOMWindow window, nsIWebVTTCue cue); * It would be good to expose the other bits of a cue that TextTrackCue expects, specifically id, startTime, endTime, pauseOnExit, snapToLines, and text. It might also be good to harmonize data types with what we have in the webidl for the DOM version of a cue (see TextTrackCue.webidl), which would make it easier to populate the DOM type directly with this object.

Thanks for the input everyone. Helps me a lot :). (In reply to David Humphrey (:humph) from comment #9) > * It would be good to expose the other bits of a cue that TextTrackCue > expects, specifically id, startTime, endTime, pauseOnExit, snapToLines, and > text. It might also be good to harmonize data types with what we have in > the webidl for the DOM version of a cue (see TextTrackCue.webidl), which > would make it easier to populate the DOM type directly with this object. I think we need to do this as the TextTrackCues need to live on the TextTrack's TextTrackCueList.

Any suggestions as to where these IDLs, JS, and C++ files should live? WebVTTLoadListener.cpp is in content/media/ so I'm thinking we could put the IDL and JS files in content/media/webvtt/ along with the parser, cue, and region stuff. This is the directory where the old WebVTT stuff lived. Talking with Boris on IRC he suggested that nsIWebVTTRegion and nsIWebVTTCue be intermediate objects (for now) and we can convert them over to there WebIDL counterparts when we receive them.

(In reply to Andreas Gal :gal from comment #2) > Something like this, and then we will use JS to implement all of this. > onregion should be caught internally here, and map that to nsIWebVTTRegion > objects in the cue. I'm not sure if we want to map nsIWebVTTRegion internally as the WebVTTRegion needs to live on the TextTrack as well. See http://dev.w3.org/html5/webvtt/#extension-of-the-texttrack-interface-for-region-support. So WebVTTRegions need to live on there own separate from the WebVTTCue. It might be best to keep it to the spec for now where WebVTTCues have a region ID only. Something like: [scriptable, uuid(2953cf08-403e-4419-8d20-ce286aac026b)] interface nsIWebVTTListener : nsISupports { void oncue(in nsIWebVTTCue); void onregion(in nsIWebVTTRegion); }; Might be better.

(In reply to Andreas Gal :gal from comment #2) > [scriptable, uuid(1604a67f-3b72-4027-bcba-6dddd5be6b10)] > interface nsIWebVTTParser : nsISupports > { > void parse(jsval moreData); > void flush(); > } Was the thinking here that nsWebVTTParser would be vtt.js itself? Or it would just hook into vtt.js? I'm thinking it would be easier to just hook into it.

First WIP Couple of things I'm not sure on that I'd like feedback about: - Where should the IDL and JS files be placed? - I'm not creating the nsWebVTTParserService correctly. Calling CallGetService() does not work. Any other help or suggestions would be great. Thanks!

Sorry, fixed a few typos. Please see comment 14 for info about this WIP.

Comment on attachment 780967 [details] [diff] [review] WIP v1 Gregor volunteered to shepard this and help driving this in.

Getting closer.

This patch has a couple changes that have yet to be landed in vtt.js. Those will be landed before this lands so we can reflect those changes properly. See https://github.com/andreasgal/vtt.js/issues/81. This also doesn't print out bold, underline, and italic tags like the last version did. That's due to how vtt.js is handling creating a DOM tree right now. We can land that separately later. See https://github.com/andreasgal/vtt.js/issues/82 for that.

Comment on attachment 781854 [details] [diff] [review] v1: Integrate vtt.js into Gecko. Review of attachment 781854 [details] [diff] [review]: ----------------------------------------------------------------- Great to see this. Good work! Can you please split this into two or three separate patches? At least the HTMLTrackElement/TextTrack* changes should be a separate commit from the WebVTTParserService and related code. You could also land vtt.js itself separately from the service wrapper. If github is still the upstream for vtt.jsm, please reference the commit id you're importing, either in the file or the commit message. ::: content/html/content/src/HTMLTrackElement.h @@ +154,5 @@ > > nsRefPtr<TextTrack> mTrack; > nsCOMPtr<nsIChannel> mChannel; > nsRefPtr<HTMLMediaElement> mMediaParent; > + nsRefPtr<WebVTTListener> mWebVTTListener; How about just mListener?

(In reply to Ralph Giles (:rillian) from comment #20) > Comment on attachment 781854 [details] [diff] [review] > v1: Integrate vtt.js into Gecko. > > Review of attachment 781854 [details] [diff] [review]: > ----------------------------------------------------------------- > Can you please split this into two or three separate patches? At least the > HTMLTrackElement/TextTrack* changes should be a separate commit from the > WebVTTParserService and related code. You could also land vtt.js itself > separately from the service wrapper. Done. > If github is still the upstream for vtt.jsm, please reference the commit id > you're importing, either in the file or the commit message. Done. > ::: content/html/content/src/HTMLTrackElement.h > @@ +154,5 @@ > > > > nsRefPtr<TextTrack> mTrack; > > nsCOMPtr<nsIChannel> mChannel; > > nsRefPtr<HTMLMediaElement> mMediaParent; > > + nsRefPtr<WebVTTListener> mWebVTTListener; > > How about just mListener? Done! Hope this is good.

Comment on attachment 787743 [details] [diff] [review] Part 2 v2: Add WebVTT parser service and WebVTT IDls. Review of attachment 787743 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the build bits with a few nits addressed. ::: content/media/WebVTTListener.cpp @@ +1,4 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ If this is actually a copy of WebVTTLoadListener.cpp, it'd be better to have the diff show it as a rename. (Looks like maybe you generated this diff from git, so I don't know if you can do that from there.) ::: content/media/moz.build @@ +10,4 @@ > > PARALLEL_DIRS += ['webaudio'] > > +PARALLEL_DIRS += ['webvtt'] You can just merge this with the line above: PARALLEL_DIRS += [ 'webaudio', 'webvtt', ] (look elsewhere in the tree for actual formatting) ::: content/media/webvtt/moz.build @@ +11,5 @@ > + 'nsIWebVTTParserWrapper.idl', > + 'nsIWebVTTRegion.idl', > +] > + > +XPIDL_MODULE = 'webvtt'; Semicolons? In my moz.build? Also you can get rid of XPIDL_MODULE, it defaults to MODULE anyway.

Comment on attachment 787744 [details] [diff] [review] Part 3 v2: Add JS WebVTT parser code in (vtt.js) Review of attachment 787744 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webvtt/vtt.jsm @@ +5,5 @@ > +this.EXPORTED_SYMBOLS = ["WebVTTParser"]; > + > +/** > + * Code below is vtt.js the JS WebVTTParser. > + * Current source code can be found at http://github.com/andreasgal/vtt.js If you generated this with a script it would be good to have that script checked in to the tree (even if it's fairly trivial) so that anyone can update the code. c.f.: http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/update-breakpad.sh

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #25) Thanks for the review Ted. > > PARALLEL_DIRS += ['webaudio'] > > > > +PARALLEL_DIRS += ['webvtt'] > > You can just merge this with the line above: > PARALLEL_DIRS += > [ > 'webaudio', > 'webvtt', > ] > (look elsewhere in the tree for actual formatting) There are a few individual PARALLEL_DIRS statements in this mozbuild file above this one. Should I create a separate patch to merge them all together? > Also you can get rid of XPIDL_MODULE, it defaults to MODULE anyway. Just to clarify -- should I replace XPIDL_MODULE with MODULE or just remove this statement completely?

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #26) > If you generated this with a script it would be good to have that script > checked in to the tree (even if it's fairly trivial) so that anyone can > update the code. c.f.: > http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/update- > breakpad.sh I just copy and pasted the one vtt.js file to the vtt.jsm. Is this simple enough to not warrant a script or is it best to have one anyway?

(In reply to Rick Eyre (:reyre) from comment #27) > There are a few individual PARALLEL_DIRS statements in this mozbuild file > above this one. Should I create a separate patch to merge them all together? You can just merge them all in this patch, it's a pretty trivial cleanup. > Just to clarify -- should I replace XPIDL_MODULE with MODULE or just remove > this statement completely? You can remove the XPIDL_MODULE line and keep the MODULE line. (In reply to Rick Eyre (:reyre) from comment #28) > I just copy and pasted the one vtt.js file to the vtt.jsm. Is this simple > enough to not warrant a script or is it best to have one anyway? That is pretty trivial, but then you must have manually inserted the upstream revision info. It'd only be a few lines of script (in whatever language) to automate that, so it's not a bad idea, but it's not a hard requirement either. I'm sure after the 10th time you do it by hand you'll wish you'd written a script. :)

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #29) > That is pretty trivial, but then you must have manually inserted the > upstream revision info. It'd only be a few lines of script (in whatever > language) to automate that, so it's not a bad idea, but it's not a hard > requirement either. I'm sure after the 10th time you do it by hand you'll > wish you'd written a script. :) Heh, this is truth. I'll write a script for it then. Thanks.

(You don't need to get review on the script at all, it'd be NPOTB, but I'd also be happy to review if you'd like.)

Comment on attachment 787743 [details] [diff] [review] Part 2 v2: Add WebVTT parser service and WebVTT IDls. Review of attachment 787743 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for splitting the patch up. r=me with comments addressed. ::: content/html/content/src/HTMLTrackElement.h @@ +138,1 @@ > // LoadResource(). How about: // Open a new channel to the HTMLTrackElement's src attribute and call mListener's LoadResource(). ::: content/media/TextTrackCue.cpp @@ +29,4 @@ > NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(TextTrackCue) > NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper) > > +nsCOMPtr<nsIWebVTTParserService> TextTrackCue::sWebVTTService = 0; Shouldn't 0 be nullptr here? @@ +182,5 @@ > + return EmptyDocumentFragment(); > + } > + > + nsRefPtr<DocumentFragment> frag = static_cast<DocumentFragment*>(docFrag.get()); > + return already_AddRefed<DocumentFragment>(frag.forget()); This is convoluted. If you need to return an already_AddRefed, can you not just return docFrag.forget() directly? @@ +199,4 @@ > mTrack->CueChanged(*this); > } > } > + Trailing whitespace. ::: content/media/TextTrackCue.h @@ +312,5 @@ > // anytime a property that relates to the display of the TextTrackCue is > // changed. > bool mReset; > + > + static nsCOMPtr<nsIWebVTTParserService> sWebVTTService; Maybe I'm being dense, but can you explain why this is static? ::: content/media/WebVTTListener.cpp @@ +1,4 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ I believe git relies on being able to detect renames after the patch is applied. The patch would have to be redone in hg to get the explicit rename...perhaps just before landing. ::: content/media/webvtt/moz.build @@ +13,5 @@ > +] > + > +XPIDL_MODULE = 'webvtt'; > + > +MODULE = 'webvtt'; Should remove the semicolon here too, of course. If it helps, moz.build files use python syntax, so (unlike js) you don't want these to terminate statements. ::: content/media/webvtt/nsIWebVTTListener.idl @@ +1,4 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + Trailing whitespace.

Comment on attachment 787743 [details] [diff] [review] Part 2 v2: Add WebVTT parser service and WebVTT IDls. Review of attachment 787743 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/TextTrackCue.cpp @@ +29,4 @@ > NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(TextTrackCue) > NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper) > > +nsCOMPtr<nsIWebVTTParserService> TextTrackCue::sWebVTTService = 0; If this is supposed to be static (when is it released?), use StaticRefPtr. @@ +153,5 @@ > already_AddRefed<DocumentFragment> > +TextTrackCue::EmptyDocumentFragment() > +{ > + nsRefPtr<DocumentFragment> docFrag = mDocument->CreateDocumentFragment(); > + return docFrag.forget(); Why does this exist? @@ +182,5 @@ > + return EmptyDocumentFragment(); > + } > + > + nsRefPtr<DocumentFragment> frag = static_cast<DocumentFragment*>(docFrag.get()); > + return already_AddRefed<DocumentFragment>(frag.forget()); return docFrag.forget().downcast<DocumentFragment>(); @@ +199,4 @@ > mTrack->CueChanged(*this); > } > } > + This is good. ::: content/media/WebVTTListener.cpp @@ +99,5 @@ > +WebVTTListener::OnStopRequest(nsIRequest* aRequest, > + nsISupports* aContext, > + nsresult aStatus) > +{ > + if(mElement->mReadyState != HTMLTrackElement::ERROR) { 'if ('

Comment on attachment 787743 [details] [diff] [review] Part 2 v2: Add WebVTT parser service and WebVTT IDls. Review of attachment 787743 [details] [diff] [review]: ----------------------------------------------------------------- You should also get a review from a DOM peer for this. ::: content/media/WebVTTListener.cpp @@ +183,5 @@ > + > + rv = settings->GetAlign(align); > + NS_ENSURE_SUCCESS(rv, rv); > + // TODO: Add string method to take here. > + //cue->SetAlign(align); When do you add this? ::: content/media/webvtt/WebVTTParserService.js @@ +19,5 @@ > + > + newParserWrapper: function(callback) > + { > + var wrapper = Components.classes["@mozilla.org/webvttParserWrapper;1"]. > + createInstance(Ci.nsIWebVTTParserWrapper); nit:whitepace ::: content/media/webvtt/nsIWebVTTParserWrapper.idl @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "nsISupports.idl" > +#include "nsIWebVTTListener.idl" > +#include "nsIDOMDocumentFragment.idl" Why do you need nsIDOMDocumentFragment.idl?

BTW: Do we have any tests for this?

We have a lot of unit tests in the upstream repo. We should at least have an end to end test in our repo too though.

Can we please get the upstream tests running in our automation too?

We already have a few tests for WebVTT in content/media/test/test_texttrackcue.html and content/media/test/test_texttrack.html. (In reply to :Ms2ger from comment #37) > Can we please get the upstream tests running in our automation too? What's the best way to go about that? Right now vtt.js's test suite runs in NodeJS.

Comment on attachment 787743 [details] [diff] [review] Part 2 v2: Add WebVTT parser service and WebVTT IDls. Review of attachment 787743 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webvtt/nsIWebVTTParserService.idl @@ +6,5 @@ > +#include "nsIWebVTTCue.idl" > +#include "nsIWebVTTParserWrapper.idl" > +#include "nsIWebVTTListener.idl" > +#include "nsIDOMWindow.idl" > +#include "nsIDOMDocumentFragment.idl" Please remove includes by forward declarations as much as possible.

As long we keep using the vtt.js repo as upstream and make all changes there and then import them, I think its ok to run the unit tests there. The integration tests should run as part of our test suite, which it sounds like they already do.

Comment on attachment 787744 [details] [diff] [review] Part 3 v2: Add JS WebVTT parser code in (vtt.js) Review of attachment 787744 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webvtt/vtt.jsm @@ +281,5 @@ > + if (!node) > + continue; > + // Determine if the tag should be added based on the context of where it > + // is placed in the cuetext. > + if (!shouldAdd(current, node)) nit: whitespace @@ +338,5 @@ > + // after it is read. > + function collectNextLine(advance) { > + var buffer = self.buffer; > + var pos = 0; > + advance = typeof advance === "undefined" ? true : advance; nit: remove whitespace @@ +555,5 @@ > + } > +}; > + > +if (typeof module !== "undefined" && module.exports) { > + module.exports.WebVTTParser = WebVTTParser; Why is this needed?

Comment on attachment 787744 [details] [diff] [review] Part 3 v2: Add JS WebVTT parser code in (vtt.js) Review of attachment 787744 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webvtt/vtt.jsm @@ +30,5 @@ > + // First position is hours as it's over 59. > + return computeSeconds(m[1], m[2], 0, m[4]); > + else > + // Timestamp takes the form of [minutes]:[seconds].[milliseconds] > + return computeSeconds(0, m[1], m[2], m[4]); I want to see some braces for this if statement. @@ +64,5 @@ > + } > + }, > + // Accept a region if it doesn't have the special string '-->' > + region: function(k, v) { > + if(!v.match(/-->/)) { nit: if ( @@ +281,5 @@ > + if (!node) > + continue; > + // Determine if the tag should be added based on the context of where it > + // is placed in the cuetext. > + if (!shouldAdd(current, node)) nit: whitespace @@ +338,5 @@ > + // after it is read. > + function collectNextLine(advance) { > + var buffer = self.buffer; > + var pos = 0; > + advance = typeof advance === "undefined" ? true : advance; nit: remove whitespace @@ +523,5 @@ > + default: // BADCUE > + // 54-62 - Collect and discard the remaining cue. > + if (!line) { > + self.state = "ID"; > + continue; redundant 'continue'?

(In reply to Gregor Wagner [:gwagner] from comment #42) > Comment on attachment 787744 [details] [diff] [review] > Part 3 v2: Add JS WebVTT parser code in (vtt.js) > > Review of attachment 787744 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/webvtt/vtt.jsm > @@ +30,5 @@ > > + // First position is hours as it's over 59. > > + return computeSeconds(m[1], m[2], 0, m[4]); > > + else > > + // Timestamp takes the form of [minutes]:[seconds].[milliseconds] > > + return computeSeconds(0, m[1], m[2], m[4]); > > I want to see some braces for this if statement. Do you want braces for all the if statements (no curly braces is the style for vtt.js) or just this one because of the multiline comments?

(In reply to Rick Eyre (:reyre) from comment #43) > (In reply to Gregor Wagner [:gwagner] from comment #42) > > Comment on attachment 787744 [details] [diff] [review] > > Part 3 v2: Add JS WebVTT parser code in (vtt.js) > > > > Review of attachment 787744 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: content/media/webvtt/vtt.jsm > > @@ +30,5 @@ > > > + // First position is hours as it's over 59. > > > + return computeSeconds(m[1], m[2], 0, m[4]); > > > + else > > > + // Timestamp takes the form of [minutes]:[seconds].[milliseconds] > > > + return computeSeconds(0, m[1], m[2], m[4]); > > > > I want to see some braces for this if statement. > > Do you want braces for all the if statements (no curly braces is the style > for vtt.js) or just this one because of the multiline comments? Yeah because of the multiline comment.

(In reply to :Ms2ger from comment #33) > @@ +153,5 @@ > > already_AddRefed<DocumentFragment> > > +TextTrackCue::EmptyDocumentFragment() > > +{ > > + nsRefPtr<DocumentFragment> docFrag = mDocument->CreateDocumentFragment(); > > + return docFrag.forget(); > > Why does this exist? I added this as there are a lot of instances in TextTrackCue::GetCueAsHTML() where we have to return an empty document. > return docFrag.forget().downcast<DocumentFragment>(); Thanks for this Ms2ger it really helped.

(In reply to Gregor Wagner [:gwagner] from comment #41) > @@ +555,5 @@ > > + } > > +}; > > + > > +if (typeof module !== "undefined" && module.exports) { > > + module.exports.WebVTTParser = WebVTTParser; > > Why is this needed? This is specifically for our test suite written in node.js. I can remove it in the imported version.

(In reply to Ralph Giles (:rillian) from comment #32) > This is convoluted. If you need to return an already_AddRefed, can you not > just return docFrag.forget() directly? The problem was that docFrag.forget() would return an nsIDOMDocumentFragment when we need a DocumentFragment. So we need to do the cast at least. The extra nsRefPtr<> there was to avoid a crash that we were having. Ms2ger has given me a better way to do this now: return docFrag.forget().downcast<DocumentFragment>(); > Maybe I'm being dense, but can you explain why this is static? It's static because TextTrackCue only needs to call ConvertCueToDOMTree() which is a static function on the JS WebVTT parser. Similarly, I've changed WebVTTListener's WebVTTParserService to be static as well as it is only used by the WebVTTListener to get WebVTTParserWrapper objects. (In reply to Gregor Wagner [:gwagner] from comment #34) > When do you add this? Added it now. Sorry about that. > Why do you need nsIDOMDocumentFragment.idl? I didn't. Removed.

Updating commit message to have r=rillian. Carrying forward r=rillian.

Carrying forward r=rillian, ted.

Patch 2 now includes a rename from WebVTTLoadListener to WebVTTListener. Bugzilla doesn't pick up on this and you'll have to go to the raw diff to see it. Mercurial should accept this format as well.

I've included a script to update vtt.jsm in this. Can you review Ted? Carrying forward r=rillian.

Comment on attachment 790401 [details] [diff] [review] Part 3 v3: Add JS WebVTT parser code in (vtt.js) Review of attachment 790401 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for including that. You will thank yourself in the future. :) ::: content/media/webvtt/update-webvtt.js @@ +1,1 @@ > +#!/usr/bin/env node A node script to update your js library. Figures. :)

This needs a patch to turn test_texttrackcue.html back on as well. I'll add that tomorrow. (In reply to Ted Mielczarek [:ted.mielczarek] from comment #52) > A node script to update your js library. Figures. :) Only way to go! :)

Comment on attachment 791283 [details] [diff] [review] Part 4 v1: Turn TextTrackCue tests back on Great work!

(In reply to Andreas Gal :gal from comment #55) > Comment on attachment 791283 [details] [diff] [review] > Part 4 v1: Turn TextTrackCue tests back on > > Great work! Thanks! :)

Comment on attachment 790400 [details] [diff] [review] Part 2 v3: Add WebVTT parser service and WebVTT IDls. Review of attachment 790400 [details] [diff] [review]: ----------------------------------------------------------------- A DOM peer should review the DOM parts.

Comment on attachment 790401 [details] [diff] [review] Part 3 v3: Add JS WebVTT parser code in (vtt.js) Review of attachment 790401 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webvtt/update-webvtt.js @@ +45,5 @@ > + ' */\n' + > + vttjs; > + > + fs.writeFileSync(argv.w, vttjs); > +}); nice :) ::: content/media/webvtt/vtt.jsm @@ +38,5 @@ > +// A settings object holds key/value pairs and will ignore anything but the first > +// assignment to a specific key. > +function Settings() { > + this.values = Object.create(null); > +} I would prefer another less generic name like VTTSettins but it's up to you.

(In reply to Gregor Wagner [:gwagner] from comment #58) > nice :) :) > I would prefer another less generic name like VTTSettins but it's up to you. I'm fine with that name. I'll update it. Thanks for flagging khuey for me.

Comment on attachment 790400 [details] [diff] [review] Part 2 v3: Add WebVTT parser service and WebVTT IDls. Review of attachment 790400 [details] [diff] [review]: ----------------------------------------------------------------- I'm not a fan of this approach. Here's how I think this should work: 1. We should put a ChromeOnly static method on TextTrackCue that is basically a constructor (ChromeOnly constructors are more complicated). 2. We should ditch the parser service entirely, and just instantiate the parser wrapper directly via do_CreateInstance. 3. The parser wrapper should grab the element and the window of the thing it is creating and use window.TextTrackCue.createStuff to create the TextTrackCue in the oncue callback. 4. The parser wrapper should stick the TextTrackCue on the element directly. Don't know if we need more ChromeOnly APIs for that. ::: b2g/installer/package-manifest.in @@ +457,5 @@ > @BINPATH@/components/SettingsService.js > @BINPATH@/components/SettingsService.manifest > +@BINPATH@/components/WebVTT.manifest > +@BINPATH@/components/WebVTTParserService.js > +@BINPATH@/components/WebVTTParserWrapper.js You need to package this for the browser and fennec too, and file bugs on seamonkey and thunderbird ...

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #60) Thanks for review Kyle. > ::: b2g/installer/package-manifest.in > @@ +457,5 @@ > > @BINPATH@/components/SettingsService.js > > @BINPATH@/components/SettingsService.manifest > > +@BINPATH@/components/WebVTT.manifest > > +@BINPATH@/components/WebVTTParserService.js > > +@BINPATH@/components/WebVTTParserWrapper.js > > You need to package this for the browser and fennec too, and file bugs on > seamonkey and thunderbird ... Don't know how I missed this, heh. From further talks on IRC with Kyle we've decided it would be better to have vtt.js just create TextTrackCues (VTTCues) directly. The VTTCue has a content ctor which we can use and all the relevant attributes are writable. This way we don't have to create any kind of new API and the cues will be able to be passed back and forth out of the box. I'll update the patch accordingly. However, I think we may have to take the approach laid out by Kyle with VTTRegions. The VTTRegion has no content ctor so we wouldn't be able to do it that way.

I've added a few more things that are being removed to this patch. Mainly just unused includes.

There have been some significant changes due to Kyle's requests. Carrying forward r=ted.

Again, some big changes. Mainly in vtt.js. Carrying forward r=ted.

Please note that this these patches now depend on the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=868509 being applied.

Comment on attachment 795459 [details] [diff] [review] Part 2 v4: Add WebVTT parser wrapper Review of attachment 795459 [details] [diff] [review]: ----------------------------------------------------------------- Why doesn't WebVTTParserWrapper.js create the DOM objects directly? Why does it need to go back through WebVTTListener?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #66) > Comment on attachment 795459 [details] [diff] [review] > Part 2 v4: Add WebVTT parser wrapper > > Review of attachment 795459 [details] [diff] [review]: > ----------------------------------------------------------------- > > Why doesn't WebVTTParserWrapper.js create the DOM objects directly? Why > does it need to go back through WebVTTListener? The wrapper does create them directly. All the WebVTTListener does is add the VTTCue to the HTMLTrackElement's Track and VTTCue's reference to its Track object. So the workflow is: the VTT file gets parsed, VTTCues get passed back to the WebVTTListener's OnCue() callback, the WebVTTListener then adds the VTTCue to its owning HTMLTrackElement's Track.

Comment on attachment 795456 [details] [diff] [review] Part 1 v3: Remove references to C WebVTT parser and unused code Review of attachment 795456 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nit addressed. ::: content/media/TextTrackCue.cpp @@ -83,4 @@ > } > } > > -TextTrackCue::~TextTrackCue() You should also remove the declaration in TextTrackCue.h

Just a heads up; I'm unlikely to get to the other two parts before next week. Hopefully your other reviewers will have something to say in the meantime.

(In reply to Ralph Giles (:rillian) from comment #69) > Just a heads up; I'm unlikely to get to the other two parts before next > week. Hopefully your other reviewers will have something to say in the > meantime. No worries Ralph. I'm working on the processing model bug in JS as well so I have that to take my time.

Comment on attachment 795459 [details] [diff] [review] Part 2 v4: Add WebVTT parser wrapper Review of attachment 795459 [details] [diff] [review]: ----------------------------------------------------------------- r- primarily because of why I'm r-ing the following patch. ::: content/media/Makefile.in @@ +17,4 @@ > -I$(topsrcdir)/content/base/src \ > -I$(topsrcdir)/layout/generic \ > -I$(topsrcdir)/layout/xul/base/src \ > + -I$(topsrcdir)/xpcom/base \ What do you need this for? ::: content/media/TextTrackCue.cpp @@ +151,5 @@ > already_AddRefed<DocumentFragment> > +TextTrackCue::EmptyDocumentFragment() > +{ > + nsRefPtr<DocumentFragment> docFrag = mDocument->CreateDocumentFragment(); > + return docFrag.forget(); I think you can just return mDocument->CreateDocumentFragment() these days. @@ +187,5 @@ > + > + return docFrag.forget(); > +} > + > +void nit: whitespace at eol. ::: content/media/WebVTTListener.cpp @@ +152,5 @@ > { > + if (!aCue) > + return NS_ERROR_FAILURE; > + > + TextTrackCue* cue = static_cast<TextTrackCue*>(aCue); This should unwrap rather than just static_casting. You'll need to make this implicit_jscontext and take a jsval and then use UNWRAP_OBJECT. @@ +163,3 @@ > > +NS_IMETHODIMP > +WebVTTListener::OnRegion(nsISupports* aRegion) Same here, probably. ::: content/media/webvtt/nsIWebVTTParserWrapper.idl @@ +18,5 @@ > + * parse, flush, or watch. > + * > + * @param window The window that the parser will use to create VTTCues and > + * VTTRegions. > + * nit: whitespace at EOL.

Comment on attachment 795463 [details] [diff] [review] Part 3 v4: Add JS WebVTT parser code in (vtt.js) Review of attachment 795463 [details] [diff] [review]: ----------------------------------------------------------------- r- because this does not support parsing multiple WebVTT files simultaneously afaict. Please yell at me if I'm wrong ;-) Please write a test for this before resubmitting the patch. ::: content/media/webvtt/WebVTTParserWrapper.js @@ +23,1 @@ > }, There's only a single static WebVTTParserWrapper, so there's only a single WebVTTParser at any given time. Since WebVTT parsing is not atomic (data comes in in chunks off the network) you're gonna have a bad time if two WebVTT things try to load at the same time.

Thanks for review Kyle. (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #72) > r- because this does not support parsing multiple WebVTT files > simultaneously afaict. Please yell at me if I'm wrong ;-) > > Please write a test for this before resubmitting the patch. > > ::: content/media/webvtt/WebVTTParserWrapper.js > @@ +23,1 @@ > > }, > > There's only a single static WebVTTParserWrapper, so there's only a single > WebVTTParser at any given time. Since WebVTT parsing is not atomic (data > comes in in chunks off the network) you're gonna have a bad time if two > WebVTT things try to load at the same time. One HTMLTrackElement can only have one VTT resource at a single time. Each HTMLTrackElement has one WebVTTListener and one WebVTTParser. So it should be alright. The way multiple VTT files are loaded at once is by having multiple HTMLTrackElements on one MediaResource. So: > <video> > <track source="vtt1"> > <track source="vtt2> > </video> I can definitely write a test for this though to see if it works :).

I don't think you understand what I'm referring to. There is a single global WebVTTParserWrapper. If you load one WebVTT track and another later the order of operations is: Track 1 - call loadParser Track 1 - call parse several times Track 1 - call flush Track 2 - call loadParser Track 2 - call parse several times Track 2 - call flush What happens if these loads happen simultaneously and some of these calls are interleaved?

TextTrackCue has a static WebVTTParserWrapper, but each WebVTTListener has its own. So in this scenario each of the loads for the separate tracks would be handled by different WebVTTParserWrappers. Resetting review flags as per Kyle's request on IRC.

Comment on attachment 795459 [details] [diff] [review] Part 2 v4: Add WebVTT parser wrapper Review of attachment 795459 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comments I made earlier addressed.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #71) > ::: content/media/Makefile.in > @@ +17,4 @@ > > -I$(topsrcdir)/content/base/src \ > > -I$(topsrcdir)/layout/generic \ > > -I$(topsrcdir)/layout/xul/base/src \ > > + -I$(topsrcdir)/xpcom/base \ > > What do you need this for? Removed. Thought I needed this in order to include StaticPtr. > ::: content/media/TextTrackCue.cpp > @@ +151,5 @@ > > already_AddRefed<DocumentFragment> > > +TextTrackCue::EmptyDocumentFragment() > > +{ > > + nsRefPtr<DocumentFragment> docFrag = mDocument->CreateDocumentFragment(); > > + return docFrag.forget(); > > I think you can just return mDocument->CreateDocumentFragment() these days. Done. > @@ +187,5 @@ > > + > > + return docFrag.forget(); > > +} > > + > > +void > > nit: whitespace at eol. Done. > ::: content/media/WebVTTListener.cpp > @@ +152,5 @@ > > { > > + if (!aCue) > > + return NS_ERROR_FAILURE; > > + > > + TextTrackCue* cue = static_cast<TextTrackCue*>(aCue); > > This should unwrap rather than just static_casting. You'll need to make > this implicit_jscontext and take a jsval and then use UNWRAP_OBJECT. Done. > @@ +163,3 @@ > > > > +NS_IMETHODIMP > > +WebVTTListener::OnRegion(nsISupports* aRegion) > Done. > > ::: content/media/webvtt/nsIWebVTTParserWrapper.idl > @@ +18,5 @@ > > + * parse, flush, or watch. > > + * > > + * @param window The window that the parser will use to create VTTCues and > > + * VTTRegions. > > + * > > nit: whitespace at EOL. Done. Carrying forward r=khuey.

Comment on attachment 795463 [details] [diff] [review] Part 3 v4: Add JS WebVTT parser code in (vtt.js) Review of attachment 795463 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, but please address the nits. ::: content/media/webvtt/WebVTTParserWrapper.js @@ +23,1 @@ > }, Can you elaborate? That sounds like the sort of thing that's can very easily happen, but is hard to test. Rick, can you open a bug for this? ::: content/media/webvtt/update-webvtt.js @@ +18,5 @@ > + > +var repo = gift(argv.d); > + > +repo.commits("master", 1, function(err, commits) { > + var vttjs = fs.readFileSync(argv.d + "/vtt.js", 'utf8'); This copies the current checkout, which might be different from the master commit. Can you use "HEAD" instead of master, or gift.head to get the revision of the file you're copying? You should also use status.clean to check whether the there are uncommitted changes. @@ +38,5 @@ > + ' * file, You can obtain one at http://mozilla.org/MPL/2.0/. */\n\n' + > + 'this.EXPORTED_SYMBOLS = ["WebVTTParser"];\n\n' + > + '/**\n' + > + ' * Code below is vtt.js the JS WebVTTParser.\n' + > + ' * Current source code can be found at http://github.com/andreasgal/vtt.js\n' + Seems like it would be nice if the github url should come from argv.d's current branch's upstream or something, but hard-coding it is fine if you can't think of an easy way.

Comment on attachment 799749 [details] [diff] [review] Part 5 v1: Test for multiple WebVTT files loading at once Review of attachment 799749 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/test/test_bug895091.html @@ +40,5 @@ > + // element has loaded its data. > + if (trackElement.readyState == 1 || trackElementTwo.readyState == 1) { > + setTimeout(run_tests, 0); > + return; > + } Are we really not firing readystatechange events (or any other event that lets you observe this)?

Comment on attachment 799748 [details] [diff] [review] Part 2 v5: Add WebVTT parser wrapper Review of attachment 799748 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/WebVTTListener.cpp @@ +1,1 @@ > + nit: newline

(In reply to Ralph Giles (:rillian) from comment #79) > ::: content/media/webvtt/WebVTTParserWrapper.js > @@ +23,1 @@ > > }, > > Can you elaborate? That sounds like the sort of thing that's can very easily > happen, but is hard to test. Rick, can you open a bug for this? Sorry, we discussed this more on IRC and the full extent of that convo didn't come through in my post. TextTrackCue uses a static reference to a WebVTTParserWrapper as it only ever uses it to convert cues into DOM trees, these calls are all synchronous. Whereas, each WebVTTListener has its own WebVTTParserWrapper. So the loading of each VTT file is handled by completely separate parsers. I've added a test to as per Kyle's request to ensure this. > This copies the current checkout, which might be different from the master > commit. Can you use "HEAD" instead of master, or gift.head to get the > revision of the file you're copying? > > You should also use status.clean to check whether the there are uncommitted > changes. I've changed it up so that it checks if the directory is clean and if so checks out the master branch and then copies the file. This ensures we're getting the same file that the commit references. > Seems like it would be nice if the github url should come from argv.d's > current branch's upstream or something, but hard-coding it is fine if you > can't think of an easy way. We can get the URL of the origin and any other remotes, however, we can't get the remote of the most upstream repository on GitHub. My intentions with this comment were to give a reference to where the official repo lives and we can't figure that out through gift or git. If its alright with you I'd like to leave this to just being manually updated. Carrying forward r=rillian, khuey, ted, gwagner.

(In reply to Gregor Wagner [:gwagner] from comment #81) > ::: content/media/WebVTTListener.cpp > @@ +1,1 @@ > > + > > nit: newline Done. Thanks for review!

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #80) > Are we really not firing readystatechange events (or any other event that > lets you observe this)? We cleared this up on IRC, but I'll reply here as well. Right now in the spec the only way to figure out if an HTMLTrackElement has loaded its data is by checking its ReadyState. There is no event. The only other event that can be observed is the MediaElement's 'onloadedmetadata' event. However, the bits that ensure this fires after the TextTracks are ready hasn't been implemented yet. Kyle suggested we should try to get an 'onreadystatechange' event on the HTMLTrackElement in the future. I'll open a bug for that.

Just rebased. Carrying forward r=rillian. Try push: https://tbpl.mozilla.org/?tree=Try&rev=752af529bd5e

For some reason the tryserver couldn't find the typelib of nsIWebVTTListener. Kyle recommended to include XPIDL_MODULE = 'webvtt' in the moz.build file and include webvtt.xpt in the package-manifest files and it works now.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=f552a87fc715

Kyle asked me to remove the DOM object flag from the JS WebVTTParserWrapper component's class information. Try push: https://tbpl.mozilla.org/?tree=Try&rev=c34fa4ea7d3f Just did mochitest-1 to confirm that the DOM object doesn't mess up the parsing.

This fixes the failure on B2G. Gregory said the problem was that on B2G builds JSMs need to have the symbols that are exported be attached to 'this'. I've updated it so that the update-webvtt.js script adds a single line to the bottom of vtt.jsm -- "this.WebVTTParser = WebVTTParser". Try push: https://tbpl.mozilla.org/?tree=Try&rev=ab13a259a045 There's now a failure on bug895091 in a few places where it's getting the incorrect number of cues.

That change is what https://github.com/andreasgal/vtt.js/blob/master/vtt.js#L675 is doing for the node.js case (i.e., this === module.exports in a node module), so why not just remove the condition and always do `this.WebVTTParser = WebVTTParser;`?

(In reply to David Humphrey (:humph) from comment #90) > That change is what > https://github.com/andreasgal/vtt.js/blob/master/vtt.js#L675 is doing for > the node.js case (i.e., this === module.exports in a node module), so why > not just remove the condition and always do `this.WebVTTParser = > WebVTTParser;`? That sounds like a much better idea Dave. Thanks I'll do that.

The number of cues being loaded seems to be an intermittent problem only effecting the test where we try to load two VTT files at once.

One thing you could do to vtt.js is wrap it like this: (function(global) { function WebVTTParser() { ... } ... global.WebVTTParser = WebVTTParser; }(this)); This will do the right thing in the browser, b2g, or node.js contexts, and also help isolate things a bit more for the browser case.

Just rebased to current. Carrying forward r=rillian.

The problem of incorrect number of cues, I believe, was caused by assuming that the buffer passed to WebVTTParserWrapper::Parse was null terminated. In regards to this I've changed it so that WebVTTParserWrapper::Parse takes the length of the buffer to as an argument and copies over that much. It seems as if this has solved the issue, but since it's intermittent we'll have to see. Carrying forward r=khuey, gwagner, ted, rillian.

I've changed it so that update-webvtt.js no longer adds this.WebVTTParser line to vtt.js. Instead we've implemented Dave's suggestion of wrapper vtt.js in a function expression and exporting WebVTTParser on the global (this). This takes care of the B2G problem. Carrying forward r=ted, rillian. The newest version of vtt.js that has these changes also has some extra changes. I'm not sure if you'd like to review those Gregor and Kyle?

Carrying forward r=khuey. I've changed the test to take a long vtt file (2000 cues). I was able to reliably reproduce the problems of incorrect number of cues at these sizes. So that is what we should be testing against.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=f430933af112 The patches don't show up in the push as I pushed them earlier and ran a small set of tests.

Comment on attachment 802324 [details] [diff] [review] Part 3 v7: Add JS WebVTT parser code in (vtt.js) No I don't want to rereview.

Try push to verify again that the intermittent problem is gone: https://tbpl.mozilla.org/?tree=Try&rev=dc133bcc3221.

Interdiff between Part 3 v6 and v8 requested by Gregor.

Comment on attachment 803036 [details] [diff] [review] Part 3 v6 v8 interdiff Review of attachment 803036 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webvtt/vtt.jsm @@ +320,5 @@ > + return 100; > + } > + > + function CueBoundingBox(cue) { > + nit: remove newline. @@ +345,5 @@ > + if (cue.position <= 50) > + maxLen = cue.position * 2; > + else > + maxLen = (100 - cue.position) * 2; > + } Can we get into a state where maxLen is undefined and we return undefined? @@ +354,5 @@ > + if (cue.vertical === "") { > + if (direction === "ltr") { > + if (cue.align === "start" || cue.align === "left") > + return cue.position; > + else if (cue.align === "end" || cue.align === "right") nit: throughout this function there is no need for else after return.

Thanks Gregor.

Try looks green: https://tbpl.mozilla.org/?tree=Try&rev=f5f9fa68f77e

https://hg.mozilla.org/integration/mozilla-inbound/rev/f56acf83688c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e110a9187bb1 https://hg.mozilla.org/integration/mozilla-inbound/rev/330a415e7723 https://hg.mozilla.org/integration/mozilla-inbound/rev/feca3867b395 https://hg.mozilla.org/integration/mozilla-inbound/rev/813a35c5b24a

Backed out for ASan bustage running test_bug895091. See bug 918041. test: https://hg.mozilla.org/integration/mozilla-inbound/rev/861cdeda898e and the feature: https://hg.mozilla.org/integration/mozilla-inbound/rev/159553d6e855 https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3c7aa902e7 https://hg.mozilla.org/integration/mozilla-inbound/rev/5f1ad58a3886 https://hg.mozilla.org/integration/mozilla-inbound/rev/a4b29680fab5

Carrying forward r=khuey,ted,gwagner,ralph. Small change was discussed on IRC. See bug 918041 comment 3 for explanation: https://bugzilla.mozilla.org/show_bug.cgi?id=918041#c3

Try push: https://tbpl.mozilla.org/?tree=Try&rev=69f4d1a55df5 And an ASAN one in case the Trychooser problem still exists: https://tbpl.mozilla.org/?tree=Try&rev=3dbadf0efe1a

Try is green. Cross your fingers! https://hg.mozilla.org/integration/mozilla-inbound/rev/8974a95a4149 https://hg.mozilla.org/integration/mozilla-inbound/rev/19ceac099a12 https://hg.mozilla.org/integration/mozilla-inbound/rev/a797a0cd579f https://hg.mozilla.org/integration/mozilla-inbound/rev/14d6115639e7 https://hg.mozilla.org/integration/mozilla-inbound/rev/d7dcd9309577

https://hg.mozilla.org/mozilla-central/rev/8974a95a4149 https://hg.mozilla.org/mozilla-central/rev/19ceac099a12 https://hg.mozilla.org/mozilla-central/rev/a797a0cd579f https://hg.mozilla.org/mozilla-central/rev/14d6115639e7 https://hg.mozilla.org/mozilla-central/rev/d7dcd9309577