Bug 79750 – Display a TextTrackCue when snap-to-lines flag is not set

• Component: Media
• Status: RESOLVED FIXED

[Victor Carbune]

Substeps 10.10 - 10.16, from section 3.5 of the WebVTT rendering rules:
http://dev.w3.org/html5/webvtt/#webvtt-cue-text-rendering-rules

"10.10 Let left be 'x-position vw' and top be 'y-position vh'. (These again are CSS values used by the next section to set CSS properties for the rendering; 'vw' and 'vh' are CSS units.) [CSSVALUES]
[...]
10.13 If cue's text track cue snap-to-lines flag is not set
[...]
10.16 Add the CSS boxes in boxes to output."

[Victor Carbune]

Created attachment 137340 [details]
Preliminary patch

[WebKit Review Bot]

Comment on attachment 137340 [details]
Preliminary patch

Attachment 137340 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12416120

New failing tests:
fast/canvas/webgl/shader-precision-format.html

[WebKit Review Bot]

Created attachment 137349 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Carlson]

Comment on attachment 137340 [details]
Preliminary patch

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

Nice test!

Marking r- for the relatively minor cleanup notes.

> Source/WebCore/html/track/TextTrackCue.cpp:414
> +int TextTrackCue::determineComputedLinePosition()

Nit: this isn't a great name, maybe something like "calculateComputedLinePosition"?

> Source/WebCore/html/track/TextTrackCue.cpp:423
> +    // If the text track cue snap-to-lines flag of the text track cue is not set

It looks like you left out part of this comment?

> Source/WebCore/html/track/TextTrackCue.cpp:435
>  void TextTrackCue::determineDisplayParameters()

Nit: maybe "calculateDisplayParameters"?

> Source/WebCore/html/track/TextTrackCue.cpp:469
> +    m_displaySize = std::min(m_cueSize, maximumSize);

Nit: Adding "using namespace std" at the beginning of the file would let you omit the "std::" here and elsewhere in the file.

> Source/WebCore/html/track/TextTrackCue.cpp:476
> +    // 10.7 If the text track cue writing direction is horizontal, then let
> +    // width be 'size vw' and height be 'auto'. Otherwise, let width be 'auto'
> +    // and height be 'size vh'. (These are CSS values used by the next section
> +    // to set CSS properties for the rendering; 'vw' and 'vh' are CSS units.)
> +    m_displayWidth = m_writingDirection == Horizontal ? m_displaySize : 0;
> +    m_displayHeight = m_writingDirection == Horizontal ? 0 : m_displaySize;

Does "0" mean "size vw" and "auto"? If so, is there any reason to not have named constants instead?

> Source/WebCore/html/track/TextTrackCue.cpp:546
> -    m_displayTree->setShadowPseudoId(AtomicString("-webkit-media-text-track-display"), ASSERT_NO_EXCEPTION);
> +    m_displayTree->setShadowPseudoId("-webkit-media-text-track-display", ASSERT_NO_EXCEPTION);

setShadowPseudoId takes a "const AtomicString&". If this is called frequently it might be worth defining and using a static const.

> Source/WebCore/html/track/TextTrackCue.cpp:568
> +        m_displayTree->setInlineStyleProperty(
> +                CSSPropertyLeft,
> +                getPositionCoordinates().first,
> +                CSSPrimitiveValue::CSS_PERCENTAGE);

I don't think breaking this into multiple lines makes it more readable.

> Source/WebCore/html/track/TextTrackCue.cpp:573
> +        m_displayTree->setInlineStyleProperty(
> +                CSSPropertyTop,
> +                getPositionCoordinates().second,
> +                CSSPrimitiveValue::CSS_PERCENTAGE);

Ditto.

> Source/WebCore/html/track/TextTrackCue.cpp:583
> +        String translateX = "-" + String::number(getPositionCoordinates().first) + "%";
> +        String translateY = "-" + String::number(getPositionCoordinates().second) + "%";

Is it necessary call getPositionCoordinates() twice?

> Source/WebCore/html/track/TextTrackCue.cpp:587
> +        m_displayTree->setInlineStyleProperty(CSSPropertyWebkitTransform,
> +                                              webkitTransformTranslateValue);

Ditto.

