Bug 79750

Summary: Display a TextTrackCue when snap-to-lines flag is not set
Product: WebKit Reporter: Victor Carbune <vcarbune>
Component: MediaAssignee: Victor Carbune <vcarbune>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric.carlson, feature-media-reviews, macpherson, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 79747    
Bug Blocks: 79347    
Attachments:
Description Flags
Preliminary patch
none
Archive of layout-test-results from ec2-cr-linux-04
none
Updated patch
none
Patch none

Description Victor Carbune 2012-02-27 23:29:14 PST
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."
Comment 1 Victor Carbune 2012-04-16 07:07:29 PDT
Created attachment 137340 [details]
Preliminary patch
Comment 2 WebKit Review Bot 2012-04-16 08:05:45 PDT
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
Comment 3 WebKit Review Bot 2012-04-16 08:05:58 PDT
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
Comment 4 Eric Carlson 2012-04-16 08:49:02 PDT
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"?
Comment 5 Victor Carbune 2012-04-16 14:58:13 PDT
Created attachment 137406 [details]
Updated patch
Comment 6 Victor Carbune 2012-04-16 15:04:38 PDT
(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.
Comment 7 Build Bot 2012-04-16 15:27:34 PDT
Comment on attachment 137406 [details]
Updated patch

Attachment 137406 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12410879
Comment 8 Eric Carlson 2012-04-17 09:48:53 PDT
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"));
Comment 9 Eric Carlson 2012-04-17 10:07:45 PDT
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.
Comment 10 Victor Carbune 2012-04-18 16:25:47 PDT
Created attachment 137800 [details]
Patch

Updated patch
Comment 11 WebKit Review Bot 2012-04-19 09:27:42 PDT
Comment on attachment 137800 [details]
Patch

Clearing flags on attachment: 137800

Committed r114640: <http://trac.webkit.org/changeset/114640>
Comment 12 WebKit Review Bot 2012-04-19 09:27:56 PDT
All reviewed patches have been landed.  Closing bug.