[webkit-reviews] review granted: [Bug 101670] Make track list control active : [Attachment 176182] Patch 3 for QT
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Nov 27 11:55:00 PST 2012
Eric Carlson <eric.carlson at apple.com> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 101670: Make track list control active
https://bugs.webkit.org/show_bug.cgi?id=101670
Attachment 176182: Patch 3 for QT
https://bugs.webkit.org/attachment.cgi?id=176182&action=review
------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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
More information about the webkit-reviews
mailing list