If we select a larger caption font size in user prefs we should scale the width percentage up so that we don't end up with really tall caption boxes. <rdar://problem/17905918>
Created attachment 236151 [details] patch I don't do the same for VTT Cues because I believe some sort of calculation is going that doesn't like it when I forcibly modify the cue box width. This patch solves the problem for 608 cues however, which we are seeing more often for example, on sites like: http://bcoveliveios-i.akamaihd.net/hls/live/207145/374678397001/KTVU_NEWSCAST/index.m3u8
Attachment 236151 [details] did not pass style-queue: ERROR: Source/WebCore/html/track/TextTrackCueGeneric.cpp:86: Missing spaces around / [whitespace/operators] [3] ERROR: Source/WebCore/html/track/TextTrackCueGeneric.cpp:89: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/html/track/TextTrackCueGeneric.cpp:91: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 3 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 236155 [details] patch style fix
new patch incoming with a fix for webvtt cues as well
Comment on attachment 236155 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=236155&action=review > Source/WebCore/html/track/TextTrackCueGeneric.cpp:45 > We should add a comment along the lines of: // This default value must be the same as the one specified in mediaControlsApple.css for -webkit-media-controls-closed-captions-container > Source/WebCore/html/track/TextTrackCueGeneric.cpp:82 > + float authorFontSize = fmax(defaultFontSize, videoSize.height() * cue->baseFontSizeRelativeToVideoHeight() / 100); I think we usually use std::max (or maybe std::max<float>). > Source/WebCore/html/track/TextTrackCueGeneric.cpp:86 > + float multiplier = fmax(1.0, m_fontSizeFromCaptionUserPrefs / authorFontSize); Ditto. > Source/WebCore/html/track/TextTrackCueGeneric.cpp:88 > + setInlineStyleProperty(CSSPropertyWidth, size*multiplier, CSSPrimitiveValue::CSS_PERCENTAGE); Need spaces around the '*' > Source/WebCore/html/track/TextTrackCueGeneric.cpp:90 > + setInlineStyleProperty(CSSPropertyHeight, size*multiplier, CSSPrimitiveValue::CSS_PERCENTAGE); Ditto > Source/WebCore/html/track/VTTCue.h:62 > + virtual void setFontSizeFromCaptionUserPrefs(int fontSize) { m_fontSizeFromCaptionUserPrefs = fontSize; } Is this meant to be overridden? This might not need to be virtual.
The current patch is a huge improvement in my test cases. Nice job!
Created attachment 236195 [details] patch
Comment on attachment 236195 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=236195&action=review > Source/WebCore/ChangeLog:14 > + Extra space isn't needed. > Source/WebCore/html/track/TextTrackCueGeneric.cpp:80 > + float authorFontSize = fmax(defaultFontSize, videoSize.height() * cue->baseFontSizeRelativeToVideoHeight() / 100); We should be using std::max or maybe std::max<float> here. defaultFontSize should be VTTCueBox::DEFAULT_FONT_SIZE. > Source/WebCore/html/track/TextTrackCueGeneric.cpp:84 > + float multiplier = fmax(1.0, m_fontSizeFromCaptionUserPrefs / authorFontSize); Ditto. > Source/WebCore/html/track/TextTrackCueGeneric.cpp:86 > + setInlineStyleProperty(CSSPropertyWidth, size*multiplier, CSSPrimitiveValue::CSS_PERCENTAGE); Missing spaces around the '*'. > Source/WebCore/html/track/TextTrackCueGeneric.cpp:88 > + setInlineStyleProperty(CSSPropertyHeight, size*multiplier, CSSPrimitiveValue::CSS_PERCENTAGE); Ditto. > Source/WebCore/html/track/VTTCue.cpp:173 > + float multiplier = fmax(1.0, m_fontSizeFromCaptionUserPrefs / defaultFontSize); std::max or std::max<float>, please. VTTCueBox::DEFAULT_FONT_SIZE > Source/WebCore/html/track/VTTCue.cpp:176 > + setInlineStyleProperty(CSSPropertyWidth, static_cast<double>(m_cue.getCSSSize()*multiplier), CSSPrimitiveValue::CSS_PERCENTAGE); Need spaces around the '*'. > Source/WebCore/html/track/VTTCue.cpp:181 > + setInlineStyleProperty(CSSPropertyHeight, static_cast<double>(m_cue.getCSSSize()*multiplier), CSSPrimitiveValue::CSS_PERCENTAGE); Ditto. > Source/WebCore/html/track/VTTCue.h:72 > + static const float defaultFontSize; I think this should be named DEFAULT_FONT_SIZE > Source/WebCore/html/track/VTTCue.h:74 > We should add a comment along the lines of: // This default value must be the same as the one specified in mediaControlsApple.css for -webkit-media-controls-closed-captions-container > Source/WebCore/html/track/VTTCue.h:75 > +const float VTTCueBox::defaultFontSize = 10; ... then this becomes VTTCueBox::DEFAULT_FONT_SIZE
Comment on attachment 236195 [details] patch Patch looks great; can you correct the style suggestions and add the comment I asked for? Thanks.
Created attachment 236206 [details] patch
Comment on attachment 236206 [details] patch r=me
http://trac.webkit.org/changeset/172224
This change caused some test failures: media/track/track-cue-rendering-horizontal.html media/track/track-cue-rendering-rtl.html The issue is tracked in <https://webkit.org/b/135720>.