Bug 101670 - Make track list control active
Summary: Make track list control active
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on: 88871 101669
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-08 17:14 PST by Dean Jackson
Modified: 2012-12-20 14:54 PST (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2012-11-08 17:14:28 PST
Hook selection in the media control track list (for captions and subtitles) up to the HTMLMediaElement.
Comment 1 Radar WebKit Bug Importer 2012-11-08 17:14:53 PST
<rdar://problem/12667877>
Comment 2 Silvia Pfeiffer 2012-11-09 18:04:43 PST
Note that the media controls code is all in flux right now
Comment 3 Dean Jackson 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?
Comment 4 Silvia Pfeiffer 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.
Comment 5 Dean Jackson 2012-11-26 22:12:59 PST
Created attachment 176173 [details]
Patch
Comment 6 Early Warning System Bot 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
Comment 7 Early Warning System Bot 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
Comment 8 Dean Jackson 2012-11-26 22:36:54 PST
Created attachment 176177 [details]
Patch

Patch 2
Comment 9 Early Warning System Bot 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
Comment 10 Early Warning System Bot 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
Comment 11 Dean Jackson 2012-11-26 22:53:07 PST
Created attachment 176182 [details]
Patch 3 for QT
Comment 12 Silvia Pfeiffer 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)
Comment 13 Eric Carlson 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
Comment 14 Silvia Pfeiffer 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?
Comment 15 Dean Jackson 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.
Comment 16 Dean Jackson 2012-11-27 15:59:54 PST
Committed r135934: <http://trac.webkit.org/changeset/135934>