Summary: | <track> element's mode set to "disabled" after load although it was explicitly set to "hidden" | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||||
Component: | Media | Assignee: | Antoine Quint <graouts> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dglazkov, eric.carlson, feature-media-reviews, gyuyoung.kim, ojan.autocc, rakuco, webkit-bug-importer, webkit.review.bot | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 103258 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Antoine Quint
2012-12-20 07:37:34 PST
Created attachment 180525 [details] WIP Attaching work-in-progress patch where we remove all concept of "showing by default". This means removing the showingByDefault() and setShowingByDefault() methods on TextTrack. As I was going through this code, I also noticed two oddities: 1. TextTrack::setIsDefault() didn't do anything and TextTrack::isDefault() always returned false. There is now a new m_isDefault ivar that actually remembers the isDefault status. 2. In HTMLMediaElement::configureTextTrackGroup(), we would set defaultTrack to textTrack.get() even though defaultTrack was created to be a RefPtr<TextTrack>. This just seemed wrong, probably an oversight. As a result, we can now pass an additional 9 tests from the Opera-submitted test suite. I'm not submitting this patch for review yet as test results depends on https://bugs.webkit.org/show_bug.cgi?id=103258 and that bug's patch has been rolled back. Comment on attachment 180525 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=180525&action=review Looks good to me, thank you! > Source/WebCore/html/HTMLMediaElement.cpp:3011 > - defaultTrack = textTrack.get(); > + defaultTrack = textTrack; Oops, good catch! > Source/WebCore/html/track/TextTrack.h:121 > + virtual bool isDefault() const { return m_isDefault; } > + virtual void setIsDefault(bool flag) { m_isDefault = flag; } "default" never applies to tracks created with addTextTrack(), so I would leave the "isDefault() const { return false; }" and remove setIsDefault() and "m_isDefault". LoadableTextTrack is the only track type that needs setIsDefault() so you can make it non-virtual. Created attachment 181497 [details]
Patch
Comment on attachment 181497 [details] Patch Attachment 181497 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15739777 New failing tests: media/video-controls-captions.html (In reply to comment #5) > (From update of attachment 181497 [details]) > Attachment 181497 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/15739777 > > New failing tests: > media/video-controls-captions.html Raised https://bugs.webkit.org/show_bug.cgi?id=106230 to track this issue. Will upload a new patch to expect failure for this test. Created attachment 181532 [details]
Patch
Created attachment 181563 [details]
Updated patch which should apply correctly.
Comment on attachment 181563 [details] Updated patch which should apply correctly. Clearing flags on attachment: 181563 Committed r139080: <http://trac.webkit.org/changeset/139080> All reviewed patches have been landed. Closing bug. |