Bug 97800 – Allow ports to override text track rendering style

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Ports should be able to modify the style used when rendering captions and subtitles.

[Radar WebKit Bug Importer]

<rdar://problem/12386612>

[Eric Carlson]

Created attachment 166067 [details]
Proposed patch

[WebKit Review Bot]

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.

[Build Bot]

Comment on attachment 166067 [details]
Proposed patch

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

[Early Warning System Bot]

Comment on attachment 166067 [details]
Proposed patch

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

[Early Warning System Bot]

Comment on attachment 166067 [details]
Proposed patch

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

[Build Bot]

Comment on attachment 166067 [details]
Proposed patch

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

[WebKit Review Bot]

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

[Eric Carlson]

Created attachment 166134 [details]
Updated patch

[Build Bot]

Comment on attachment 166134 [details]
Updated patch

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

[Eric Carlson]

(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.

[Eric Carlson]

Created attachment 166291 [details]
YAP

[Jer Noble]

LGTM. Unofficial r+.

[Anders Carlsson]

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?

[Sam Weinig]

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.

[Jer Noble]

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

[Jer Noble]

Created attachment 167190 [details]
Patch

[Silvia Pfeiffer]

Comment on attachment 167190 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167190&action=review

> Source/WebKit/mac/ChangeLog:25
> [phone]  Eric Carlson  <[e-mail]>
> +
> +        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.
> +
> [phone]  Eric Carlson  <[e-mail]>
> +
> +        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.

[Jer Noble]

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

[WebKit Review Bot]

Re-opened since this is blocked by bug 98572

[Julien Chaffraix]

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.

[Jer Noble]

Comment on attachment 167190 [details]
Patch

Moving back to review? as I don't think Sylvia is technically a reviewer.

[Jer Noble]

(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.

[Silvia Pfeiffer]

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

[Jer Noble]

Created attachment 167418 [details]
Patch

[Sam Weinig]

Comment on attachment 167418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167418&action=review

> Source/WebKit/mac/ChangeLog:24
> [phone]  Eric Carlson  <[e-mail]>
> +
> +        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.
> +
> [phone]  Eric Carlson  <[e-mail]>
> +
> +        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.

[Jer Noble]

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.

[Eric Carlson]

Created attachment 169339 [details]
Updated patch

[Silvia Pfeiffer]

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.

[Eric Carlson]

(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.

[Eric Carlson]

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.

[WebKit Review Bot]

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.

[Eric Carlson]

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.

[WebKit Review Bot]

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.

[Maciej Stachowiak]

Comment on attachment 169480 [details]
Another updated patch

Comments have been address afaict. r=me

[WebKit Review Bot]

Comment on attachment 169480 [details]
Another updated patch

Clearing flags on attachment: 169480

Committed r132349: <http://trac.webkit.org/changeset/132349>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.