Make WebVTTParser return cue data instead of cue DOM objects
Created attachment 206685 [details] Patch
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
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
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
Comment on attachment 206685 [details] Patch r- since the patch appears to break tests.
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.
(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).
Actually, I think I made a mistake with the removeCue() method. I thought it was only valid for user-created tracks.
Created attachment 206796 [details] Set id, fix removeCue, and make ChangeLog more detailed
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..).
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?
(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.
(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`?
Created attachment 206810 [details] Return faster, use ASSERT_UNUSED, and use HashMap pointers
Comment on attachment 206810 [details] Return faster, use ASSERT_UNUSED, and use HashMap pointers Thank you!
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>
All reviewed patches have been landed. Closing bug.