Closed Bug 833382 Opened 11 years ago Closed 11 years ago

[webvtt] Implement WebVTTParser to manage the libwebvtt parser

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: reyre, Assigned: reyre)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [fuzzing:rforbes])

Attachments

(2 files, 26 obsolete files)

1.18 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
35.86 KB, patch
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.52 Safari/537.17
Component: Untriaged → Video/Audio
OS: Windows 7 → All
Product: Firefox → Core
Hardware: x86_64 → All
Version: 21 Branch → unspecified
Spin-off from bug 629350 to implement media decoder bits for webvtt.
Assignee: nobody → rick.eyre
Blocks: webvtt
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: [webvtt] Implement Track element and TextTrack* DOM classe → [webvtt] Implement TextTrackDecoder to manage the libwebvtt decoder
Version: unspecified → Trunk
From dev-media, some of cpearce's thoughts:

The W.I.P. patch (see https://bug629350.bugzilla.mozilla.org/attachment.cgi?id=606673)is already downloading the webvtt file using an nsIChannel. This is good. The data is delivered to you incrementally in chunks in the callback HTMLTrackElement::LoadListener::OnDataAvailable(). In that function you should pass the data off to the incremental webvtt parser, and then when cues are ready you can then construct the TextTrackCue objects and attach them to their owning TextTrack by calling the C++ implementation of TextTrack.addCue().

nsHTMLMediaElement::FireTimeUpdate(bool) then needs to use the TextTrackList API to query whether it should change the cue displayed on the screen.

I think that you should create a WebVTTParser object that manages the webvtt_parser_t, and manage the parsing from there. This can be owned (i.e. created, destroyed, and the owning reference/pointer held) by the HTMLTrackElement. Even better, we can just turn the existing HTMLTrackElement::LoadListener into this, as then we're right on the receiving end of the incoming unparsed data.

So I'd recommend your list of things to do is this, building on top of Ralph's patch:

1. Rename HTMLTrackElement::LoadListener to WebVTTParser, and split it
   out into its own file. Or have the LoadListener forward
   OnDataAvailable/OnStopRequest calls to the WebVTTParser.
2. Change HTMLTrackElement to store a nsRefPtr<WebVTTParser> reference
   to the parser that you create in HTMLTrackElement::LoadResource().
3. WebVTTParser::OnDataAvailable() is currently creating and destroying
   a new webvtt_parser_t on every call. OnDataAvailable is called
   multiple times, everytime we have a new chunk of the file
   downloaded. So we should instead create the webvtt_parser_t once per
   WebVTTParser, say in the constructor, or in a new Init() method.
4. Change WebVTTParser::OnDataAvailable() to use the parser created
   from step 3 incrementally and parse the chunk of data that was just
   downloaded.
5. Extract the cue from the webvtt_parser_t (they're reported in a
   callback right?) and use the TextTrack.addCue() API to add them to
   the appropriate TextTrack object.
6. Change nsHTMLMediaElement::FireTimeupdate() to query the
   TextTrackList and update the cue being displayed on the video frame
   in a timely fashion.

Once you've got that working, you then need to check the spec [1] and ensure the right text tracks are being loaded. It looks to me like we just load text tracks for all <track> elements which have a src attribute that are added to a document, we're only supposed to load the <track> elements with a "default" attribute?
Summary: [webvtt] Implement TextTrackDecoder to manage the libwebvtt decoder → [webvtt] Implement WebVTTParser to manage the libwebvtt parser
Attached patch Work done so far (obsolete) — Splinter Review
So this is the progress we've made so far. We've accomplished steps 1 - 4 from Chris's instructions. 

- We've hooked the webvtt_parser into the WebVTTLoadListener and specified the call back functions. 
- Specified private functions to convert the cue's to DOM Elements.
- Mostly completed the cCueToDomCue function which turns a cue into a TextTrackCue.
- Implemented a cNodeListToDocumentFragment that loops through the head node's children and appends them as children.

Main things that still need to be tackled:

- Adding the Document Fragment to the HTMLMediaElement's Caption Overlay. This is still a place for confusion for us.
- Translating the parsed webvtt nodes into HTMLElements. This is kind of confusing as well. Are we to just make generic HTMLElements to hold these or should we use HTMLFontElement, etc, for the different nodes. 
- The last thing that needs to be accomplished is setting some of the properties on the HTMLTrackElement such as Vertical and Align which receive nsAString. We're still kind of confused what the format of the string should be for them i.e. should we be passing in "center", "top", etc, for Align?
- Need to figure out the appropriate way to handle errors returned from the webvtt parser.
Thanks for fixing that for me Chris. I'll make sure to check is patch from now on.
I've updated the patch to only show changes that this bug specifically addresses.
Attachment #708423 - Attachment is obsolete: true
I incorrectly diffed the last update against old code. I'll be more careful next time.

Please note my comments on https://bugzilla.mozilla.org/show_bug.cgi?id=833382#c3 still apply to this update.
Attachment #708831 - Attachment is obsolete: true
I haven't had a chance to go over this in detail yet, but here's some drive-by comments:

- Use an nsAutoRef to manage the lifetime of mParser.  See http://mxr.mozilla.org/mozilla-central/source/content/media/AudioStream.cpp#25 and http://mxr.mozilla.org/mozilla-central/source/content/media/AudioStream.cpp#759 for an example.
- Use an nsAutoArrayPtr to manage the lifetime of buffer in OnDataAvailable.
- Look at using ReadSegments in OnDataAvailable to avoid extra buffer copies.  See http://mxr.mozilla.org/mozilla-central/source/content/media/MediaResource.cpp#490 for an example
- parsedCue and reportError need to be wrapped in standalone functions or static member functions, since you can't pass member functions in to webvtt's API directly.
Thanks for the tips Matthew. There much appreciated. We'll get that into the next patch that we post.
Attached patch Another WIP (obsolete) — Splinter Review
I've attempted to implement all the things that Matthew mentioned in his comment. Some things that I know still need to be done:

- Finish CCueToTextTrackCue
- Finish CNodeToHtmlElement
- Get it building with patches from bugs 833385, 833388, 833403
Attachment #708836 - Attachment is obsolete: true
Attachment #713918 - Flags: feedback?(kinetik)
Attachment #713918 - Flags: feedback?(cpearce)
Comment on attachment 713918 [details] [diff] [review]
Another WIP

Review of attachment 713918 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, you approach here looks good.

::: content/html/content/src/WebVTTLoadListener.cpp
@@ +41,5 @@
> +{
> +  webvtt_parser *parser;
> +  
> +  webvtt_create_parser(&__parsedCue, &__reportError, this, parser);
> +  if (!parser) {

We don't expect this to fail right? If so use:
NS_ENSURE_TRU(parser != nullptr, NS_ERROR_FAILURE);

Then we'll get a warning written to stdout (or maybe stderr? not sure) to notify us.

@@ +46,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  mParser.own(parser);
> +  if (!mParser) {

Ditto.

@@ +53,5 @@
> +
> +  return NS_OK;
> +}
> +
> +NS_METHOD WevVTTLoadListener::WebVTTParseChunk(nsIInputStream *aInStream, void *aClosure,

Return value on its own line, e.g.:

NS_METHOD
WevVTTLoadListener::WebVTTParseChunk(...)
{
  // ....
}

@@ +59,5 @@
> +                                               uint32_t aCount, uint32_t *aWriteCount)
> +{
> +  // How to determine if this is the final chunk?
> +  if (!webvtt_parse_chunk(mParser, aFromSegment, aCount, 0))
> +  {

Braces in if statements riding high, i.e.

if (condition) {
  // ...
}

Instead of:
if (condition)
{
  // ...
}

W.R.T. handling errors here, you probably still want to keep parsing the remainder of the file if possible, so printing a warning might suffice.

@@ +75,5 @@
> +                            const PRUnichar* aData)
> +{
> +  nsContentUtils::UnregisterShutdownObserver(this);
> +
> +  // Clear mElement to break cycle so we don't leak on shutdown

You should check aTopic to ensure that you're responding to the right notification, e.g.:
http://mxr.mozilla.org/mozilla-central/source/content/media/MediaDecoder.cpp#869

@@ +107,5 @@
> +  nsresult rv;
> +  uint64_t available;
> +  bool blocking;
> +
> +  rv = aStream->IsNonBlocking(&blocking);

You're not using this?

@@ +167,5 @@
> +    nsIContent *overlay = static_cast<nsVideoFrame*>(frame)->GetCaptionOverlay();
> +    nsCOMPtr<nsIDOMNode> div = do_QueryInterface(overlay);
> +
> +    if (div) {
> +      div->appendChild(frag);

You're appending the child here? Do you need to remove previously appended chilren?

@@ +179,5 @@
> +  // TODO: Handle error here
> +}
> +
> +TextTrackCue 
> +WebVTTLoadListener::CCueToTextTrackCue(const webvtt_cue aCue)

Why do you prefix these function names with "C"? That's not our convention.

Are the callbacks? In general we use names like "OnFooBarred" to name callbacks which occur when a "Foo" has been "Barred".

@@ +224,5 @@
> +  return frag;
> +}
> +
> +HtmlElement
> +WebVTTLoadListener::CNodeToHtmlElement(const webvtt_node aWebVttNode)

Maybe you should pass the webvtt_node by reference here, rather than making a shallow copy.

@@ +249,5 @@
> +
> +  return htmlElement;
> +}
> +
> +static void WEBVTT_CALLBACK

These don't need to belong to WebVTTLoadListener, they can just be static functions defined in this class.

You should just name them OnCueParsedWebVTTCallback or somesuch, rather than prefixing with "__", as that makes it more obvious what they're used for.

::: content/html/content/src/WebVTTLoadListener.h
@@ +46,5 @@
> +                                       uint32_t aFlags,
> +                                       nsIAsyncVerifyRedirectCallback* cb);
> +  NS_IMETHODIMP GetInterface(const nsIID &aIID, void **aResult);
> +
> +protected:

Why can't these by private? What subclass needs to call these?
Attachment #713918 - Flags: feedback?(cpearce) → feedback+
Comment on attachment 713918 [details] [diff] [review]
Another WIP

Review of attachment 713918 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/WebVTTLoadListener.cpp
@@ +46,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  mParser.own(parser);
> +  if (!mParser) {

It's not necessary to check this at all, you've already established parser is non-null.

@@ +53,5 @@
> +
> +  return NS_OK;
> +}
> +
> +NS_METHOD WevVTTLoadListener::WebVTTParseChunk(nsIInputStream *aInStream, void *aClosure,

Wev/Web typo.  Also, the function can just be named ParseChunk.

You should, if possible, make sure your patches compile before asking for feedback/review.

@@ +65,5 @@
> +  }
> +
> +  *aWriteCount = aCount;
> +
> +  return NS_OK && (*aWriteCount > 0)

Since this function returns an nsresult, you need to return NS_OK or an appropriate NS_ERROR*.  NS_OK is 0, so this will always return 0.  That's probably not what you want.

@@ +111,5 @@
> +  rv = aStream->IsNonBlocking(&blocking);
> +  NS_ENSURE_SUCCESS(rv,rv);
> +
> +  rv = aStream->Available(&available);
> +  NS_ENSURE_SUCCESS(rv, rv);

The Available call isn't used or needed, either.

@@ +250,5 @@
> +  return htmlElement;
> +}
> +
> +static void WEBVTT_CALLBACK
> +WebVTTLoadListener::__parsedCue(void *aUserData, webvtt_cue *aCue)

Identifiers containing double underscores probably falls foul of C's reserved identifiers.

::: content/html/content/src/WebVTTLoadListener.h
@@ +67,5 @@
> +
> +} // namespace dom
> +} // namespace mozilla
> +
> +#endif // mozilla_dom_WEBVTTLoadListner_h
\ No newline at end of file

