[webkit-reviews] review granted: [Bug 190004] WebVTT cue alignment broken : [Attachment 350876] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Per Arne Vollan
<pvollan at apple.com>'s request for review:
Bug 190004: WebVTT cue alignment broken
https://bugs.webkit.org/show_bug.cgi?id=190004

Attachment 350876: Patch

https://bugs.webkit.org/attachment.cgi?id=350876&action=review




--- Comment #3 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 350876
  --> https://bugs.webkit.org/attachment.cgi?id=350876
Patch

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

> Source/WebCore/html/track/TextTrackCueGeneric.cpp:173
> +ExceptionOr<void> TextTrackCueGeneric::setPosition(const Variant<double,
AutoKeyword>& position)

Nit: "Variant<double, AutoKeyword>" is used often enough that it would be good
to define a type for it:  

using LineAndPositionSetting = Variant<double, AutoKeyword>;

> Source/WebCore/html/track/VTTCue.cpp:401
> +    Variant<double, AutoKeyword> pos;
> +    if (textPositionIsAuto())
> +	   pos = Auto;
> +    else
> +	   pos = m_textPosition;
> +    return pos;

Nit: this can be reduced to something like:

return textPositionIsAuto() ? Auto : m_textPosition;

> Source/WebCore/html/track/VTTCue.cpp:412
> +    if (position.index() == 1) {

I think WTF::holds_alternative<> would be cleaner: 

WTF::holds_alternative<AutoKeyword>(position)