Bug 118687 – Make WebVTTParser return cue data instead of cue DOM objects

• Component: New Bugs
• Status: RESOLVED FIXED

[Brendan Long]

Make WebVTTParser return cue data instead of cue DOM objects

[Brendan Long]

Created attachment 206685 [details]
Patch

[Brendan Long]

This would be useful because the GStreamer media player can get WebVTT cues, and then use this same parser: https://bugs.webkit.org/show_bug.cgi?id=103771

[Build Bot]

Comment on attachment 206685 [details]
Patch

Attachment 206685 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1087131

New failing tests:
http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html
fullscreen/full-screen-iframe-with-max-width-height.html
media/track/opera/interfaces/TextTrackCue/id.html
media/track/track-add-remove-cue.html
media/track/track-css-matching.html
editing/pasteboard/copy-image-with-alt-text.html
http/tests/inspector/inspect-element.html

[Build Bot]

Created attachment 206750 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Ryosuke Niwa]

Comment on attachment 206685 [details]
Patch

r- since the patch appears to break tests.

[Eric Carlson]

Comment on attachment 206685 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206685&action=review

> Source/WebCore/ChangeLog:15
> +        (WebCore::TextTrackCueMap::add):
> +        (WebCore::TextTrackCueMap::find):
> +        (WebCore::TextTrackCueMap::remove):
> +        (WebCore::InbandTextTrack::removeGenericCue):
> +        (WebCore::InbandTextTrack::addWebVTTCue):

I find it much easier to review a patch when the ChangeLog has per-method comments.

[Brendan Long]

(In reply to comment #5)
> (From update of attachment 206685 [details])
> r- since the patch appears to break tests.

I missed the setId() calls. I'll upload a new version in just a minute (with per-method comments for Eric).

[Brendan Long]

Actually, I think I made a mistake with the removeCue() method. I thought it was only valid for user-created tracks.

[Brendan Long]

Created attachment 206796 [details]
Set id, fix removeCue, and make ChangeLog more detailed

[Brendan Long]

This version is a little more complicated to make the removeCue() method work correctly, since it needs to be able to look up both kinds of cues without casting (since we don't know if a cue is a TextTrackCueGeneric until we've found it in the map..).

[Eric Carlson]

Comment on attachment 206796 [details]
Set id, fix removeCue, and make ChangeLog more detailed

View in context: https://bugs.webkit.org/attachment.cgi?id=206796&action=review

> Source/WebCore/html/track/InbandTextTrack.cpp:108
> +        m_genericDataToCueMap.remove(genericData);
> +        m_genericCueToDataMap.remove(cue);

Can't you return here and avoid the call to findWebVTTData() (which will fail anyway)?

> Source/WebCore/html/track/InbandTextTrack.cpp:316
> +    UNUSED_PARAM(trackPrivate);
> +    ASSERT(trackPrivate == m_private);

I think you can use "ASSERT_UNUSED(trackPrivate, trackPrivate == m_private)" here.

> Source/WebCore/html/track/InbandTextTrack.h:69
> +    GenericCueToCueDataMap m_genericCueToDataMap;
> +    GenericCueDataToCueMap m_genericDataToCueMap;
> +    CueToWebVTTCueDataMap m_webVTTCueToDataMap;
> +    WebVTTCueDataToCueMap m_webVTTDataToCueMap;

A track will never have more than one type of cue so we will only ever use two of these. Can they be explicitly allocated?

[Brendan Long]

(In reply to comment #11)
> (From update of attachment 206796 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206796&action=review
> 
> > Source/WebCore/html/track/InbandTextTrack.cpp:108
> > +        m_genericDataToCueMap.remove(genericData);
> > +        m_genericCueToDataMap.remove(cue);
> 
> Can't you return here and avoid the call to findWebVTTData() (which will fail anyway)?

That makes sense.

> > Source/WebCore/html/track/InbandTextTrack.cpp:316
> > +    UNUSED_PARAM(trackPrivate);
> > +    ASSERT(trackPrivate == m_private);
> 
> I think you can use "ASSERT_UNUSED(trackPrivate, trackPrivate == m_private)" here.

Cool, I didn't know about that macro.

> A track will never have more than one type of cue so we will only ever use two of these. Can they be explicitly allocated?

How? I tried making it a RefPtr<HashMap<...> >, but HashMap doesn't implement RefCounted.

[Brendan Long]

(In reply to comment #12)
> > A track will never have more than one type of cue so we will only ever use two of these. Can they be explicitly allocated?
> 
> How? I tried making it a RefPtr<HashMap<...> >, but HashMap doesn't implement RefCounted.

Or do you mean explicit `new`/`delete`?

[Brendan Long]

Created attachment 206810 [details]
Return faster, use ASSERT_UNUSED, and use HashMap pointers

[Eric Carlson]

Comment on attachment 206810 [details]
Return faster, use ASSERT_UNUSED, and use HashMap pointers

Thank you!

[WebKit Commit Bot]

Comment on attachment 206810 [details]
Return faster, use ASSERT_UNUSED, and use HashMap pointers

Clearing flags on attachment: 206810

Committed r152741: <http://trac.webkit.org/changeset/152741>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.