Bug 97800

<rdar://problem/12386612>

Created attachment 166067 [details] Proposed patch

Attachment 166067 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/html/HTMLMediaElement.cpp:4193: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/rendering/RenderTheme.h:237: Missing space inside { }. [whitespace/braces] [5] Source/WebCore/rendering/RenderTheme.h:238: Missing space inside { }. [whitespace/braces] [5] Source/WebCore/html/shadow/MediaControlElements.cpp:49: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 4 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 166067 [details] Proposed patch Attachment 166067 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14031996

Comment on attachment 166067 [details] Proposed patch Attachment 166067 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14032968

Comment on attachment 166067 [details] Proposed patch Attachment 166067 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14035959

Comment on attachment 166067 [details] Proposed patch Attachment 166067 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14043674

Comment on attachment 166067 [details] Proposed patch Attachment 166067 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14059072 New failing tests: media/track/track-cue-rendering-horizontal.html media/track/track-cue-rendering-vertical.html media/track/track-cue-rendering.html

Created attachment 166134 [details] Updated patch

Comment on attachment 166134 [details] Updated patch Attachment 166134 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14043799

(In reply to comment #10) > (From update of attachment 166134 [details]) > Attachment 166134 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/14043799 This is failing because the updated WKSI libraries aren't in the patch because they pushed it well beyond the maximum size.

Created attachment 166291 [details] YAP

LGTM. Unofficial r+.

Comment on attachment 166291 [details] YAP View in context: https://bugs.webkit.org/attachment.cgi?id=166291&action=review > Source/WebCore/html/shadow/MediaControlElements.cpp:1340 > +void MediaControlTextTrackContainerElement::userCaptionPreferencesChanged() > +{ > + Page* page = document()->page(); > + if (!page) > + return; > + > + DEFINE_STATIC_LOCAL(KURL, captionsStyleSheetURL, (ParsedURLString, "user-captions-override:01F6AF12-C3B0-4F70-AF5E-A3E00234DC23")); > + > + RenderTheme* theme = page->theme(); > + HTMLMediaElement* mediaElement = toParentMediaElement(this); > + > + mediaElement->setNeedsStyleRecalc(); > + > + page->group().removeUserStyleSheetFromWorld(mainThreadNormalWorld(), captionsStyleSheetURL); > + > + if (!mediaElement->closedCaptionsVisible() && !theme->userHasCaptionPreferences()) > + return; > + > + String captionsOverrideStyleSheet = theme->captionsStyleSheetOverride(); > + if (captionsOverrideStyleSheet.isEmpty()) > + return; > + > + page->group().addUserStyleSheetToWorld(mainThreadNormalWorld(), captionsOverrideStyleSheet, captionsStyleSheetURL, adoptPtr(new Vector<String>), adoptPtr(new Vector<String>), > + InjectInAllFrames, UserStyleAuthorLevel, InjectInExistingDocuments); > +} > + Not sure about the user style sheet code here. I'd prefer if someone familiar with that code would take a look at this. > Source/WebCore/html/shadow/MediaControlElements.cpp:1415 > + float fontSize = videoBox.size().height() * (document()->page()->theme()->captionFontSizeScale()); Can page be null here?

Comment on attachment 166291 [details] YAP View in context: https://bugs.webkit.org/attachment.cgi?id=166291&action=review > Source/WebCore/html/HTMLMediaElement.cpp:4210 > + for (Node* node = firstChild(); node; node = node->nextSibling()) { It might be slightly safer to make node a RefPtr<Node> here. > Source/WebCore/html/HTMLMediaElement.cpp:4222 > + String kind = textTrack->kind(); > + > + if (kind == TextTrack::subtitlesKeyword() || kind == TextTrack::captionsKeyword()) > + trackElement->setHasBeenConfigured(false); Should these be AtomicStrings? > Source/WebCore/html/shadow/MediaControlElements.cpp:1321 > + DEFINE_STATIC_LOCAL(KURL, captionsStyleSheetURL, (ParsedURLString, "user-captions-override:01F6AF12-C3B0-4F70-AF5E-A3E00234DC23")); This would read slightly nicer at the top of the function. > Source/WebCore/rendering/RenderTheme.h:37 > > + > namespace WebCore { Unnecessary whitespace.

Since Eric is on vacation, I'll take his patch and make the suggested improvements by Anders and Sam below.

Created attachment 167190 [details] Patch

Comment on attachment 167190 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167190&action=review > Source/WebKit/mac/ChangeLog:25 > +2012-10-04 Eric Carlson <eric.carlson@apple.com> > + > + Allow ports to override text track rendering style > + https://bugs.webkit.org/show_bug.cgi?id=97800 > + <rdar://problem/12044964> > + > + Reviewed by NOBODY (OOPS!). > + > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > + > + * WebCoreSupport/WebSystemInterface.mm: > + (InitWebCoreSystemInterface): Initialize new WKSI function pointers. > + > +2012-09-27 Eric Carlson <eric.carlson@apple.com> > + > + Need a short description (OOPS!). > + Need the bug URL (OOPS!). > + > + Reviewed by NOBODY (OOPS!). > + > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > + > + * WebCoreSupport/WebSystemInterface.mm: > + (InitWebCoreSystemInterface): > + Two entries? You might want to clean up this ChangeLog. > Source/WebCore/html/HTMLMediaElement.cpp:3999 > configureTextTracks(); You might want to remove this, since markCaptionAndSubtitleTracksAsUnconfigured is already doing this as its last action.

Committed r130556: <http://trac.webkit.org/changeset/130556>

Re-opened since this is blocked by bug 98572

Comment on attachment 167190 [details] Patch Looks like this change broke several platforms (not sure why none of the bots complained): * Most Mac ones (andersca fixed them in http://trac.webkit.org/changeset/130564) * Windows, here is the warning: 26>..\..\WebCore\rendering\RenderTheme.h(239): error C2220: warning treated as error - no 'object' file generated 26>..\..\WebCore\rendering\RenderTheme.h(239): warning C4305: 'return' : truncation from 'double' to 'float' This comes from this line: virtual float captionFontSizeScale() const { return 0.05; } Mostly the patch was landed with an r+ from a non-reviewer which was the reason for rolling out instead of fixing the build.

Comment on attachment 167190 [details] Patch Moving back to review? as I don't think Sylvia is technically a reviewer.

(In reply to comment #22) > (From update of attachment 167190 [details]) > Moving back to review? as I don't think Sylvia is technically a reviewer. I(In reply to comment #21) > (From update of attachment 167190 [details]) > Looks like this change broke several platforms (not sure why none of the bots complained): > > * Most Mac ones (andersca fixed them in http://trac.webkit.org/changeset/130564) > * Windows, here is the warning: > > 26>..\..\WebCore\rendering\RenderTheme.h(239): error C2220: warning treated as error - no 'object' file generated > 26>..\..\WebCore\rendering\RenderTheme.h(239): warning C4305: 'return' : truncation from 'double' to 'float' > > This comes from this line: > > virtual float captionFontSizeScale() const { return 0.05; } > I'll fix these and reupload for review.

Oops, sorry. I was under the impression it already had an r+. No, I am not a reviewer yet.

Created attachment 167418 [details] Patch

Comment on attachment 167418 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167418&action=review > Source/WebKit/mac/ChangeLog:24 > +2012-10-05 Eric Carlson <eric.carlson@apple.com> > + > + Allow ports to override text track rendering style > + https://bugs.webkit.org/show_bug.cgi?id=97800 > + <rdar://problem/12044964> > + > + Reviewed by NOBODY (OOPS!). > + > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > + > + * WebCoreSupport/WebSystemInterface.mm: > + (InitWebCoreSystemInterface): Initialize new WKSI function pointers. > + > +2012-09-27 Eric Carlson <eric.carlson@apple.com> > + > + Need a short description (OOPS!). > + Need the bug URL (OOPS!). > + > + Reviewed by NOBODY (OOPS!). > + > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > + > + * WebCoreSupport/WebSystemInterface.mm: > + (InitWebCoreSystemInterface): Double Changelog! > Source/WebCore/html/HTMLMediaElement.cpp:557 > +#if ENABLE(VIDEO_TRACK) > + if (document()->page()) > + document()->page()->theme()->registerForCaptionPreferencesChangedCallbacks(this); > +#endif Are caption preferences only observable when in the render tree? In other words, why is attach the right place for this, rather than say, insertedIntoDocument()? > Source/WebCore/html/shadow/MediaControlElements.cpp:1315 > +void MediaControlTextTrackContainerElement::userCaptionPreferencesChanged() Does this change captions for all pages in the page group? If so, why does this need to be down in MediaControlTextTrackContainerElement. Should it be up in PageGroup instead? > Source/WebCore/html/shadow/MediaControlElements.cpp:1317 > + DEFINE_STATIC_LOCAL(KURL, captionsStyleSheetURL, (ParsedURLString, "user-captions-override:01F6AF12-C3B0-4F70-AF5E-A3E00234DC23")); I think a comment explaining the weird URL would be useful.

Comment on attachment 167418 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167418&action=review >> Source/WebKit/mac/ChangeLog:24 >> + (InitWebCoreSystemInterface): > > Double Changelog! Whoops, removed. >> Source/WebCore/html/HTMLMediaElement.cpp:557 >> +#endif > > Are caption preferences only observable when in the render tree? In other words, why is attach the right place for this, rather than say, insertedIntoDocument()? Changing preferences results in a new set of CSS style rules being applied to the captions. So yes, changes to the caption preferences only result in observable changes when the media element is in a render tree. >> Source/WebCore/html/shadow/MediaControlElements.cpp:1315 >> +void MediaControlTextTrackContainerElement::userCaptionPreferencesChanged() > > Does this change captions for all pages in the page group? If so, why does this need to be down in MediaControlTextTrackContainerElement. Should it be up in PageGroup instead? I don't know. It seems that way. But then again, that could get complicated, as the page group may media elements to register with it, so it can query whether or not the element's captions are visible, and so it can mark those media elements as setNeedsStyleRecalc(). However it does appear that there's no reason for this to be in MediaControlTextTrackContainerElement rather than HTMLMediaElement. >> Source/WebCore/html/shadow/MediaControlElements.cpp:1317 >> + DEFINE_STATIC_LOCAL(KURL, captionsStyleSheetURL, (ParsedURLString, "user-captions-override:01F6AF12-C3B0-4F70-AF5E-A3E00234DC23")); > > I think a comment explaining the weird URL would be useful. Sho nuff.

Created attachment 169339 [details] Updated patch

I still think that in Source/WebCore/html/HTMLMediaElement.cpp, > void HTMLMediaElement::setClosedCaptions .. > markCaptionAndSubtitleTracksAsUnconfigured(); > configureTextTracks(); that configureTextTracks() call is surplus because markCaptionAndSubtitleTracksAsUnconfigured() already calls that function as its last action.

(In reply to comment #29) > I still think that in Source/WebCore/html/HTMLMediaElement.cpp, > > > void HTMLMediaElement::setClosedCaptions > .. > > markCaptionAndSubtitleTracksAsUnconfigured(); > > configureTextTracks(); > > that configureTextTracks() call is surplus because markCaptionAndSubtitleTracksAsUnconfigured() already calls that function as its last action. Good point. Sorry, I overlooked your earlier comment.

Created attachment 169469 [details] Updated patch Updated to fix the problem Silvia noted. Also includes the changes Sam suggested: the PageGroup now listens for notifications of user preference changes and updates the CSS override, and a media element registers with the PageGroup for preference notifications so it can re-evaluate which tracks should be enabled.

Attachment 169469 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1 Source/WebCore/page/CaptionUserPreferences.h:26: #ifndef header guard has wrong style, please use: CaptionUserPreferences_h [build/header_guard] [5] Source/WebCore/page/PageGroup.h:40: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WebCore/page/CaptionUserPreferencesMac.h:32: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/page/CaptionUserPreferencesMac.h:53: The parameter name "group" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 169480 [details] Another updated patch Fixed the style-nits in my new code. This will fail the style-bot because the indentation in PageGroup.h is still wrong, but I just used the same indentation the file already has.

Attachment 169480 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1 Source/WebCore/page/PageGroup.h:40: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 169480 [details] Another updated patch Comments have been address afaict. r=me

Comment on attachment 169480 [details] Another updated patch Clearing flags on attachment: 169480 Committed r132349: <http://trac.webkit.org/changeset/132349>

All reviewed patches have been landed. Closing bug.