Bug 79368 – Update TextTrackCue API

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

https://www.w3.org/Bugs/Public/show_bug.cgi?id=14929 changed TextTrackCue.alignment to .align, TextTrackCue.linePosition and .textPosition to .line
and .position, and TextTrackCue.direction to .vertical.

[Radar WebKit Bug Importer]

<rdar://problem/10918891>

[Eric Carlson]

And the possible values for direction changed as well: "horizontal" -> "","vertical" - > "rl", "vertical-lr" -> "lr"

[Eric Carlson]

Created attachment 128526 [details]
Proposed patch

[Adam Barth]

Comment on attachment 128526 [details]
Proposed patch

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

> Source/WebCore/html/track/TextTrackCue.cpp:70
> -    DEFINE_STATIC_LOCAL(const AtomicString, horizontal, ("horizontal"));
> +    DEFINE_STATIC_LOCAL(const AtomicString, horizontal, (""));
>      return horizontal;

Why not just use emptyString() ?  There's already a global static for the empty string.

[Eric Carlson]

(In reply to comment #4)
> (From update of attachment 128526 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128526&action=review
> 
> > Source/WebCore/html/track/TextTrackCue.cpp:70
> > -    DEFINE_STATIC_LOCAL(const AtomicString, horizontal, ("horizontal"));
> > +    DEFINE_STATIC_LOCAL(const AtomicString, horizontal, (""));
> >      return horizontal;
> 
> Why not just use emptyString() ?  There's already a global static for the empty string.

I initially tried to just "return emptyString()", but that doesn't work:

error: returning reference to local temporary object [-Werror,-Wreturn-stack-address]
    return emptyString();
           ^~~~~~~~~~~~~
1 error generated.

If you mean I should initialize my static with emptyString(), I can do that.

[Adam Barth]

From WTFString.cpp:

const String& emptyString()
{
    DEFINE_STATIC_LOCAL(String, emptyString, (StringImpl::empty()));
    return emptyString;
}

[Adam Barth]

Comment on attachment 128526 [details]
Proposed patch

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

> Source/WebCore/html/track/TextTrackCue.cpp:67
>  static const AtomicString& horizontalKeyword()

Ah, the problem is that you're returning an AtomicString rather than a String.  Do we have an emptyAtomicString() ?

[Eric Carlson]

Created attachment 128777 [details]
Updated patch.

[Eric Seidel (no email)]

Comment on attachment 128526 [details]
Proposed patch

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

> Source/WebCore/html/track/TextTrackCue.cpp:67
>  static const AtomicString& horizontalKeyword()

The two uses in this patch use this as a String& instead of an AtomicString&.  Doesn't really matter, but it's possible that those uses would want to be AtomicStrings themselves to get the benifit of AtomicString.

> Source/WebCore/html/track/TextTrackCue.cpp:210
>      if (value == horizontalKeyword())
>          direction = Horizontal;

So now the value must be empty to mean horizongal?  Does it ignore leading/trailing spaces?

[Eric Carlson]

Created attachment 128800 [details]
Updated patch.

[Eric Carlson]

(In reply to comment #9)
> (From update of attachment 128526 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128526&action=review
> 
> > Source/WebCore/html/track/TextTrackCue.cpp:67
> >  static const AtomicString& horizontalKeyword()
> 
> The two uses in this patch use this as a String& instead of an AtomicString&.  Doesn't really matter, but it's possible that those uses would want to be AtomicStrings themselves to get the benifit of AtomicString.
> 
Good point, I updated the static functions to all return Strings.


> > Source/WebCore/html/track/TextTrackCue.cpp:210
> >      if (value == horizontalKeyword())
> >          direction = Horizontal;
> 
> So now the value must be empty to mean horizongal?  Does it ignore leading/trailing spaces?

The spec says [1]:

On setting, the text track cue writing direction must be set to the value given in the first cell of the row in the table above whose second cell is a case-sensitive match for the new value, if any. If none of the values match, then the user agent must instead throw a SyntaxError exception. 

The second cell in the table is: "" (the empty string)

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-texttrackcue-vertical

[Eric Seidel (no email)]

Comment on attachment 128800 [details]
Updated patch.

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

I feel a bit under-informed on this effort, so I fear I'm just picking at nits.  I'm comfortable rubber-stamping this change though, I trust your awareness of the spec.

> Source/WebCore/html/track/TextTrackCue.cpp:423
> +            if (writingDirection == verticalKeywordOLD())

We've historically used deprecated() for this sort of thing, but it's not clear (to me at least) what the proper replacement function is.  Will we always have these OLD() functions around?

[Eric Carlson]

(In reply to comment #12)
> (From update of attachment 128800 [details])
> > Source/WebCore/html/track/TextTrackCue.cpp:423
> > +            if (writingDirection == verticalKeywordOLD())
> 
> We've historically used deprecated() for this sort of thing, but it's not clear (to me at least) what the proper replacement function is.  Will we always have these OLD() functions around?

No, the implementation of those functions both have this comment:

// FIXME: remove this once https://bugs.webkit.org/show_bug.cgi?id=78706 has been fixed.

Thanks for the review!

[WebKit Review Bot]

Comment on attachment 128800 [details]
Updated patch.

Clearing flags on attachment: 128800

Committed r108872: <http://trac.webkit.org/changeset/108872>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.