[webkit-reviews] review granted: [Bug 79747] Determine parameters for rendering a TextTrackCue : [Attachment 132011] Preliminary patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Victor Carbune
<vcarbune at adobe.com>'s request for review:
Bug 79747: Determine parameters for rendering a TextTrackCue
https://bugs.webkit.org/show_bug.cgi?id=79747

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

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=132011&action=review


This is close, but it needs some clean-up and tests.

> Source/WebCore/html/track/TextTrackCue.cpp:128
> +    m_displayWritingModeMap[Horizontal] = CSSValueHorizontalTb;
> +    m_displayWritingModeMap[VerticalGrowingLeft] = CSSValueVerticalLr;
> +    m_displayWritingModeMap[VerticalGrowingRight] = CSSValueVerticalRl;

Nit: it would be good to have a comment here about why these are correct
defaults.

> Source/WebCore/html/track/TextTrackCue.cpp:438
> +    int maximumSize;

This will remain uninitialized if none of the conditions below evaluate to
true. Even if this can't logically happen, the compiler will complain.

> Source/WebCore/html/track/TextTrackCue.cpp:456
> +    m_displaySize = m_cueSize < maximumSize ? m_cueSize : maximumSize;

std::min() ?

> Source/WebCore/html/track/TextTrackCue.cpp:460
> +    // 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

Nit: "size vw" and "size vh" ?

> Source/WebCore/html/track/TextTrackCue.cpp:504
> +    // 10.10 Let left be 'x-position vw' and top be 'y-position vh'.

Nit: copy pasta here as well

> Source/WebCore/html/track/TextTrackCue.h:161
> +    int m_displayWritingModeMap[3];

WritingDirection is a private enum, so I think we should add a sentinel
terminal value and use it here instead of hard coding the size of this array.