> Source/WebCore/html/track/TextTrackCue.cpp:619
> +    if (m_writingDirection == Horizontal && m_displayDirection == CSSValueLtr) {
> +        coordinates.first = m_textPosition;
> +        coordinates.second = m_computedLinePosition;
> +    }
> +
> +    if (m_writingDirection == Horizontal && m_displayDirection == CSSValueRtl) {
> +        coordinates.first = 100 - m_textPosition;
> +        coordinates.second = m_computedLinePosition;
> +    }
> +
> +    if (m_writingDirection == VerticalGrowingLeft) {
> +        coordinates.first = 100 - m_computedLinePosition;
> +        coordinates.second = m_textPosition;
> +    }
> +
> +    if (m_writingDirection == VerticalGrowingRight) {
> +        coordinates.first = m_computedLinePosition;
> +        coordinates.second = m_textPosition;
> +    }

These are all mutually exclusive so you should us an early returns and add an ASSERT_NOT_REACHED at the end.

> Source/WebCore/html/track/TextTrackCue.h:152
> +        TotalWritingDirections

Nit: maybe WritingDirectionCount or NumberOfWritingDirections?

> LayoutTests/ChangeLog:14
> +        * media/track/captions-webvtt/captions-snap-to-lines-unset.vtt: Added.
> +        * media/track/track-cue-rendering-snap-to-lines-unset-expected.txt: Added.
> +        * media/track/track-cue-rendering-snap-to-lines-unset.html: Added.

Nit: I think "track-cue-rendering-snap-to-lines-not-set" would be a better name.

> LayoutTests/media/media-controls.js:57
> +            throw "There are not " + i + " text track cues visible";

Do you mean: "There are not " + cueNumber + " text track cues visible"?

[Victor Carbune]

Created attachment 137406 [details]
Updated patch

[Victor Carbune]

