Bug 157253 – Stop using string-based enumerations in TextTrack, and eliminate support for string-based enumerations

• Component: Bindings
• Status: RESOLVED FIXED

[Darin Adler]

Stop using string-based enumerations in TextTrack, and eliminate support for string-based enumerations

[Darin Adler]

Created attachment 277890 [details]
Patch

[Chris Dumez]

Comment on attachment 277890 [details]
Patch

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

r=me with comments

> Source/WebCore/html/HTMLTrackElement.cpp:124
>      setAttribute(kindAttr, kind);

setAttributeWithoutSynchronization()

> Source/WebCore/html/HTMLTrackElement.h:44
> +    const AtomicString& srclang() const;

Do we still need this one now that the attribute has been marked as [Reflect]? Is this called from non-bindings code?

> Source/WebCore/html/HTMLTrackElement.h:45
> +    const AtomicString& label() const;

ditto.

> Source/WebCore/html/track/TextTrack.cpp:462
> +    return (m_kind == TextTrackKind::Captions || m_kind == TextTrackKind::Subtitles || m_kind != TextTrackKind::Forced)

I think this should be m_kind == TextTrackKind::Forced

> Source/WebCore/html/track/TextTrack.h:198
> +#if !ENABLE(MEDIA_SOURCE)

I would have done the #ifdefing inside the function instead of duplicating it.

[Build Bot]

Comment on attachment 277890 [details]
Patch

Attachment 277890 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1253882

New failing tests:
imported/w3c/web-platform-tests/html/dom/reflection-embedded.html

[Build Bot]

Created attachment 277891 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Darin Adler]

Comment on attachment 277890 [details]
Patch

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

>> Source/WebCore/html/HTMLTrackElement.cpp:124
>>      setAttribute(kindAttr, kind);
> 
> setAttributeWithoutSynchronization()

Makes sense. I wish fastGetAttribute had a name that was more similar since both are basically the same concept!

>> Source/WebCore/html/HTMLTrackElement.h:44
>> +    const AtomicString& srclang() const;
> 
> Do we still need this one now that the attribute has been marked as [Reflect]? Is this called from non-bindings code?

Yes, it’s called in a couple places. Could change those to call fastGetAttribute directly but I thought I would leave it like this for now.

>> Source/WebCore/html/HTMLTrackElement.h:45
>> +    const AtomicString& label() const;
> 
> ditto.

Same here.

>> Source/WebCore/html/track/TextTrack.cpp:462
>> +    return (m_kind == TextTrackKind::Captions || m_kind == TextTrackKind::Subtitles || m_kind != TextTrackKind::Forced)
> 
> I think this should be m_kind == TextTrackKind::Forced

Oops, yes. I bet that’s why a test is failing. Fixing it.

>> Source/WebCore/html/track/TextTrack.h:198
>> +#if !ENABLE(MEDIA_SOURCE)
> 
> I would have done the #ifdefing inside the function instead of duplicating it.

I duplicated it to avoid UNUSED_PARAM. What do you think?

[Darin Adler]

Committed r200317: <http://trac.webkit.org/changeset/200317>