Typo in listener here and at the #ifdef.
Attachment #713918 - Flags: feedback?(kinetik) → feedback+
Attached patch Addressed most of the feedback (obsolete) — Splinter Review
Thanks for you feedback Matthew and Chris.

I've addressed most of the things brought up in the feedback in this WIP. The things that I haven't are:

- Removing previous children of the div element as I'm not sure if this is needed yet.
- Moving the GetInterface and other functions to being private as I'm unsure if they need to be public yet. OnDataAvailable definitely needs to be public so that the track element can call it. 
- Removing the check for mParser to be null. I've kept it as in the example that Matthew provided a check is done there. So I'm unsure what should be done - http://mxr.mozilla.org/mozilla-central/source/content/media/AudioStream.cpp#767

I've changed the names of the functions prefixed with "C", CNodeToTextTrackCue for example, to ConvertNodeToTextTrackCue because I've been looking around and seeing that this is what is done in other parts of the mozilla code when a function converts one thing to another.

I'd like to have addressed more than this, but I've been requested to step back from this bug for a while until we get the actually webvtt parser up and running, so I wanted to get this patch up so someone else might be able to work on it.
Attachment #713918 - Attachment is obsolete: true
Blocks: 833385
No longer blocks: 833385
Depends on: 833385
Depends on: 860338
Attached patch Current progress. (obsolete) — Splinter Review
Attachment #714577 - Attachment is obsolete: true
Depends on: 853589
Blocks: 863485
Blocks: 863488
Attached patch Current progress (obsolete) — Splinter Review
Attachment #737319 - Attachment is obsolete: true
Attachment #739355 - Flags: feedback?(kinetik)
Attachment #739355 - Flags: feedback?(cpearce)
Attachment #739355 - Flags: feedback?(bzbarsky)
Comment on attachment 739355 [details] [diff] [review]
Current progress

It would help to document what the anonymous content setup here is...

Still need to observe redirects on the channel.

Please add the new members to cycle collection; otherwise this will leak.
Attachment #739355 - Flags: feedback?(bzbarsky) → feedback+
Whiteboard: [fuzzing:rforbes]
Attached patch Current progress (obsolete) — Splinter Review
- Addressed bz's review
- Finished initializing TextTrackCue properly in WebVTTLoadListener
- Call to finish_parsing now in OnStopRequest
- Other minor changes
Attachment #739355 - Attachment is obsolete: true
Attachment #739355 - Flags: feedback?(kinetik)
Attachment #739355 - Flags: feedback?(cpearce)
Attachment #742159 - Flags: feedback?(kinetik)
Attachment #742159 - Flags: feedback?(cpearce)
Attachment #742159 - Flags: feedback?(bzbarsky)
Rick, are there particular things you're looking for feedback on at this point?  If you just want feedback on the changes you made, would you mind attaching an interdiff?  Otherwise, please be clear about what the goal of the feedback request is?
Sorry about that Boris. Hope this is better!
Attachment #742324 - Flags: feedback?(bzbarsky)
Attachment #742324 - Attachment is patch: true
Attachment #742159 - Flags: feedback?(bzbarsky)
Boris, I'd like to get your comments on the cycle collection, channel redirection, and anonymous content documentation that I've done as per your last review comments. 

For Chris and Mathew I'd like to get feedback on the WebVTTLoadListener in order to make sure that we are doing everything correctly in it.
Comment on attachment 742324 [details] [diff] [review]
Interdiff between attachment 739355 [details] [diff] [review] and current

Thank you very much for the interdiff!  Makes this much simpler.

It's not clear to me why we have an mNextListener member: we never set it, as far as I can tell.

The QI impl for WebVTTLoadListener needs to include QI to nsISupports.

Why do we still need the shutdown observer bits?  Aren't those leaking the load listener until shutdown by holding a reference to it in the observer service?

The assert in HTMLTrackElement::OnChannelRedirect is clearly wrong.  The whole point of needing that method is that aChannel != aNewChannel.

The documentation looks great.  So the point is that this anonymous stuff is completely managed content-side and then on the layout side we end up doing .... something sometime, right?  Sounds good.
Attachment #742324 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 742159 [details] [diff] [review]
Current progress

Review of attachment 742159 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, looks good. Lots of great work here, keep it up!

