WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 101670
Make track list control active
https://bugs.webkit.org/show_bug.cgi?id=101670
Summary
Make track list control active
Dean Jackson
Reported
2012-11-08 17:14:28 PST
Hook selection in the media control track list (for captions and subtitles) up to the HTMLMediaElement.
Attachments
Patch
(32.14 KB, patch)
2012-11-26 22:12 PST
,
Dean Jackson
no flags
Details
Formatted Diff
Diff
Patch
(32.32 KB, patch)
2012-11-26 22:36 PST
,
Dean Jackson
no flags
Details
Formatted Diff
Diff
Patch 3 for QT
(32.38 KB, patch)
2012-11-26 22:53 PST
,
Dean Jackson
eric.carlson
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2012-11-08 17:14:53 PST
<
rdar://problem/12667877
>
Silvia Pfeiffer
Comment 2
2012-11-09 18:04:43 PST
Note that the media controls code is all in flux right now
Dean Jackson
Comment 3
2012-11-10 21:24:08 PST
(In reply to
comment #2
)
> Note that the media controls code is all in flux right now
Yes, when will you be landing the refactoring?
Silvia Pfeiffer
Comment 4
2012-11-11 04:26:37 PST
If all goes well, the first patch should land tomorrow. I still have to prepare the second one. Don't want to hold you up though, just thought to make you aware.
Dean Jackson
Comment 5
2012-11-26 22:12:59 PST
Created
attachment 176173
[details]
Patch
Early Warning System Bot
Comment 6
2012-11-26 22:25:35 PST
Comment on
attachment 176173
[details]
Patch
Attachment 176173
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/14985984
Early Warning System Bot
Comment 7
2012-11-26 22:29:23 PST
Comment on
attachment 176173
[details]
Patch
Attachment 176173
[details]
did not pass qt-wk2-ews (qt): Output:
http://queues.webkit.org/results/14987968
Dean Jackson
Comment 8
2012-11-26 22:36:54 PST
Created
attachment 176177
[details]
Patch Patch 2
Early Warning System Bot
Comment 9
2012-11-26 22:43:45 PST
Comment on
attachment 176177
[details]
Patch
Attachment 176177
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/14992925
Early Warning System Bot
Comment 10
2012-11-26 22:44:06 PST
Comment on
attachment 176177
[details]
Patch
Attachment 176177
[details]
did not pass qt-wk2-ews (qt): Output:
http://queues.webkit.org/results/14983985
Dean Jackson
Comment 11
2012-11-26 22:53:07 PST
Created
attachment 176182
[details]
Patch 3 for QT
Silvia Pfeiffer
Comment 12
2012-11-26 23:00:13 PST
FYI:
Bug 101877
will make it possible to give media elements different functionality depending on platform. I have a patch to look at almost ready. (but don't wait to land this)
Eric Carlson
Comment 13
2012-11-27 11:57:13 PST
Comment on
attachment 176182
[details]
Patch 3 for QT View in context:
https://bugs.webkit.org/attachment.cgi?id=176182&action=review
Marking r+ but I would please consider the suggested changes.
> Source/WebCore/ChangeLog:14 > + (WebCore):
Nit: you can remove this worthless entry added by prepare-ChangeLog.
> Source/WebCore/ChangeLog:19 > + (WebCore):
Ditto.
> Source/WebCore/ChangeLog:39 > + (MediaControlsApple):
Ditto.
> Source/WebCore/html/HTMLMediaElement.cpp:161 > +static const int cTextTracksOff = -1;
Nit: I don't think there is an explicit style rule about using "c" prefix on constants, and none of the others in this file do so I would prefer this didn't either.
> Source/WebCore/html/shadow/MediaControlElements.cpp:85 > +static const char* cTextTracksOffAttrValue = "-1"; // This must match cTextTracksOff in HTMLMediaElement. > +static const int cTextTracksIndexOff = -1; // This must match cTextTracksOff in HTMLMediaElement.
It would be better to define and use a static function in HTMLMediaElement (like MediaPlayer::invalidTime()).
> Source/WebCore/html/shadow/MediaControlElements.cpp:87 > +static const int cTextTracksIndexNotFound = -2; > +static const char* cTextTrackSelectedClassValue = "selected";
No "c" prefixes please.
> Source/WebCore/html/shadow/MediaControlElements.cpp:115 > + const AtomicString trackIndexAttributeValue = element->getAttribute(trackIndexAttr);
getAttribute() takes a const AtomicString&, so it would be more efficient to create one once instead of every time it is called (eg. as we do in MediaControlTimelineContainerElement::shadowPseudoId).
> Source/WebCore/html/shadow/MediaControlElements.cpp:929 > setDisplayType(mediaController()->closedCaptionsVisible() ? MediaHideClosedCaptionsButton : MediaShowClosedCaptionsButton); > + setChecked(mediaController()->closedCaptionsVisible());
Nit: it would be more efficient to put mediaController()->closedCaptionsVisible() into a local.
> Source/WebCore/html/shadow/MediaControlElements.cpp:937 > + // FIXME: It's not great that the shared code is dictating behavior of platform-specific > + // UI. Not all ports may want the closed captions button to toggle a list of tracks, so > + // we have to use #if.
Maybe mention
https://bugs.webkit.org/show_bug.cgi?id=101877
?
> Source/WebCore/html/shadow/MediaControlElements.cpp:1030 > + if (trackIndex != cTextTracksIndexNotFound) {
Nit: "if (trackIndex == cTextTracksIndexNotFound) continue" would make this slightly easier to read.
> Source/WebCore/html/shadow/MediaControlElements.cpp:1032 > + if (mediaElement->closedCaptionsVisible())
Nit: mediaElement->closedCaptionsVisible() won't change during the loop so you can cache it in a local.
> LayoutTests/media/video-controls-captions-trackmenu.html:96 > + consoleWrite("Track 2 should be disabled"); > + track = tracks[2]; > + testExpected("track.mode", "disabled");
Minor nit: I think the results would be easier to read if you didn't use the "track" and "tracks" variables. Instead of Track 2 should be disabled EXPECTED (track.mode == 'disabled') OK it would read: Track 2 should be disabled EXPECTED (video.textTracks[2].mode == 'disabled') OK
Silvia Pfeiffer
Comment 14
2012-11-27 12:45:47 PST
(In reply to
comment #13
)
> > Source/WebCore/html/HTMLMediaElement.cpp:161 > > +static const int cTextTracksOff = -1; >
Would it make sense to define this as a class constant in HTMLMediaElement and then use it in MediaControlElement as HTMLMediaElement::constname , including a cast to string where you need it as a string?
Dean Jackson
Comment 15
2012-11-27 15:11:05 PST
(In reply to
comment #14
)
> (In reply to
comment #13
) > > > Source/WebCore/html/HTMLMediaElement.cpp:161 > > > +static const int cTextTracksOff = -1; > > > > Would it make sense to define this as a class constant in HTMLMediaElement and then use it in MediaControlElement as HTMLMediaElement::constname , including a cast to string where you need it as a string?
That's basically what I've done (when landing). Thanks.
Dean Jackson
Comment 16
2012-11-27 15:59:54 PST
Committed
r135934
: <
http://trac.webkit.org/changeset/135934
>
Ryosuke Niwa
Comment 17
2012-12-20 14:54:58 PST
The test added by this patch is flaky:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=media%2Fvideo-controls-captions-trackmenu.html
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug