[webkit-reviews] review denied: [Bug 79750] Display a TextTrackCue when snap-to-lines flag is not set : [Attachment 137340] Preliminary patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Victor Carbune
<vcarbune at adobe.com>'s request for review:
Bug 79750: Display a TextTrackCue when snap-to-lines flag is not set
https://bugs.webkit.org/show_bug.cgi?id=79750

Attachment 137340: Preliminary patch
https://bugs.webkit.org/attachment.cgi?id=137340&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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"?