(In reply to comment #4)
Thank you for the fast review!

> (From update of attachment 137340 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137340&action=review
> 
> Nice test!
> 
> Marking r- for the relatively minor cleanup notes.
> 
> > Source/WebCore/html/track/TextTrackCue.cpp:414
> > +int TextTrackCue::determineComputedLinePosition()
> 
> Nit: this isn't a great name, maybe something like "calculateComputedLinePosition"?
Renamed. I can't think of anything better.

> > Source/WebCore/html/track/TextTrackCue.cpp:423
> > +    // If the text track cue snap-to-lines flag of the text track cue is not set
> 
> It looks like you left out part of this comment?
Added the whole comment.

> > Source/WebCore/html/track/TextTrackCue.cpp:435
> >  void TextTrackCue::determineDisplayParameters()
> 
> Nit: maybe "calculateDisplayParameters"?
Renamed.

> > Source/WebCore/html/track/TextTrackCue.cpp:469
> > +    m_displaySize = std::min(m_cueSize, maximumSize);
> 
> Nit: Adding "using namespace std" at the beginning of the file would let you omit the "std::" here and elsewhere in the file.
I know, but I would prefer to keep it prefixed, unless you believe it breaks readability.

> > Source/WebCore/html/track/TextTrackCue.cpp:476
> > +    // 10.7 If the text track cue writing direction is horizontal, then let
> > +    // width be 'size vw' and height be 'auto'. Otherwise, let width be 'auto'
> > +    // and height be 'size vh'. (These are CSS values used by the next section
> > +    // to set CSS properties for the rendering; 'vw' and 'vh' are CSS units.)
> > +    m_displayWidth = m_writingDirection == Horizontal ? m_displaySize : 0;
> > +    m_displayHeight = m_writingDirection == Horizontal ? 0 : m_displaySize;
> 
> Does "0" mean "size vw" and "auto"? If so, is there any reason to not have named constants instead?
I added a constant named autoSize, which has the value 0. "size vw" means m_displaySize in vw css units.
  
> > Source/WebCore/html/track/TextTrackCue.cpp:546
> > -    m_displayTree->setShadowPseudoId(AtomicString("-webkit-media-text-track-display"), ASSERT_NO_EXCEPTION);
> > +    m_displayTree->setShadowPseudoId("-webkit-media-text-track-display", ASSERT_NO_EXCEPTION);
> 
> setShadowPseudoId takes a "const AtomicString&". If this is called frequently it might be worth defining and using a static const.
Done.

> > Source/WebCore/html/track/TextTrackCue.cpp:568
> > +        m_displayTree->setInlineStyleProperty(
> > +                CSSPropertyLeft,
> > +                getPositionCoordinates().first,
> > +                CSSPrimitiveValue::CSS_PERCENTAGE);
> 
> I don't think breaking this into multiple lines makes it more readable.
Left it as a single line.

> > Source/WebCore/html/track/TextTrackCue.cpp:573
> > +        m_displayTree->setInlineStyleProperty(
> > +                CSSPropertyTop,
> > +                getPositionCoordinates().second,
> > +                CSSPrimitiveValue::CSS_PERCENTAGE);
> 
> Ditto.
Same.

> > Source/WebCore/html/track/TextTrackCue.cpp:583
> > +        String translateX = "-" + String::number(getPositionCoordinates().first) + "%";
> > +        String translateY = "-" + String::number(getPositionCoordinates().second) + "%";
> 
> Is it necessary call getPositionCoordinates() twice?
I stored the position coordinates in a local variable.

> > Source/WebCore/html/track/TextTrackCue.cpp:587
> > +        m_displayTree->setInlineStyleProperty(CSSPropertyWebkitTransform,
> > +                                              webkitTransformTranslateValue);
> 
> Ditto.
Same.

> > Source/WebCore/html/track/TextTrackCue.cpp:619
> > +    if (m_writingDirection == Horizontal && m_displayDirection == CSSValueLtr) {
> > +        coordinates.first = m_textPosition;
> > +        coordinates.second = m_computedLinePosition;
> > +    }
> > +
> > +    if (m_writingDirection == Horizontal && m_displayDirection == CSSValueRtl) {
> > +        coordinates.first = 100 - m_textPosition;
> > +        coordinates.second = m_computedLinePosition;
> > +    }
> > +
> > +    if (m_writingDirection == VerticalGrowingLeft) {
> > +        coordinates.first = 100 - m_computedLinePosition;
> > +        coordinates.second = m_textPosition;
> > +    }
> > +
> > +    if (m_writingDirection == VerticalGrowingRight) {
> > +        coordinates.first = m_computedLinePosition;
> > +        coordinates.second = m_textPosition;
> > +    }
> 
> These are all mutually exclusive so you should us an early returns and add an ASSERT_NOT_REACHED at the end.
Added.

> > Source/WebCore/html/track/TextTrackCue.h:152
> > +        TotalWritingDirections
> 
> Nit: maybe WritingDirectionCount or NumberOfWritingDirections?
Renamed to NumberOfWritingDirections.

> > LayoutTests/ChangeLog:14
> > +        * media/track/captions-webvtt/captions-snap-to-lines-unset.vtt: Added.
> > +        * media/track/track-cue-rendering-snap-to-lines-unset-expected.txt: Added.
> > +        * media/track/track-cue-rendering-snap-to-lines-unset.html: Added.
> 
> Nit: I think "track-cue-rendering-snap-to-lines-not-set" would be a better name.
I agree, renamed.

> > LayoutTests/media/media-controls.js:57
> > +            throw "There are not " + i + " text track cues visible";
> 
> Do you mean: "There are not " + cueNumber + " text track cues visible"?
Fixed.

[Build Bot]

Comment on attachment 137406 [details]
Updated patch

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

[Eric Carlson]

Comment on attachment 137406 [details]
Updated patch

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

> Source/WebCore/html/track/TextTrackCue.cpp:54
> +static const AtomicString trackDisplayShadowPseudoId = "-webkit-media-text-track-display";

This can't be in the global scope, or it creates an initializer that slows down loading. It is only used in one method so you can define it there, you can use something like:

    DEFINE_STATIC_LOCAL(const AtomicString, trackDisplayShadowPseudoId, ("-webkit-media-text-track-display"));

[Eric Carlson]

Comment on attachment 137406 [details]
Updated patch

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

This looks good modulo the static initializer mentioned previously.

> Source/WebCore/html/track/TextTrackCue.cpp:443
> +    m_displayDirection = CSSValueLtr;

Nit: because this part of the algorithm comes from the WebVTT spec, it would be nice to have a URL here.

[Victor Carbune]

Created attachment 137800 [details]
Patch

Updated patch

[WebKit Review Bot]

Comment on attachment 137800 [details]
Patch

Clearing flags on attachment: 137800

Committed r114640: <http://trac.webkit.org/changeset/114640>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.