Summary: | Make track list control active | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dean Jackson <dino> | ||||||||
Component: | Media | Assignee: | Dean Jackson <dino> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric.carlson, feature-media-reviews, gyuyoung.kim, ojan, rakuco, rniwa, silviapf, silviapfeiffer1, webkit-bug-importer, webkit-ews, webkit.review.bot | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 88871, 101669 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Dean Jackson
2012-11-08 17:14:28 PST
Note that the media controls code is all in flux right now (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? 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. Created attachment 176173 [details]
Patch
Comment on attachment 176173 [details] Patch Attachment 176173 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14985984 Comment on attachment 176173 [details] Patch Attachment 176173 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14987968 Created attachment 176177 [details]
Patch
Patch 2
Comment on attachment 176177 [details] Patch Attachment 176177 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14992925 Comment on attachment 176177 [details] Patch Attachment 176177 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14983985 Created attachment 176182 [details]
Patch 3 for QT
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 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 (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? (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. Committed r135934: <http://trac.webkit.org/changeset/135934> 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 |