Bug 105536

Summary: <track> element's mode set to "disabled" after load although it was explicitly set to "hidden"
Product: WebKit Reporter: Antoine Quint <graouts>
Component: MediaAssignee: 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 Flags
WIP
none
Patch
none
Patch
none
Updated patch which should apply correctly. none

Description Antoine Quint 2012-12-20 07:37:34 PST
A lot of the Opera-submitted <track> tests added to regression test suite being skipped due to a failure related to the track's mode being set to "disabled" even though it was originally set to "hidden" in script. After debugging, it appears the change of mode is performed in HTMLMediaElement::configureTextTrackGroup() which is called when the <video> element to which the <track> element is attached is loaded. It appears the code in question relates to the concept of a "showing by default" mode which was present in an older spec draft (http://www.w3.org/TR/2011/WD-html5-20110405/video.html#text-track-showing-by-default) but is no longer available. It sounds to me that all concept of "showing by default" should be removed from the codebase, and hopefully remove this issue.

These are the affected tests that pass if the call to textTrack->setMode(TextTrack::disabledKeyword()) in HTMLMediaElement::configureTextTrackGroup() is commented out:

LayoutTests/media/track/opera/interfaces/TextTrack/addCue.html
LayoutTests/media/track/opera/interfaces/TextTrack/removeCue.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/endTime.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/startTime.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/align.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/id.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/pauseOnExit.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/track.html

These tests are also affected but have other failures beyond this specific issue with "showing by default":

LayoutTests/media/track/opera/interfaces/TextTrackCue/snapToLines.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/text.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/vertical.html
Comment 1 Radar WebKit Bug Importer 2012-12-20 07:38:27 PST
<rdar://problem/12917581>
Comment 2 Antoine Quint 2012-12-21 08:38:30 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 3 Eric Carlson 2012-12-21 10:27:53 PST
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.
Comment 4 Antoine Quint 2013-01-07 05:44:59 PST
Created attachment 181497 [details]
Patch
Comment 5 WebKit Review Bot 2013-01-07 08:31:55 PST
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
Comment 6 Antoine Quint 2013-01-07 10:16:52 PST
(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.
Comment 7 Antoine Quint 2013-01-07 10:51:09 PST
Created attachment 181532 [details]
Patch
Comment 8 Antoine Quint 2013-01-07 14:17:49 PST
Created attachment 181563 [details]
Updated patch which should apply correctly.
Comment 9 WebKit Review Bot 2013-01-08 10:51:10 PST
Comment on attachment 181563 [details]
Updated patch which should apply correctly.

Clearing flags on attachment: 181563

Committed r139080: <http://trac.webkit.org/changeset/139080>
Comment 10 WebKit Review Bot 2013-01-08 10:51:15 PST
All reviewed patches have been landed.  Closing bug.