::: content/html/content/src/WebVTTLoadListener.cpp
@@ +87,5 @@
> +WebVTTLoadListener::Observe(nsISupports* aSubject,
> +                            const char *aTopic,
> +                            const PRUnichar* aData)
> +{
> +  if (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {

The only thing your shutdown observer does is remove itself as a shutdown observer. Maybe it should either do something else, or not exist in the first place. ;)

@@ +123,5 @@
> +{
> +  uint32_t count = aCount;
> +  while (count > 0) {
> +    uint32_t read;
> +    nsresult rv = aStream->ReadSegments(ParseChunk, this, count, &read);

Maybe we should be doing the parsing in a worker thread, so that we keep the amount of work done on the main thread to a minimum, but for the time being this is OK.

@@ +181,5 @@
> +  const char* text = webvtt_string_text(&aCue->body);
> +
> +  nsRefPtr<TextTrackCue> textTrackCue =
> +      new TextTrackCue(mElement->OwnerDoc()->GetParentObject(),
> +                       (double)(aCue->from/1000), (double)(aCue->until/1000),

You're better off either defining an inline function SECONDS_TO_MS(s) or defining USECS_PER_S=1000 and dividing by that instead of dividing by raw number literal, it's too easy to miss 0 on raw literals.

We define USECS_PER_S in content/media/VideoUtils.h, so you could use that, or define a SECONDS_TO_MS (or whatever is appropriate for your conversion here) and add that to VideoUtils.h.

::: content/media/TextTrackCue.cpp
@@ +86,5 @@
> +    CreateCueOverlay();
> +  }
> +
> +  HTMLMediaElement* parent =
> +      static_cast<HTMLMediaElement*>(mTrackElement->mMediaParent.get());

Can mTrackElement be null?

@@ +145,5 @@
> +
> +  return frag.forget();
> +}
> +
> +// TODO: Change to iterative solution instead of recursive

Yes, please change this to an iterative solution, else the Bad Guys might be able to smash your stack.

It would be good if you could break up this function into logical sub-functions too, it's quite long.
Attachment #742159 - Flags: feedback?(cpearce) → feedback+
(In reply to Boris Zbarsky (:bz) from comment #20)
> Comment on attachment 742324 [details] [diff] [review]
> Interdiff between attachment 739355 [details] [diff] [review] and current
> 
> The assert in HTMLTrackElement::OnChannelRedirect is clearly wrong.  The
> whole point of needing that method is that aChannel != aNewChannel.

It's checking that the currnt mChannel is equal to what the function call believes is the current mChannel. I don't know if the ASSERT is necessary -- I copied it from HTMLMediaElement's OnChannelRedirect function. http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#537

Thanks for the review comments.
> It's checking that the currnt mChannel is equal to what the function call believes is the
> current mChannel

Ah, good point.  So it is.
Blocks: 871747
Comment on attachment 742159 [details] [diff] [review]
Current progress

Review of attachment 742159 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry about the delay giving feedback on this. :-(

::: content/html/content/src/WebVTTLoadListener.cpp
@@ +45,5 @@
> +WebVTTLoadListener::~WebVTTLoadListener()
> +{
> +  if (mParser) {
> +    mParser.reset();
> +  }

You can remove this block because nsAutoRef's destructor handles it already, and that'll be called automatically when WebVTTLoadListener is destroyed.

@@ +52,5 @@
> +
> +nsresult
> +WebVTTLoadListener::LoadResource()
> +{
> +  webvtt_parser_t *parser = 0;

Use nullptr rather than 0.  Also, the * should hug the type rather than the variable.

@@ +77,5 @@
> +
> +  NS_ENSURE_TRUE(parser != nullptr, NS_ERROR_FAILURE);
> +
> +  mParser.own(parser);
> +  NS_ENSURE_TRUE(mParser != nullptr, NS_ERROR_FAILURE);

You don't need to check that parser and mParser are null, just check one or the other.  There's no way mParser could be null after mParser.own(parser) if parser is non-null.  What I'd suggest is doing the same thing that the example nsAutoRef use I linked to earlier does: don't null check parser, just call mParser.own(parser) and null-check that.

@@ +108,5 @@
> +                                  nsresult aStatus)
> +{
> +  nsContentUtils::UnregisterShutdownObserver(this);
> +  
> +  webvtt_finish_parsing( mParser );

No spaces inside () here.

@@ +115,5 @@
> +}
> +
> +NS_IMETHODIMP
> +WebVTTLoadListener::OnDataAvailable(nsIRequest* aRequest,
> +                                    nsISupports *aContext,

* should hug the type rather than the argument name.

(This also applies in a bunch of other places.  The style should be consistent within the file.)

@@ +167,5 @@
> +{
> +  WebVTTLoadListener* loadListener = static_cast<WebVTTLoadListener *>(aClosure);
> +
> +  if (!webvtt_parse_chunk(loadListener->mParser, aFromSegment, aCount)) {
> +    LOG("UNABLE TO PARSE CHUNK OF WEBVTT TEXT.");

What's supposed to happen after webvtt_parse_chunk fails in this situation?  Do we need to signal an error and abort?

@@ +229,5 @@
> +  }
> +  textTrackCue->SetAlign(align);
> +
> +  ErrorResult rv;
> +  mElement->mTrack->AddCue(*textTrackCue.get(), rv);

You need to check rv and handle failure here, otherwise the TextTrackCue could leak.

Another option is to add an AddCue overload (or new function called AddCueInternal or something) that makes the transfer of ownership clearer:

  void AddCue(already_AddRefed<TextTrackCue> aCue);

  mElement->mTrack->AddCue(textTrackCue.forget());
Attachment #742159 - Flags: feedback?(kinetik) → feedback+
Attached patch Bug 833382v1 (obsolete) — Splinter Review
- Addressed review comments
- Updated to current tree in master

We will be pushing on bug 833385 and once that lands will try to push on this to get it landed.
Attachment #742159 - Attachment is obsolete: true
Attachment #742324 - Attachment is obsolete: true
Attached patch Bug 833382v2 (obsolete) — Splinter Review
Update based on try build: https://tbpl.mozilla.org/?tree=Try&rev=a5f59f86d294
Attachment #752006 - Attachment is obsolete: true
Attached patch Bug 833382.v3 (obsolete) — Splinter Review
Fixes based on try build https://tbpl.mozilla.org/?tree=Try&rev=bd6d9f1399ce
Attachment #752030 - Attachment is obsolete: true
Attached patch Bug 833382v4 (obsolete) — Splinter Review
Changed to use nsDeque instead of nsTArray for keeping track of the node stack in the C node tree to DOM tree conversion code based on cpearce's recommendations.
Attachment #752074 - Attachment is obsolete: true
Attached patch Bug 833382v5 (obsolete) — Splinter Review
Updated based on https://tbpl.mozilla.org/?tree=Try&rev=2d491f0137e8
Attachment #752491 - Attachment is obsolete: true
Attached patch Bug 833382v6 (obsolete) — Splinter Review
Updated based on try https://tbpl.mozilla.org/?tree=Try&rev=3c81b240ab3d

Had to add webvtt_finish_parsing to symbols.def.in in order to be able to link it on Windows 7.
Attachment #752551 - Attachment is obsolete: true
Blocks: 875169
Comment on attachment 752632 [details] [diff] [review]
Bug 833382v6

Review of attachment 752632 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/html.css
@@ +741,5 @@
> +  text-shadow:
> +    -2px -2px 1px #000,
> +    2px -2px 1px #000,
> +    -2px 2px 1px #000,
> +    2px 2px 1px #000;

Please request ui-review on these changes. I'm kind of disappointed bug 833388 (which added the bits you're modifying) had already landed without any visual design input.

Most of this style should probably live in the app's theme, anyway.
Attachment #752632 - Flags: review-
Anyone I should be asking in particular for ui-review? 

Also, this feature is still behind a pref so we will have more time to fix it in the future. We're wanting to just get basic initial support for WEBVTT in this patch.
Attached patch Bug 833382v7 (obsolete) — Splinter Review
- Fixed some minor comments about the new iterative solution for conversion of DOM nodes.

I don't think an interdiff will be possible on this patch from the last one that was reviewed as a lot of the code had to be changed when I updated to the current rev in master. Also my workflow got kind of screwed up which will make it difficult as well. I'll manage that better next time.
Attachment #752632 - Attachment is obsolete: true
Attachment #753097 - Flags: ui-review?(dolske)
Attachment #753097 - Flags: review?(cpearce)
Attachment #753097 - Flags: review?(bzbarsky)
I'm not going to get to this until at least Tuesday, at best.
Comment on attachment 753097 [details] [diff] [review]
Bug 833382v7

Review of attachment 753097 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few nits to pick, plus the graph search needs work.

::: content/media/TextTrackCue.cpp
@@ +110,5 @@
> +  if (!mTrackElement) {
> +    return;
> +  }
> +  
> +  HTMLMediaElement* parent =

Do you need to null check parent here? Is it possible for another event runner to run and remove this from its parent? Maybe you should null check it anyway...

@@ +114,5 @@
> +  HTMLMediaElement* parent =
> +      static_cast<HTMLMediaElement*>(mTrackElement->mMediaParent.get());
> +
> +  nsIFrame* frame = parent->GetPrimaryFrame();
> +  if(!frame || frame->GetType() != nsGkAtoms::HTMLVideoFrame) {

"if ("

@@ +152,5 @@
> +  return frag.forget();
> +}
> +
> +void
> +TextTrackCue::ConvertNodeTreeToDOMTree(nsIContent* parentContent)

aParentContent

@@ +155,5 @@
> +void
> +TextTrackCue::ConvertNodeTreeToDOMTree(nsIContent* parentContent)
> +{
> +  nsDeque nodeStack;
> +  nsDeque childBranchStack;

childBranchStack is unused, remove it.

@@ +174,5 @@
> +      if (childCount > 0) {
> +        if (nodeStack.GetSize() > 0) {
> +          // Remember the last branching parent so that we know when we need to
> +          // move back up the DOM tree.
> +          parentStack.Push((void*)nodeStack.Peek());

I can't find where parentStack is defined...

@@ +201,5 @@
> +    }
> +
> +    // If we've reached the last branching parent then we have processed all the
> +    // current child nodes -- move back up the DOM tree.
> +    if (nodeStack.GetSize() > 0 && (parentStack.Peek() == nodeStack.Peek())) {

Rather than having to keep track of what the parent is, I think you should have a stack of struct { webvtt_node*,IContent* parent} instead, so that you don't need to manage both stacks, so that you remove the possibility of making a mistake to keep the two stacks in sync. Remember to delete the stucts you pop out of the stack (I recommend putting the pop'd elements in an nsAutoPtr).

@@ +233,5 @@
> +    case WEBVTT_RUBY_TEXT:
> +      atomName = nsGkAtoms::rt;
> +      break;
> +    case WEBVTT_VOICE:
> +    {

You're injecting a scope here than spans two cases (the {}).

@@ +261,5 @@
> +
> +    if (genericHtmlElement) {
> +      text = webvtt_string_text(&aWebVTTNode->data.internal_data->annotation);
> +      if (text) {
> +        genericHtmlElement->SetTitle(NS_ConvertUTF8toUTF16(text));

I'm not sure if this section is correct, is setting the title the correct thing to do here?

bz or ms2ger might know.

@@ +297,5 @@
> +{
> +  nsCOMPtr<nsIContent> cueTextContent;
> +  nsNodeInfoManager* nimgr = mTrackElement->NodeInfo()->NodeInfoManager();
> +  switch (aWebVTTNode->kind) {
> +      case WEBVTT_TEXT:

Indentation is off here.

::: content/media/TextTrackCue.h
@@ +309,5 @@
> +  /**
> +   * Converts a leaf webvtt node, i.e. one that does not have children, to
> +   * either a text node or processing instruction.
> +   */
> +  nsCOMPtr<nsIContent>

In general, if you want to ensure what you're returning must be stored by the caller in a refcounted smart pointer, you should return already_AddRefed<T> (by calling forget() on a comptr on the stack), you don't want to be returning nsCOMPtr<T>s by value as that creates a copy of the smart pointer, which may actually release the refcount.

::: content/media/TextTrackList.h
@@ +38,4 @@
>      return mTextTracks.Length();
>    }
>  
> +  void Update(double time);

aTime, add comment stating time units, seconds I presume.

::: content/media/WebVTTLoadListener.cpp
@@ +239,5 @@
> +}
> +
> +int WEBVTT_CALLBACK
> +WebVTTLoadListener::OnReportErrorWebVTTCallBack(void* aUserData, uint32_t aLine,
> +                            uint32_t aCol, webvtt_error aError)

Indentation here.

::: content/media/WebVTTLoadListener.h
@@ +55,5 @@
> +  nsRefPtr<mozilla::dom::HTMLTrackElement> mElement;
> +  nsAutoRef<webvtt_parser_t> mParser;
> +
> +  static void WEBVTT_CALLBACK OnParsedCueWebVTTCallBack(void* aUserData,
> +                                                      webvtt_cue* aCue);

Nit: Indentation is off here, and on the arguments to OnReportErrorWebVTTCallBack.
Attachment #753097 - Flags: review?(cpearce) → review-
Attached patch Patch version 8 (obsolete) — Splinter Review
Thanks for the review Chris. Hopefully I've addressed all your comments here.
Attachment #753097 - Attachment is obsolete: true
Attachment #753097 - Flags: ui-review?(dolske)
Attachment #753097 - Flags: review?(bzbarsky)
Attachment #753699 - Flags: ui-review?(dolske)
Attachment #753699 - Flags: review?(cpearce)
Attachment #753699 - Flags: review?(bzbarsky)
Comment on attachment 753699 [details] [diff] [review]
Patch version 8

Review of attachment 753699 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, looks good. r=cpearce, with the nits below picked. Thanks for the patch!

::: content/media/TextTrackCue.cpp
@@ +152,5 @@
> +
> +  return frag.forget();
> +}
> +
> +struct StackInfo {

You could call it something more descriptive, like WebVTTNodeParentPair, or somesuch.

@@ +171,5 @@
> +  // mHead should actually be the head of a node tree.
> +  if (mHead->kind != WEBVTT_HEAD_NODE) {
> +    return;
> +  }
> +  // Push the children of mHead onto the stack. If we don't do this

Comment is out of date; you're populating nodeStack with the children to seed the traversal of the tree, you never pass mHead to ConvertInternalNodeToContent.

@@ +174,5 @@
> +  }
> +  // Push the children of mHead onto the stack. If we don't do this
> +  // ConvertInternalNodeToContent() will return null when passed mHead and the
> +  // tree will not be processed properly.
> +  for (int i = mHead->data.internal_data->length; i > 0; i--) {

Factor this for loop out into its own static function, passing in the webvtt node and the nsDeque. Then you can call it here and inside the WEBVTT_IS_VALID_INTERNAL_NODE block below, and your code will be much more understandable.

::: content/media/TextTrackCue.h
@@ +303,5 @@
> +   * Converts an internal webvtt node, i.e. one that has children, to an
> +   * anonymous HTMLElement.
> +   */
> +  already_AddRefed<nsIContent>
> +  ConvertInternalNodeToContent( const webvtt_node *aWebVTTNode );

You have inconsistent spacings, sometimes you do:

returnType
FunctionName( argumentType argumentName )

And other times you do:

returnType
FunctionName(argumentType argumentName)

The later is the prevalent style in mozilla code.

::: content/media/TextTrackList.cpp
@@ +24,5 @@
> +void
> +TextTrackList::Update(double time)
> +{
> +  uint32_t length = Length(), i;
> +  for( i = 0; i < length; i++ ) {

for (i = 0; i < length; i++) {


Be consistent.

::: content/media/VideoUtils.h
@@ +164,5 @@
> +// Number of milliseconds per second.
> +static const int64_t MS_PER_S = 1000;
> +
> +// Converts seconds to milliseconds.
> +#define SECONDS_TO_MS(s) s / MS_PER_S

#define SECONDS_TO_MS(s) ((s) / MS_PER_S)

To safeguard against operator precedence causing problems in future if people pass expressions in that have higher precedence.

::: content/media/WebVTTLoadListener.cpp
@@ +223,5 @@
> +
> +  nsAutoCString file = NS_ConvertUTF16toUTF8(autoString);
> +
> +  const char* error = "parser error";
> +  if( aError >= 0 )

if (aError >= 0) {
  error = webvtt_strerror(aError);
}

Always have braces, except on trivial "if (FailedCondition)\n\t return error;" blocks.
Attachment #753699 - Flags: review?(cpearce) → review+
Comment on attachment 753699 [details] [diff] [review]
Patch version 8

Review of attachment 753699 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/HTMLTrackElement.h
@@ +19,5 @@
>  #include "nsIDOMHTMLElement.h"
>  #include "nsIDOMEventTarget.h"
>  #include "nsIHttpChannel.h"
> +
> +namespace mozilla { namespace dom { class WebVTTLoadListener; } }

Move the forward declaration into the namespaces below

::: content/media/TextTrackCue.cpp
@@ +87,2 @@
>  {
> +  nsNodeInfoManager *nodeInfoManager =

* to the left

@@ +109,5 @@
> +    return;
> +  }
> +
> +  HTMLMediaElement* parent =
> +      static_cast<HTMLMediaElement*>(mTrackElement->mMediaParent.get());

Fishy... Wasn't mMediaParent supposed to be an HTMLMediaElement already?

@@ +119,5 @@
> +  if (!frame || frame->GetType() != nsGkAtoms::HTMLVideoFrame) {
> +    return;
> +  }
> +
> +  nsIContent *overlay = static_cast<nsVideoFrame*>(frame)->GetCaptionOverlay();

do_QueryFrame instead of cast?

@@ +121,5 @@
> +  }
> +
> +  nsIContent *overlay = static_cast<nsVideoFrame*>(frame)->GetCaptionOverlay();
> +  nsCOMPtr<nsINode> div = do_QueryInterface(overlay);
> +  nsCOMPtr<nsINode> cueDiv = do_QueryInterface(mCueDiv);

Don't need those

@@ +128,5 @@
> +    return;
> +  }
> +
> +  ErrorResult rv;
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(div);

No QI

@@ +131,5 @@
> +  ErrorResult rv;
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(div);
> +  if (content) {
> +    nsContentUtils::SetNodeTextContent(content, EmptyString(), true);
> +    div->AppendChild(*cueDiv, rv);

Probably return if rv failed

@@ +152,5 @@
> +
> +  return frag.forget();
> +}
> +
> +struct StackInfo {

{ to the left

@@ +153,5 @@
> +  return frag.forget();
> +}
> +
> +struct StackInfo {
> +  webvtt_node* node;

mFoo

@@ +154,5 @@
> +}
> +
> +struct StackInfo {
> +  webvtt_node* node;
> +  nsIContent* parent;

What's the ownership model here?

@@ +159,5 @@
> +
> +  StackInfo(webvtt_node* aNode, nsIContent* aParent)
> +    : node(aNode)
> +    , parent(aParent)
> +    {}

{} two spaces to the left

@@ +190,5 @@
> +
> +      // Push on in reverse order so we process the nodes in the correct
> +      // order -- left to right.
> +      for (int i = stackInfo->node->data.internal_data->length; i > 0; i--) {
> +        nodeStack.Push((void*)new StackInfo(

Do you need this cast? C++-style cast in any case

@@ +197,5 @@
> +    }
> +    if (content && stackInfo->parent) {
> +      ErrorResult rv;
> +      nsCOMPtr<nsINode> childNode = do_QueryInterface(content);
> +      nsCOMPtr<nsINode> parentNode = do_QueryInterface(stackInfo->parent);

Pointless QIs

@@ +234,5 @@
> +      atomName = nsGkAtoms::span;
> +      break;
> +    default:
> +      return nullptr;
> +      break;

No point in having this break.

@@ +245,5 @@
> +  nsCOMPtr<nsIContent> cueTextContent;
> +  NS_NewHTMLElement(getter_AddRefs(cueTextContent), nodeInfo.forget(),
> +                    mozilla::dom::NOT_FROM_PARSER);
> +
> +  nsCOMPtr<nsGenericHTMLElement> genericHtmlElement;

This is not an interface, so nsRefPtr

@@ +246,5 @@
> +  NS_NewHTMLElement(getter_AddRefs(cueTextContent), nodeInfo.forget(),
> +                    mozilla::dom::NOT_FROM_PARSER);
> +
> +  nsCOMPtr<nsGenericHTMLElement> genericHtmlElement;
> +  const char* text;

Declare this when you use it.

@@ +248,5 @@
> +
> +  nsCOMPtr<nsGenericHTMLElement> genericHtmlElement;
> +  const char* text;
> +  if (aWebVTTNode->kind == WEBVTT_VOICE) {
> +    genericHtmlElement = do_QueryInterface(cueTextContent);

This QI is probably broken

@@ +268,5 @@
> +      classString.Append(NS_ConvertUTF8toUTF16(text));
> +    }
> +
> +    for (uint32_t i = 1; i < classes->length; i++) {
> +      classString.Append(NS_LITERAL_STRING("."));

Append('.')

@@ +284,5 @@
> +  return cueTextContent.forget();
> +}
> +
> +already_AddRefed<nsIContent>
> +TextTrackCue::ConvertLeafNodeToContent( const webvtt_node* aWebVTTNode )

No spaces within parens

@@ +295,5 @@
> +      cueTextContent = new nsTextNode(nimgr);
> +
> +      if (!cueTextContent) {
> +        return nullptr;
> +      }

Can't happen

@@ +314,5 @@
> +      break;
> +    }
> +    default:
> +      return nullptr;
> +      break;

As before

::: content/media/TextTrackCue.h
@@ +13,4 @@
>  #include "mozilla/dom/DocumentFragment.h"
>  #include "mozilla/dom/TextTrack.h"
>  #include "mozilla/dom/TextTrackCueBinding.h"
> +#include "mozilla/dom/DocumentFragment.h"

Already included within the context...

@@ +296,5 @@
> +   * http://dev.w3.org/html5/webvtt/#webvtt-cue-text-dom-construction-rules
> +   * Current rules taken from revision on May 13, 2013.
> +   */
> +  void
> +  ConvertNodeTreeToDOMTree(nsIContent *aParentContent);

* to the left

::: content/media/TextTrackCueList.h
@@ +44,1 @@
>    void Update(double time);

aTime

::: content/media/TextTrackList.cpp
@@ +21,5 @@
>    SetIsDOMBinding();
>  }
>  
> +void
> +TextTrackList::Update(double time)

aTime

::: content/media/TextTrackList.h
@@ +38,5 @@
>      return mTextTracks.Length();
>    }
>  
> +  // Time is in seconds.
> +  void Update(double time);

Ditto

::: content/media/VideoUtils.h
@@ +164,5 @@
> +// Number of milliseconds per second.
> +static const int64_t MS_PER_S = 1000;
> +
> +// Converts seconds to milliseconds.
> +#define SECONDS_TO_MS(s) s / MS_PER_S

No need for this to be a macro.

::: content/media/WebVTTLoadListener.cpp
@@ +29,5 @@
> +#else
> +# define LOG(msg)
> +#endif
> +
> +WebVTTLoadListener::WebVTTLoadListener(mozilla::dom::HTMLTrackElement* aElement)

No qualification

@@ +32,5 @@
> +
> +WebVTTLoadListener::WebVTTLoadListener(mozilla::dom::HTMLTrackElement* aElement)
> +  : mElement(aElement)
> +{
> +  NS_ABORT_IF_FALSE(mElement, "Must pass an element to the callback");

MOZ_ASSERT

@@ +50,5 @@
> +nsresult
> +WebVTTLoadListener::LoadResource()
> +{
> +  webvtt_parser_t* parser = nullptr;
> +  webvtt_status status;

Declare those below. This isn't C.

@@ +67,5 @@
> +  if (status != WEBVTT_SUCCESS) {
> +    NS_ENSURE_TRUE(status == WEBVTT_OUT_OF_MEMORY,
> +                   NS_ERROR_OUT_OF_MEMORY);
> +    NS_ENSURE_TRUE(status == WEBVTT_INVALID_PARAM,
> +                   NS_ERROR_INVALID_ARG);

You have those conditionals inverted

@@ +106,5 @@
> +    uint32_t read;
> +    nsresult rv = aStream->ReadSegments(ParseChunk, this, count, &read);
> +
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    NS_ASSERTION(read > 0, "Read 0 bytes while data was available?" );

If you believe it, MOZ_ASSERT.

@@ +128,5 @@
> +}
> +
> +NS_IMETHODIMP
> +WebVTTLoadListener::GetInterface(const nsIID &aIID,
> +                                 void** aResult)

Do you actually need this? Why?

@@ +142,5 @@
> +  WebVTTLoadListener* loadListener = static_cast<WebVTTLoadListener*>(aClosure);
> +
> +  if (WEBVTT_FAILED(webvtt_parse_chunk(loadListener->mParser, aFromSegment,
> +                                       aCount))) {
> +    LOG("UNABLE TO PARSE CHUNK OF WEBVTT TEXT. ABORTING.");

No all-caps

@@ +156,5 @@
> +{
> +  const char* text = webvtt_string_text(&aCue->body);
> +
> +  nsRefPtr<TextTrackCue> textTrackCue =
> +      new TextTrackCue(mElement->OwnerDoc()->GetParentObject(),

Indentation

@@ +176,5 @@
> +      vertical = NS_LITERAL_STRING("lr");
> +      break;
> +    case WEBVTT_VERTICAL_RL:
> +       vertical = NS_LITERAL_STRING("rl");
> +       break;

Indentation

@@ +204,5 @@
> +      break;
> +  }
> +  textTrackCue->SetAlign(align);
> +
> +  mElement->mTrack->AddCue(*textTrackCue.get());

Check if you need the .get()

@@ +218,5 @@
> +  DOMString tmp;
> +  mElement->GetSrc(tmp);
> +
> +  nsAutoString autoString;
> +  tmp.ToString(autoString);

nsString wideFile;
mElement->GetSrc(wideFile);

@@ +220,5 @@
> +
> +  nsAutoString autoString;
> +  tmp.ToString(autoString);
> +
> +  nsAutoCString file = NS_ConvertUTF16toUTF8(autoString);

NS_ConvertUTF16toUTF8 file(wideFile);

@@ +233,5 @@
> +
> +void WEBVTT_CALLBACK
> +WebVTTLoadListener::OnParsedCueWebVTTCallBack(void* aUserData, webvtt_cue* aCue)
> +{
> +  WebVTTLoadListener* self = reinterpret_cast<WebVTTLoadListener*>(aUserData);

static_cast

@@ +242,5 @@
> +WebVTTLoadListener::OnReportErrorWebVTTCallBack(void* aUserData, uint32_t aLine,
> +                                                uint32_t aCol,
> +                                                webvtt_error aError)
> +{
> +  WebVTTLoadListener* self = reinterpret_cast<WebVTTLoadListener*>(aUserData);

static_cast

::: content/media/WebVTTLoadListener.h
@@ +40,5 @@
> +  NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(WebVTTLoadListener,
> +                                           nsIStreamListener)
> +
> +public:
> +  WebVTTLoadListener(mozilla::dom::HTMLTrackElement* aElement);

No need for the mozilla::dom::

@@ +42,5 @@
> +
> +public:
> +  WebVTTLoadListener(mozilla::dom::HTMLTrackElement* aElement);
> +  ~WebVTTLoadListener();
> +  void OnParsedCue(webvtt_cue* cue);

aFoo

@@ +43,5 @@
> +public:
> +  WebVTTLoadListener(mozilla::dom::HTMLTrackElement* aElement);
> +  ~WebVTTLoadListener();
> +  void OnParsedCue(webvtt_cue* cue);
> +  void OnReportError(uint32_t line, uint32_t col, webvtt_error error);

Ditto

@@ +51,5 @@
> +  static NS_METHOD ParseChunk(nsIInputStream* aInStream, void* aClosure,
> +                              const char* aFromSegment, uint32_t aToOffset,
> +                              uint32_t aCount, uint32_t* aWriteCount);
> +
> +  nsRefPtr<mozilla::dom::HTMLTrackElement> mElement;

No need for the qualification

::: layout/media/symbols.def.in
@@ +581,4 @@
>  webvtt_parse_chunk
>  webvtt_ref_node
>  webvtt_release_node
> +webvtt_finish_parsing

Sort?

::: layout/xul/base/src/moz.build
@@ +6,4 @@
>  
>  MODULE = 'layout'
>  
> +EXPORTS += ['nsBox.h']

EXPORTS += [
    'nsBox.h',
]
Ms2ger wrote:

>> +struct StackInfo {
>> +  webvtt_node* node;
>> +  nsIContent* parent;
>
> What's the ownership model here?

In webvtt_node has its own refcount and is shared with the webvtt_parser context. We webvtt_ref_node/webvtt_release_node mHead in the TextTrackCue ctor/dtor; it's a long-lived reference to the head of the webvtt_node tree. Could do the same thing in StackInfo, but since the lifetime is limited to ::ConvertNodeTreeToDOMTree it isn't necessary.
(In reply to :Ms2ger from comment #38)
> ::: content/media/VideoUtils.h
> @@ +164,5 @@
> > +// Number of milliseconds per second.
> > +static const int64_t MS_PER_S = 1000;
> > +
> > +// Converts seconds to milliseconds.
> > +#define SECONDS_TO_MS(s) s / MS_PER_S
> 
> No need for this to be a macro.

This was one of the review requests from :cpearce.
Blocks: 876505
Attached patch Patch version 9 (obsolete) — Splinter Review
Attachment #753699 - Attachment is obsolete: true
Attachment #753699 - Flags: ui-review?(dolske)
Attachment #753699 - Flags: review?(bzbarsky)
Comment on attachment 754570 [details] [diff] [review]
Patch version 9

Carrying forward r=cpearce
Attachment #754570 - Flags: review?(bzbarsky)
This was requested by Ms2ger as a better alternative to do_QueryInterface on nsVideoFrame. Patch version 9 is dependent on this one.
Attachment #754572 - Flags: review?(bzbarsky)
(In reply to :Ms2ger from comment #38)
> @@ +67,5 @@
> > +  if (status != WEBVTT_SUCCESS) {
> > +    NS_ENSURE_TRUE(status == WEBVTT_OUT_OF_MEMORY,
> > +                   NS_ERROR_OUT_OF_MEMORY);
> > +    NS_ENSURE_TRUE(status == WEBVTT_INVALID_PARAM,
> > +                   NS_ERROR_INVALID_ARG);
> 
> You have those conditionals inverted

Looks good as far as I can tell: http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h#250
(In reply to Rick Eyre (:reyre) from comment #44)
> (In reply to :Ms2ger from comment #38)
> > @@ +67,5 @@
> > > +  if (status != WEBVTT_SUCCESS) {
> > > +    NS_ENSURE_TRUE(status == WEBVTT_OUT_OF_MEMORY,
> > > +                   NS_ERROR_OUT_OF_MEMORY);
> > > +    NS_ENSURE_TRUE(status == WEBVTT_INVALID_PARAM,
> > > +                   NS_ERROR_INVALID_ARG);
> > 
> > You have those conditionals inverted
> 
> Looks good as far as I can tell:
> http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h#250

Expands to:

if (!(status == WEBVTT_OUT_OF_MEMORY)) {
  return NS_ERROR_OUT_OF_MEMORY;
}

if (!(status == WEBVTT_INVALID_PARAM)) {
  return NS_ERROR_INVALID_ARG;
}
(In reply to :Ms2ger from comment #45)
> Expands to:
> 
> if (!(status == WEBVTT_OUT_OF_MEMORY)) {
>   return NS_ERROR_OUT_OF_MEMORY;
> }
> 
> if (!(status == WEBVTT_INVALID_PARAM)) {
>   return NS_ERROR_INVALID_ARG;
> }

Ah, alright. Thanks... the assert/ensure true things in Gecko always get messed up in my head.
Justin (:dolske) we'd like to land the CSS style stuff in https://bugzilla.mozilla.org/show_bug.cgi?id=876505 instead. The CSS style changes in this patch are only temporary and WEBVTT is currently behind a pref so it should be alright. We will request ui-review on bug 876505.
Depends on: 875305
Carrying forward r=cpearce.

In order to build you need the query frame changes attached to this bug and https://bugzilla.mozilla.org/show_bug.cgi?id=875305 However, bug 875305 should be landing in the tree soon so depending on when this is reviewed it might not need to be applied.
Attachment #754570 - Attachment is obsolete: true
Attachment #754570 - Flags: review?(bzbarsky)
Attachment #755538 - Flags: review?(bzbarsky)
Attachment #754572 - Attachment description: Add Query Frame Support to nsVideoFrame → Part 1 v1:add query frame support for nsVideoFrame
Attachment #755538 - Attachment description: Patch version 10 → Part 2 v10:implement the WebVTTLoadListener
I'm sorry for the lag here....

This is at the top of my list for Monday.
Blocks: 833386
Comment on attachment 754572 [details] [diff] [review]
Part 1 v1:add query frame support for nsVideoFrame

r=me
Attachment #754572 - Flags: review?(bzbarsky) → review+
Comment on attachment 755538 [details] [diff] [review]
Part 2 v10:implement the WebVTTLoadListener

>+++ b/content/base/src/moz.build

>+    'nsGenericDOMDataNode.h',
>+    'nsTextNode.h',

LOCAL_INCLUDES in content/media, please, rather than exporting these to the world.

>+++ b/content/html/content/src/HTMLTrackElement.cpp
>+nsresult
>+HTMLTrackElement::OnChannelRedirect(nsIChannel* aChannel,

Why is this not returning void?

> nsresult
>+HTMLTrackElement::SetAcceptHeader(nsIHttpChannel* aChannel)

As far as I can tell, this is unused.  Am I just missing something?

>+HTMLTrackElement::LoadResource(nsIURI* aURI)
>+  int16_t shouldLoad = nsIContentPolicy::ACCEPT;

To quote my review comments from bug 833385 comment 48:  "You need a CheckLoadURIWithPrincipal check here too.  It needs to come before the content policy check."

>+                                 EmptyCString(), // mime type

Should this really be empty, or do we have a typical MIME type here?

>+                     nsICachingChannel::LOAD_BYPASS_LOCAL_CACHE_IF_BUSY,

Please document why that flag is needed.

>+  mLoadListener->LoadResource();

Why is WebVTTLoadListener::LoadResource returning nsresult if the return value is ignored?

>@@ -235,7 +321,7 @@ HTMLTrackElement::BindToTree(nsIDocument* aDocument,
>+        LoadResource(uri);

Again, if the return value is ignored, why is it nsresult?

But also, this is not OK.  See bug 833385 comment 48.


>+++ b/content/html/content/src/HTMLTrackElement.h
>+  nsresult LoadResource(nsIURI* aURI);

Needs documentation.

>+++ b/content/media/TextTrackCue.cpp
>+  nsNodeInfoManager* nodeInfoManager =
>+    mTrackElement->NodeInfo()->NodeInfoManager();
>+  nsCOMPtr<nsINodeInfo> nodeInfo =
>+    nodeInfoManager->GetNodeInfo(nsGkAtoms::div, nullptr, kNameSpaceID_XHTML,

Why not mTrackElement->OwnerDoc()->CreateElem(), at least, instead of copy/pasting this wheel?

>+TextTrackCue::RenderCue()
>+  HTMLMediaElement* parent = mTrackElement->mMediaParent.get();

Why do you need the .get()?

>+  nsIFrame* frame = parent->GetPrimaryFrame();

How do you know "parent" is not null?  What if mTrackElement's parent is not a media element?

>+  if (!frame) {

What causes this code to rerun if and when parent gets a frame in the future?  Again, testcases?

>+  nsContentUtils::SetNodeTextContent(overlay, EmptyString(), true);
>+  overlay->AppendChild(*mCueDiv, rv);
>+
>+  nsContentUtils::SetNodeTextContent(mCueDiv, EmptyString(), true);

I recommend doing the SetNodeTextContent on mCueDiv and appending kids to it _before_ appending mCueDiv to the parent.

>+PushChildren(nsDeque* aNodeParentPairStack, webvtt_node* aNode,

Except it's not actually a stack, since you don't pop off the last thing you pushed, right?

>+  nsDeque nodeParentPairStack;

If you're really using this as a stack, why not use an nsTArray<WebVTTNodeParentPair> and avoid all the heap-allocation?

>+TextTrackCue::ConvertInternalNodeToContent(const webvtt_node* aWebVTTNode)
>+  nsCOMPtr<nsINodeInfo> nodeInfo = nimgr->GetNodeInfo(atomName, nullptr,

Once again, why not CreateElem?

>+    if (genericHtmlElement) {

How could that possibly test false?

>+      classString.Append(NS_ConvertUTF8toUTF16(text));

  AppendUTF8toUTF16(text, classString);


>+    for (uint32_t i = 1; i < classes->length; i++) {
>+      classString.Append('.');

Why '.' and not ' '?  Also, should this really be appended if !webvtt_string_text(classes->items) and i == 1?

>+        classString.Append(NS_ConvertUTF8toUTF16(text));

Again, AppendUTF8toUTF16.

>+    if (genericHtmlElement) {

Again, how can this be null?

It looks like ConvertInternalNodeToContent can return null from the "default" case, but the caller is not expecting that.  If that code is unreached, it should be asserting...

What's the ownership model for TextTrackCue?  Can it never form a cycle through mCueDiv?

>+++ b/content/media/VideoUtils.h
>+// Number of milliseconds per second.
>+static const int64_t MS_PER_S = 1000;

PR_MSEC_PER_SEC?

>+// Converts seconds to milliseconds.
>+#define SECONDS_TO_MS(s) ((s) / MS_PER_S)

>+++ b/content/media/WebVTTLoadListener.cpp
>+WebVTTLoadListener::ParseChunk(nsIInputStream* aInStream, void* aClosure,
>+    LOG("Unable to parse chunk of WEBVTT text. Aborting.");
>+    aInStream->Close();

Why is this closing the stream but claiming to have read it instead of just returning an error?

In any case, if this fails isn't it entirely possible for ReadSegments to end up reading 0 bytes?

>+WebVTTLoadListener::OnParsedCue(webvtt_cue* aCue)
>+    default:
>+      vertical = EmptyString();

It was already empty.

>+  textTrackCue.forget();

Wait.  Who took over the reference?  This at least needs really good documentation here and at the site where the reference is taken....

>+WebVTTLoadListener::OnReportError(uint32_t aLine, uint32_t aCol,
>+  nsAutoCString file = NS_ConvertUTF16toUTF8(wideFile);

  NS_ConvertUTF16toUTF8 file(wideFile);

>+WebVTTLoadListener::OnParsedCueWebVTTCallBack(void* aUserData, webvtt_cue* aCue)
>+WebVTTLoadListener::OnReportErrorWebVTTCallBack(void* aUserData, uint32_t aLine,

Are we guaranteed that these callbacks will not get called after OnStopRequest completes (and hence that aUserData cannot be stale)?

>+++ b/content/media/WebVTTLoadListener.h

Could use a tiny bit more documentation here.

>+++ b/content/media/moz.build
>@@ -82,6 +82,7 @@ EXPORTS.mozilla.dom += [
>+    'WebVTTLoadListener.h'

Should this really be exported?  Is it part of Gecko's public API?

>+++ b/layout/generic/moz.build
>@@ -12,6 +12,8 @@ EXPORTS += [
>+    'nsContainerFrame.h',
>+    'nsFrame.h',
>+    'nsSplittableFrame.h',
>+    'nsVideoFrame.h',

Those four should _definitely_ not be exported.

>+++ b/layout/xul/base/src/moz.build
>+  'nsBox.h',

Likewise!

EXPORTS means "any Gecko consumer, including various embedders and binary addons can use this", not "oh, I can now include it more easily in Gecko code"!

As usual, I'd love an interdiff addressing the comments.
Attachment #755538 - Flags: review?(bzbarsky) → review-
Blocks: 879426
(In reply to Boris Zbarsky (:bz) from comment #51)
> 
> >+PushChildren(nsDeque* aNodeParentPairStack, webvtt_node* aNode,
> 
> Except it's not actually a stack, since you don't pop off the last thing you
> pushed, right?
> 
> >+  nsDeque nodeParentPairStack;
> 
> If you're really using this as a stack, why not use an
> nsTArray<WebVTTNodeParentPair> and avoid all the heap-allocation?

We are popping off the last thing that we pushed here (line 183):

nsAutoPtr<WebVTTNodeParentPair> nodeParentPair(
	static_cast<WebVTTNodeParentPair*>(nodeParentPairStack.Pop()));

We started to us nsDeque as cpearce recommended it. What would be the better option? I'm fine with either. nsTArray has more code popping and pushing though.

> Why '.' and not ' '?  Also, should this really be appended if
> !webvtt_string_text(classes->items) and i == 1?

I think I get your meaning correctly i.e. if we don't receive text on the first
item but have an overall length greater than 1 ? Which could result in a string
like ".class-selector-1.class-selector-2" ? Or even "...." if all the text is
messed up. I'll change so that won't be possible.

> It looks like ConvertInternalNodeToContent can return null from the
> "default" case, but the caller is not expecting that.  If that code is
> unreached, it should be asserting...

What will happen right now is that all the children that are generated after
it returns null will end up not being added to the tree. This happens in the
check for content and mParent just before it adds it to the DOM tree:

if (content && nodeParentPair->mParent) {
	ErrorResult rv;
	nodeParentPair->mParent->AppendChild(*content, rv);
}

We had it checking for content == nullptr, right after calling ConvertInternalNodeToConent, before and if so not pushing it as a 'node parent' onto the stack of nodes that need to be traversed. The rationale for changing it to this new way was that if one node is not processed correctly we won't see any of the cue text that is under that node. In this way it might be easier to see that there is a problem as there is more of a visual cue.

> >+  textTrackCue.forget();
> 
> Wait.  Who took over the reference?  This at least needs really good
> documentation here and at the site where the reference is taken....

Not sure if this is correct, but the cue gets added to TextTrack's
TextTrackList which is of nsTArray< nsRefPtr< TextTrackCue > > via
nsTArray::AppendElement:

nsTArray< nsRefPtr<TextTrackCue> > mList;
mList.AppendElement(&cue);

If my understanding is correct it will be
reffed when assigned to the nsRefPtr ?

> >+WebVTTLoadListener::OnParsedCueWebVTTCallBack(void* aUserData, webvtt_cue* aCue)
> >+WebVTTLoadListener::OnReportErrorWebVTTCallBack(void* aUserData, uint32_t aLine,
> 
> Are we guaranteed that these callbacks will not get called after
> OnStopRequest completes (and hence that aUserData cannot be stale)?

We aren't guaranteed as OnStopRequest() just calls finish_parsing, which basically
forces the parser to finish any cues it might be in the middle of parsing. So it is possible if for these two callbacks to be called if more data is passed to the parser through OnDataAvailable() after OnStopRequest() is called. In that situation though it would probably be fine as finish_parsing doesn't destroy the parser, just tells it to clean up it's current stack of information about the cues it's in the middle of parsing.
(In reply to Boris Zbarsky (:bz) from comment #51)

> How do you know "parent" is not null?  What if mTrackElement's parent is not a media element?

HTMLTrackElement's mMediaParent is stored as an nsRefPtr<HTMLMediaElement> now:

nsRefPtr<HTMLMediaElement> mMediaParent;
> nsTArray has more code popping and pushing though.

Pushing is AppendElement.  Popping is a bit longer, I agree, in that you have to have two statements: a LastElement() and a RemoveElementAt(Length() - 1).

But you avoid doing a slew of small allocations, which sounds like a good idea to me.

> Which could result in a string like ".class-selector-1.class-selector-2" ?

Yes.  But also, why '.'?  It should be "class-selector-1 class-selector-2", no?

> This happens in the check for content and mParent just before it adds it to the DOM
> tree:

Ah, I see.  OK.

> If my understanding is correct it will be reffed when assigned to the nsRefPtr ?

That's right, but the key is that it will get _addrefed_.  So your code right now basically looks like this: add a reference, add another reference, do NOT drop the first reference.  Now you're holding only one reference, but the object thinks you're holding two, so you've leaked.  Unless I'm missing something?

> if more data is passed to the parser through OnDataAvailable() after OnStopRequest() is
> called.

That can't happen.  Necko always does OnStartRequest, then some number of OnDataAvailable, then OnStopRequest.

What I am worried about is that after OnStopRequest necko will drop references to its listener.  So after that point, the WebVTTLoadListener can be gone, so if the callbacks that take aUserData there can be called after that point, we have a problem.

> HTMLTrackElement's mMediaParent is stored as an nsRefPtr<HTMLMediaElement> now:

Yes, but the point is that it can be null if the parent is not an HTMLMediaElement!  Was that situation tested with this patch?
(In reply to Boris Zbarsky (:bz) from comment #54)
> > nsTArray has more code popping and pushing though.
> 
> Pushing is AppendElement.  Popping is a bit longer, I agree, in that you
> have to have two statements: a LastElement() and a RemoveElementAt(Length()
> - 1).
> 
> But you avoid doing a slew of small allocations, which sounds like a good
> idea to me.

Sounds fine to me. I'll change.

> > Which could result in a string like ".class-selector-1.class-selector-2" ?
> 
> Yes.  But also, why '.'?  It should be "class-selector-1 class-selector-2",
> no?

Sorry, I mistyped :). Your correct it should be ' '

> That can't happen.  Necko always does OnStartRequest, then some number of
> OnDataAvailable, then OnStopRequest.
> 
> What I am worried about is that after OnStopRequest necko will drop
> references to its listener.  So after that point, the WebVTTLoadListener can
> be gone, so if the callbacks that take aUserData there can be called after
> that point, we have a problem.

It's not possible for them to be called without more data being passed to the parser as
finish_parsing cleans up the cue stack, So there would be nothing to be returned. However, theoretically it is still possible for them to be called as nothing is resetting the function ptrs that were passed in. I'm thinking we should update libwebvtt to be able to do something like that possibly.

> > HTMLTrackElement's mMediaParent is stored as an nsRefPtr<HTMLMediaElement> now:
> 
> Yes, but the point is that it can be null if the parent is not an
> HTMLMediaElement!  Was that situation tested with this patch?

Nope, I'll put in a check for parent == null.
Chris, can you please comment on bz's question about LOAD_BYPASS_LOCAL_CACHE_IF_BUSY in comment #51? (why it's needed.)

This is cargo-culted from MediaLoadListener. I only vaguely remember why we do it there...something to do with not using the normal resource cache for media, or not wanting load to block since these files mind not fit in cache anyway.

Does it make sense to use this for text track files too? We don't want to delay 'loaded', I think, but if we're not using the media cache, maybe blocking is a better idea?
Flags: needinfo?(cpearce)
Ralph, the basic question is what happens if two loads for the same text track are racing.  Should the one that starts later wait till the first one finishes and then just read from our cache, or should it go ahead and skip our cache and hit the server if our cache is already being read from by the first load?

One important question is how common such races are and how long the wait is.  For video, the answer is "common" and "the length of the video stream" or so....
(In reply to Boris Zbarsky (:bz) from comment #51)

> Again, if the return value is ignored, why is it nsresult?
> 
> But also, this is not OK.  See bug 833385 comment 48.

Looking at this it seems like a pretty big lift. There is quite a bit of code that would have to be moved and modified to be more generic so that the LoadListener could use it as well. Is it possible to land with LoadResource() like this, since it is currently preffed off, and spin off another bug specifically for this? Alternatively, we could remove LoadResource() from BindToTree() altogether and spin off the bug as well.

I'd just like to get this hunk of code landed as we've been carrying it for so long.

I'm open to fixing it in this patch though.

> What causes this code to rerun if and when parent gets a frame in the future?  Again,    > testcases?

Currently RenderCue() is being called four times a second by HTMLMediaElement::FireTimeUpdate() so we will probably catch the frame there. It's super inefficient and not to spec currently so that is one of the next bugs we are going to be tackling.
> Looking at this it seems like a pretty big lift.

I'm not sure why.  You just want to do the load off a scriptrunner, not directly from BindToTree.  See what HTMLImageElement does.

But yes, I would be fine with landing with the LoadResource just removed from BindToTree altogether.

> Currently RenderCue() is being called four times a second by
> HTMLMediaElement::FireTimeUpdate()

Is that true when the media is paused?
(In reply to Boris Zbarsky (:bz) from comment #59)
> > Looking at this it seems like a pretty big lift.
> 
> I'm not sure why.  You just want to do the load off a scriptrunner, not
> directly from BindToTree.  See what HTMLImageElement does.
> 
> But yes, I would be fine with landing with the LoadResource just removed
> from BindToTree altogether.

Looking at the link you sent me, bug 833385 comment 48, you were talking about hooking into HTMLMediaElement's "stable state" stuff and moving it to a more common area for both the MediaElement and LoadListener. Looking at that it seems like we need to move a lot of things like:

https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#685

That and a lot of what it depends on is only defined in HTMLMediaElement. I might have misunderstood though.

Just loading off a script runner looks easy enough though.

> > Currently RenderCue() is being called four times a second by
> > HTMLMediaElement::FireTimeUpdate()
> 
> Is that true when the media is paused?

I don't know if we really care about re-rendering cues if stuff happens while being paused. I do think we should look at that though. Although, if it is alright again, I'd like to make a separate bug for that. We'd really just like to get some basic support for processing WEBVTT in this patch.
Oh, sure.  If you want to do the stable state bits that's a lot of work.  Followup is fine for that.  That's a spec correctness issue, so I'm fine with us addressing it whenever before we unpref.  Not doing it sync is a security issue, though.  Script runner is fine for that.

> Although, if it is alright again, I'd like to make a separate bug for that.

Sure.
(In reply to Rick Eyre (:reyre) from comment #60)
> (In reply to Boris Zbarsky (:bz) from comment #59)
> > > Currently RenderCue() is being called four times a second by
> > > HTMLMediaElement::FireTimeUpdate()
> > 
> > Is that true when the media is paused?
> 
> I don't know if we really care about re-rendering cues if stuff happens
> while being paused.

I think we should care about the case where the user seeks while paused, we want to display the cue from the new position after the seek. From memory we fire a timeupdate when we fire the "seeking" event in HTMLMediaElement::SeekStarted().
Flags: needinfo?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #62)

> I think we should care about the case where the user seeks while paused, we
> want to display the cue from the new position after the seek.

I have a test for this at https://people.xiph.org/~giles/2013/vtt-02.html It's broken in Chrome when the file load loses the race with the seek.

> From memory we
> fire a timeupdate when we fire the "seeking" event in
> HTMLMediaElement::SeekStarted().

Looks like that's still the case in the current code.
(In reply to Boris Zbarsky (:bz) from comment #57)

> Ralph, the basic question is what happens if two loads for the same text
> track are racing.  Should the one that starts later wait till the first one
> finishes and then just read from our cache, or should it go ahead and skip
> our cache and hit the server if our cache is already being read from by the
> first load?

Oh, I see. Subtitle files are small, and we don't do random access, so blocking sounds more appropriate. Rick, please just remove the LOAD_BYPASS_LOCAL_CACHE_IF_BUSY flag.
I think we have a bug open for that as well now: https://bugzilla.mozilla.org/show_bug.cgi?id=863488

Ralph just reminded me of that. It has his test case in there too! :)

So I think we can defer this change to that bug if everyone is alright with that.
(In reply to Ralph Giles (:rillian) from comment #56)
> Chris, can you please comment on bz's question about
> LOAD_BYPASS_LOCAL_CACHE_IF_BUSY in comment #51? (why it's needed.)

IIRC we do this because media files tend to be big, and we store them in the media cache anyway, so if they were also stored in necko's cache they'd be cached on disk twice. Since you're not sending your channel's data through the media cache, I agree with bz, I don't think you need this flag.
(In reply to Ralph Giles (:rillian) from comment #56)
> Chris, can you please comment on bz's question about
> LOAD_BYPASS_LOCAL_CACHE_IF_BUSY in comment #51? (why it's needed.)

We do this because two video elements racing for the cache meant that one played from it and the other blocked until the first released it. This resulted in the same video on different tabs having one played and one blocked.
Addressed all of bz's comments except for the possibility 'staleness' in the libwebvtt callbacks. I think we're at minimal risk for this as we're guaranteed OnDataAvailable() won't be called after OnStopRequest() which means no data will be passed to libwebvtt after finish_parsing is called. Hence, the callbacks won't called by the parser.

We probably want to make it 100% fool proof though and this may mean modifying libwebvtt in some way. I'll set up a new bug for that specifically.
Attachment #755538 - Attachment is obsolete: true
Attached patch Interdiff between v10 and v11 (obsolete) — Splinter Review
Attachment #758921 - Flags: review?(bzbarsky)
Comment on attachment 758921 [details] [diff] [review]
Interdiff between v10 and v11

>+HTMLTrackElement::LoadResource()
+    nsresult rvTwo = NewURIFromString(src, getter_AddRefs(uri));

Why is rvTwo called that?  Why not rv?

Also, if it's a failure this code will crash, as far as I can tell, by dereferencing a null "uri" lower down.  Please fix.

>+      LOG(PR_LOG_ALWAYS, ("%p Trying to load from src=%s", this,
>+      NS_ConvertUTF16toUTF8(src).get()));

Please fix the indent here.

>+    return;

Might be worth inverting the control flow here and doing the return if !GetAttr then outdenting.

>+  nsContentUtils::GetSecurityManager()->
>+    CheckLoadURIWithPrincipal(NodePrincipal(), uri,
>+                              nsIScriptSecurityManager::STANDARD);

Um.... you're supposed to check the return value and not do the load if not allowed, no?

>+  NS_ENSURE_TRUE_VOID(!rv);

No.  Not at all.  Was this tested?

I assume you want NS_ENSURE_TRUE_VOID(NS_SUCCEEDED(rv))...

There are 5 occurrences of this.

>+++ b/content/media/TextTrackCue.cpp
>+PushChildren(nsTArray<WebVTTNodeParentPair> *aNodeParentPairStack,

Why a pointer, not a reference?

>+PopChild(nsTArray<WebVTTNodeParentPair> *aNodeParentPairStack)

Likewise.

>+    (*aNodeParentPairStack)[aNodeParentPairStack->Length() - 1];

aNodeParentPairStack.LastElement()

>+  while (nodeParentPairStack.Length() > 0) {

  while (!nodeParentPairStack.IsEmpty()) {

>+  nsAutoString atomStr(atom->GetUTF16String(), atom->GetLength());
>+  mTrackElement->OwnerDoc()->CreateElem(atomStr, nullptr, kNameSpaceID_XHTML,

  mTrackElement-OwnerDoc()->CreateElem(nsDependentAtomString(atom), nullptr, ...

>+++ b/content/media/VideoUtils.h
>+static const int64_t PR_MS_PER_S = 1000;

No, my point was that we already have a "global constant for number of milliseconds in a second" and it's called PR_MSEC_PER_SEC.  That was we can change it in one place only if it ever changes.  ;)

>+++ b/content/media/WebVTTLoadListener.cpp
>     aInStream->Close();

This line should go away.  It's not your stream; you shouldn't be closing it.

What about fixing the asserts in ReadSegments that assume this never happens?

r=me with those fixed, but I want to take another look at the changes you end up making, especially because the CheckLoadURI stuff is very important to get right....
Attachment #758921 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #70)
> Comment on attachment 758921 [details] [diff] [review]

> What about fixing the asserts in ReadSegments that assume this never happens?

I think this is alright as we're now returning NS_ERROR_FAILURE right away and then the rv is checked with NS_ENSURE_SUCCESS. So that MOZ_ASSERT won't be hit as we'll return before that happens:

    nsresult rv = aStream->ReadSegments(ParseChunk, this, count, &read);
    NS_ENSURE_SUCCESS(rv, rv);
Attachment #758920 - Attachment is obsolete: true
Attached patch Interdiff between v11 and v12 (obsolete) — Splinter Review
(In reply to Boris Zbarsky (:bz) from comment #70)
> >+  NS_ENSURE_TRUE_VOID(!rv);
> 
> No.  Not at all.  Was this tested?

Was tested and did work. I did wonder why I had to ! all the values though... Don't know what I was thinking.

> >+PushChildren(nsTArray<WebVTTNodeParentPair> *aNodeParentPairStack,
> 
> Why a pointer, not a reference?

I've been in C land too long.

Everything should be addressed.
Attachment #758921 - Attachment is obsolete: true
Attachment #759774 - Flags: review?(bzbarsky)
> I think this is alright as we're now returning NS_ERROR_FAILURE right away and then the
> rv is checked with NS_ENSURE_SUCCESS. 

Have you tested this assumption?  Because that's not how I recall readSegments working and not how the documentation says it works: 

31  * Errors are never passed to the caller of ReadSegments.

> Was tested and did work.

Then the test was wrong?  NS_OK == 0, so !rv is equivalent to rv != NS_OK.  Which is a bogus test to start with, but ensuring that rv is NOT NS_OK is also wrong here.  I'd like to understand what test this was....
(In reply to Boris Zbarsky (:bz) from comment #74)
> > Was tested and did work.
> 
> Then the test was wrong?  NS_OK == 0, so !rv is equivalent to rv != NS_OK. 
> Which is a bogus test to start with, but ensuring that rv is NOT NS_OK is
> also wrong here.  I'd like to understand what test this was....

By test I meant that I just ran a webpage with a track element and made sure that the cues were displaying, nothing more serious. I tested with https://people.xiph.org/~giles/2013/vtt-02.html and cues were being displayed. 

This is confusing.. if I run it with NS_ENSURE_TRUE_VOID(rv) LoadResource() will return on the first call to the macro.

NS_ENSURE_TRUE_VOID(NS_SUCCEEDED(rv)) also displays cues properly and makes it all the way through LoadResource().
Attachment #759769 - Attachment is obsolete: true
Attached patch Interdiff between v11 and v13 (obsolete) — Splinter Review
I've diffed between v13 and v11 as I don't think you've had a chance to look at v12 yet.
Attachment #759774 - Attachment is obsolete: true
Attachment #759774 - Flags: review?(bzbarsky)
Attachment #759816 - Flags: review?(bzbarsky)
> NS_OK == 0, so !rv is equivalent to rv != NS_OK.

Er, this is backwards.  !rv is equivalent to rv == NS_OK.  Which is still wrong, but can work sometimes if your rv really is NS_OK.

In any case, NS_SUCCEEDED is the right thing here.  ;)

> I've diffed between v13 and v11 

Perfect, thanks.
Comment on attachment 759816 [details] [diff] [review]
Interdiff between v11 and v13

>+  LOG(PR_LOG_ALWAYS, ("%p Trying to load from src=%s", this,
>+    NS_ConvertUTF16toUTF8(src).get()));

Please do fix the indent.

r=me with that.  Thanks!
Attachment #759816 - Flags: review?(bzbarsky) → review+
Carrying forward r=cpearce, bz
Attachment #759815 - Attachment is obsolete: true
Carrying forward r=cpearce,bz

Just inserted case for WEBVTT_HORIZONTAL in WebVTTLoadListener so we don't get a warning. We don't actually handle this case right now as the spec has changed and a lot of that code will move when we implement the processing model: 

https://bugzilla.mozilla.org/show_bug.cgi?id=865407
Attachment #759816 - Attachment is obsolete: true
Attachment #760383 - Attachment is obsolete: true
Depends on: 881432
Blocks: 881475
(In reply to Rick Eyre (:reyre) from comment #68)
> Addressed all of bz's comments except for the possibility 'staleness' in the
> libwebvtt callbacks. I think we're at minimal risk for this as we're
> guaranteed OnDataAvailable() won't be called after OnStopRequest() which
> means no data will be passed to libwebvtt after finish_parsing is called.
> Hence, the callbacks won't called by the parser.
> 
> We probably want to make it 100% fool proof though and this may mean
> modifying libwebvtt in some way. I'll set up a new bug for that specifically.

Bz's main concern, in comment 54, was whether or not the callbacks can be called after the WebVTTLoadListener is deleted. The answer to that is no as the WebVTTLoadListener manages the libwebvtt parser so when the WebVTTLoadListener is destroyed the parser is as well. So I think we're good on this and won't be opening a bug about it, unless anyone has any more concerns?
https://hg.mozilla.org/mozilla-central/rev/7c3e3ad77c89
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Sorry about that. Ralph and I made some last minute changes to the commit messages, looks like we messed up.
You need to log in before you can comment on or make changes to this bug.