Bug 135677 – Increase width of caption container if a larger font size is selected from user prefs

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Roger Fong]

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>

[Roger Fong]

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

[WebKit Commit Bot]

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.

[Roger Fong]

Created attachment 236155 [details]
patch

style fix

[Roger Fong]

new patch incoming with a fix for webvtt cues as well

[Brent Fulgham]

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.

[Brent Fulgham]

The current patch is a huge improvement in my test cases. Nice job!

[Roger Fong]

Created attachment 236195 [details]
patch

[Brent Fulgham]

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

[Brent Fulgham]

Comment on attachment 236195 [details]
patch

Patch looks great; can you correct the style suggestions and add the comment I asked for? Thanks.

[Roger Fong]

Created attachment 236206 [details]
patch

[Brent Fulgham]

Comment on attachment 236206 [details]
patch

r=me

[Roger Fong]

http://trac.webkit.org/changeset/172224

[Mark Lam]

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