Same as bug 101669 but for Chromium.
Created attachment 190478 [details] WIP patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 190478 [details] WIP patch Attachment 190478 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16807132
Comment on attachment 190478 [details] WIP patch Attachment 190478 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16663255
Comment on attachment 190478 [details] WIP patch Attachment 190478 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16783181 New failing tests: media/video-controls-captions.html
Comment on attachment 190478 [details] WIP patch Attachment 190478 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16663320 New failing tests: media/video-controls-captions.html
Comment on attachment 190478 [details] WIP patch Attachment 190478 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16803090 New failing tests: media/track/track-user-preferences.html
Comment on attachment 190478 [details] WIP patch Attachment 190478 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/16791114 New failing tests: media/track/track-user-preferences.html
Comment on attachment 190478 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=190478&action=review > Source/WebCore/ChangeLog:18 > + * css/CSSDefaultStyleSheets.cpp: > + (WebCore::CSSDefaultStyleSheets::ensureDefaultStyleSheetsForElement): > + Allow mediaControlsStyleSheet to also style by siblingRules. A longer comment explaining why this is necessary would be nice. > Source/WebCore/html/shadow/MediaControlElements.cpp:868 > + if (!(track->kind() == track->captionsKeyword() || track->kind() == track->subtitlesKeyword())) > + continue; Nit: this is slightly awkward, why not "if (!captions && !subtitles)"? > Source/WebCore/html/shadow/MediaControlElementsChromium.cpp:49 > +static const char* textTracksOffAttrValue = "-1"; // This must match HTMLMediaElement::textTracksOffIndex() Why not just call HTMLMediaElement::textTracksOffIndex() (besides the fact that the old code did it ;-) ) > Source/WebCore/html/shadow/MediaControlElementsChromium.cpp:96 > + // Create first list item to turn captions/subtitles off. This comment just tells us what the code does, which is obvious by inspection. > Source/WebCore/html/shadow/MediaControlElementsChromium.cpp:109 > + if (!(track->kind() == track->captionsKeyword() || track->kind() == track->subtitlesKeyword())) Nit: this is slightly odd, why not "if (!captions && !subtitles)"? > Source/WebCore/html/shadow/MediaControlElementsChromium.cpp:142 > + // Use the language name as the display name for the track, if available. > + String labelText = displayNameForLanguageLocale(track->language()); > + > + // If the developer provided a label for the track, add it to display name. > + if (!(track->label().isNull() || track->label().isEmpty())) { > + if (labelText.isNull() || labelText.isEmpty()) > + labelText = track->label(); > + else { > + labelText.append(" - "); > + labelText.append(track->label()); > + } > + } > + > + // If track still has no display name, add "No Label". > + if (labelText.isNull() || labelText.isEmpty()) > + labelText = textTrackNoLabelText(); > + > + // Mark actual caption tracks with a special keyword. > + if (track->kind() == track->captionsKeyword()) { > + labelText.append(" ["); > + labelText.append(textTrackCaptionsMarker()); > + labelText.append("]"); > + } Couldn't you get rid of this entire function if you put the name generation logic into an override of CaptionsUserPreferences::displayNameForTrack?
Eric, thanks. That's all great feedback - will apply. >> Source/WebCore/ChangeLog:18 >> + * css/CSSDefaultStyleSheets.cpp: >> + (WebCore::CSSDefaultStyleSheets::ensureDefaultStyleSheetsForElement): >> + Allow mediaControlsStyleSheet to also style by siblingRules. > >A longer comment explaining why this is necessary would be nice. This is to allow me to use video::-webkit-media-controls-closed-captions-track-list li:first-child in the mediaControlsChromium.css style sheet. I could instead move this into a layout() function, though I thought this was easier to read. What do you think? Another architecture question: What do you think about the new paintMediaClosedCaptionsContainer() function? Would that be better in a MediaClosedCaptionsContainer::layout() function and overloaded for Chromium? (That whole painting pipeline with adjustxxx, layout(), paintxxx(), formatxxx functions still confuses me.)
(In reply to comment #10) > Eric, thanks. That's all great feedback - will apply. > > >> Source/WebCore/ChangeLog:18 > >> + * css/CSSDefaultStyleSheets.cpp: > >> + (WebCore::CSSDefaultStyleSheets::ensureDefaultStyleSheetsForElement): > >> + Allow mediaControlsStyleSheet to also style by siblingRules. > > > >A longer comment explaining why this is necessary would be nice. > > This is to allow me to use > video::-webkit-media-controls-closed-captions-track-list li:first-child > in the mediaControlsChromium.css style sheet. > I could instead move this into a layout() function, though I thought this was easier to read. > What do you think? > I think this is fine, I was actually just requesting a longer explanation in the ChangeLog. > > Another architecture question: What do you think about the new paintMediaClosedCaptionsContainer() function? Would that be better in a MediaClosedCaptionsContainer::layout() function and overloaded for Chromium? (That whole painting pipeline with adjustxxx, layout(), paintxxx(), formatxxx functions still confuses me.) It stills confuses me too, so you would probably be better off asking someone that understands renderers+theme better.
(In reply to comment #11) > (In reply to comment #10) > > Another architecture question: What do you think about the new paintMediaClosedCaptionsContainer() function? Would that be better in a MediaClosedCaptionsContainer::layout() function and overloaded for Chromium? (That whole painting pipeline with adjustxxx, layout(), paintxxx(), formatxxx functions still confuses me.) > > It stills confuses me too, so you would probably be better off asking someone that understands renderers+theme better. Can somebody with renderer experience provide some advice? I've got a caption menu that is displayed when the CC button on the video controls is clicked. It needs custom styling just before it's being shown: * adapt it's right offset to line up with the video controls right edge * adapt the controls top right corner to not be rounded but align with the menu I currently did this in a paintXXX function as part of paintMediaControlsPart, but this updates after painting, so we see an actual position change of the menu. I'd prefer using the adjustMediaControlStyle() function that Blackberry uses for this kind of stuff. However, we haven't used this in Chromium yet and I wonder if there is a reason.