Bug 114230 – [Mac] user caption styles not applied to correct element

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

User caption style overrides are not applied to same element used for the "cue" pseudo element, which makes it impossible to override styles applied by the page.

[Eric Carlson]

Created attachment 196983 [details]
Proposed patch

[Eric Carlson]

<rdar://problem/13201278>

[Early Warning System Bot]

Comment on attachment 196983 [details]
Proposed patch

Attachment 196983 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17701007

[Early Warning System Bot]

Comment on attachment 196983 [details]
Proposed patch

Attachment 196983 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17606012

[Build Bot]

Comment on attachment 196983 [details]
Proposed patch

Attachment 196983 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17592022

[Build Bot]

Comment on attachment 196983 [details]
Proposed patch

Attachment 196983 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17684004

[WebKit Review Bot]

Attachment 196983 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/track/track-css-user-override-expected.txt', u'LayoutTests/media/track/track-css-user-override.html', u'LayoutTests/media/track/track-cue-container-rendering-position-expected.txt', u'LayoutTests/media/track/track-cue-container-rendering-position.html', u'LayoutTests/media/track/track-cue-rendering-expected.txt', u'LayoutTests/media/track/track-cue-rendering.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/shadow/MediaControlElements.cpp', u'Source/WebCore/html/shadow/MediaControlElements.h', u'Source/WebCore/html/shadow/MediaControls.cpp', u'Source/WebCore/html/track/TextTrackCue.cpp', u'Source/WebCore/html/track/TextTrackCue.h', u'Source/WebCore/html/track/TextTrackCueGeneric.cpp', u'Source/WebCore/html/track/TextTrackCueGeneric.h', u'Source/WebCore/html/track/TextTrackList.cpp', u'Source/WebCore/page/CaptionUserPreferences.cpp', u'Source/WebCore/page/CaptionUserPreferences.h', u'Source/WebCore/page/CaptionUserPreferencesMac.h', u'Source/WebCore/page/CaptionUserPreferencesMac.mm', u'Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp', u'Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl']" exit_code: 1
Source/WebCore/page/CaptionUserPreferences.h:72:  The parameter name "override" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/page/CaptionUserPreferences.cpp:244:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 196991 [details]
Updated patch.

[Jer Noble]

Comment on attachment 196991 [details]
Updated patch.

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

> Source/WebCore/html/shadow/MediaControlElements.cpp:1326
> +    m_fontSize = lrintf(smallestDimension * (document()->page()->group().captionPreferences()->captionFontSizeScale(m_fontSizeIsImportant)));

This is kind of confusing, that m_fontSizeIsImportant is modified by captionFontSizeScale().  Perhaps captionFontSizeScaleAndImportance(), and breaking up this into two lines?:

float fontScale = document()->page()->group().captionPreferences()->captionFontSizeScale(m_fontSizeIsImportant);
m_fontSize = lrintf(smallestDimension * fontScale);

> Source/WebCore/html/shadow/MediaControls.cpp:415
>  void MediaControls::textTrackPreferencesChanged()
>  {
> +    closedCaptionTracksChanged();
>      if (m_textDisplayContainer)
>          m_textDisplayContainer->updateSizes(true);
> -    closedCaptionTracksChanged();
>  }

Could this be pulled into another bug? It seems unrelated.

[Eric Carlson]

https://trac.webkit.org/r148050

[Eric Carlson]

Plus https://trac.webkit.org/r148053 to fix the windows build.

[Eric Carlson]

And https://trac.webkit.org/r148058 for good measure.