Summary: | Extending WebVTT Rendering with Regions | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Victor Carbune <vcarbune> | ||||||
Component: | Media | Assignee: | Brent Fulgham <bfulgham> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bfulgham, calvaris, cdumez, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, kondapallykalyan, macpherson, menard, philipj, rniwa, sergio, silviapf | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 109570, 128412 | ||||||||
Attachments: |
|
Description
Victor Carbune
2013-02-14 05:57:01 PST
no spec, no extension; propose something in the W3C or WHATWG CG, get it written into a spec, then reopen this bug As noted in the master bug, there is a proposed solution [1] [2] which has had a great deal of discussion in the text community group mailing list (eg. [3]). Please don't close bugs without discussion. [1] https://dvcs.w3.org/hg/text-tracks/raw-file/default/608toVTT/region.html [2] http://www.w3.org/community/texttracks/wiki/MultiCueBox [3] http://lists.w3.org/Archives/Public/public-texttracks/2012Dec/0000.html This work was landed in Chromium under commit 0cdebc1d76d41bfcb9c4b022e54a4fbff132225d, and should be merged. Created attachment 226603 [details]
WIP Patch Not Tested Yet
Created attachment 226754 [details]
Patch
Comment on attachment 226754 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226754&action=review > Source/WebCore/html/shadow/MediaControlElements.cpp:1301 > + TextTrackRegion* region = cue->track()->regions()->getRegionById(regionId); Both TextTrackCue::track and TextTrack::regions() can return NULL, either of which will make this crash. > Source/WebCore/html/track/TextTrackRegion.cpp:334 > + DEPRECATED_DEFINE_STATIC_LOCAL(const AtomicString, trackRegionCueContainerScrollingClass, ("scrolling", AtomicString::ConstructFromLiteral)); Nit: you might as well go straight to NeverDestroyed<T>. > Source/WebCore/html/track/TextTrackRegion.cpp:341 > + DEPRECATED_DEFINE_STATIC_LOCAL(const AtomicString, trackRegionCueContainerPseudoId, Ditto. > Source/WebCore/html/track/TextTrackRegion.cpp:349 > + DEPRECATED_DEFINE_STATIC_LOCAL(const AtomicString, trackRegionShadowPseudoId, Ditto. > Source/WebCore/html/track/TextTrackRegion.cpp:366 > + LOG(Media, "TextTrackRegion::displayLastTextTrackCueBox"); Nit: won't this will be call really frequently? if so, you might not want to log here. > Source/WebCore/html/track/VTTCue.cpp:223 > + displayTreeInternal()->remove(ASSERT_NO_EXCEPTION); displayTreeInternal allocates the display tree if it is NULL, so you should return "if (!hasDisplayTree())" > Source/WebCore/html/track/VTTCue.cpp:747 > + TextTrackRegion* region = track()->regions()->getRegionById(m_regionId); TextTrackCue::track() and TextTrack::regions() can both return NULL Comment on attachment 226754 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226754&action=review >> Source/WebCore/html/track/TextTrackRegion.cpp:334 >> + DEPRECATED_DEFINE_STATIC_LOCAL(const AtomicString, trackRegionCueContainerScrollingClass, ("scrolling", AtomicString::ConstructFromLiteral)); > > Nit: you might as well go straight to NeverDestroyed<T>. Or maybe not, see https://bugs.webkit.org/show_bug.cgi?id=130256#c3. Comment on attachment 226754 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226754&action=review >> Source/WebCore/html/shadow/MediaControlElements.cpp:1301 >> + TextTrackRegion* region = cue->track()->regions()->getRegionById(regionId); > > Both TextTrackCue::track and TextTrack::regions() can return NULL, either of which will make this crash. Yikes! cue->track() is tested a few lines above, but I'll add code for the regions() == nullptr case. Committed r165765: <http://trac.webkit.org/changeset/165765> *** Bug 117469 has been marked as a duplicate of this bug. *** |