Bug 114230 - [Mac] user caption styles not applied to correct element
Summary: [Mac] user caption styles not applied to correct element
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
Keywords: InRadar
Depends on:
Reported: 2013-04-08 19:41 PDT by Eric Carlson
Modified: 2013-04-09 15:50 PDT (History)
9 users (show)

See Also:

Proposed patch (47.01 KB, patch)
2013-04-08 19:46 PDT, Eric Carlson
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Updated patch. (47.10 KB, patch)
2013-04-08 21:33 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2013-04-08 19:41:56 PDT
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.
Comment 1 Eric Carlson 2013-04-08 19:46:26 PDT
Created attachment 196983 [details]
Proposed patch
Comment 2 Eric Carlson 2013-04-08 19:48:12 PDT
Comment 3 Early Warning System Bot 2013-04-08 20:04:54 PDT
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
Comment 4 Early Warning System Bot 2013-04-08 20:10:54 PDT
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
Comment 5 Build Bot 2013-04-08 20:24:13 PDT
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
Comment 6 Build Bot 2013-04-08 20:28:10 PDT
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
Comment 7 WebKit Review Bot 2013-04-08 20:48:50 PDT
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.
Comment 8 Eric Carlson 2013-04-08 21:33:53 PDT
Created attachment 196991 [details]
Updated patch.
Comment 9 Jer Noble 2013-04-09 10:52:47 PDT
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.
Comment 10 Eric Carlson 2013-04-09 14:01:12 PDT
Comment 11 Eric Carlson 2013-04-09 14:29:45 PDT
Plus https://trac.webkit.org/r148053 to fix the windows build.
Comment 12 Eric Carlson 2013-04-09 15:50:55 PDT
And https://trac.webkit.org/r148058 for good measure.