Bug 171672 – MediaSource.readyState should use an IDL enum

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

[Nael Ouedraogo]

MediaSource.readyState should use an IDL enum as per specification (https://www.w3.org/TR/2016/CR-media-source-20160503/#idl-def-ReadyState).

[Nael Ouedraogo]

Created attachment 309147 [details]
Patch

[Eric Carlson]

Comment on attachment 309147 [details]
Patch

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

> Source/WebCore/Modules/mediasource/MediaSource.cpp:56
> +static const char* dumpReadyState(WebCore::MediaSource::ReadyState readyState)

Nit: "dumpReadyState" doesn't describe what this function does. Maybe "toString" instead?

> Source/WebCore/Modules/mediasource/MediaSource.cpp:61
> +    case WebCore::MediaSource::ReadyState::Closed: return "closed";
> +    case WebCore::MediaSource::ReadyState::Ended: return "ended";
> +    case WebCore::MediaSource::ReadyState::Open: return "open";

Nit: I think having the "return" on the same line as the "case" makes these slightly harder to read.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:528
> +    ReadyState oldState = readyState();

Nit: "auto" please.

[Chris Dumez]

Comment on attachment 309147 [details]
Patch

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

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:56
>> +static const char* dumpReadyState(WebCore::MediaSource::ReadyState readyState)
> 
> Nit: "dumpReadyState" doesn't describe what this function does. Maybe "toString" instead?

WebCore:: is not needed.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:85
> +    , m_readyState(ReadyState::Closed)

Could be inline initialization of the member in the header.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:526
> +    ASSERT(state == ReadyState::Open || state == ReadyState::Closed || state == ReadyState::Ended);

This assertion is no longer useful I believe.

> Source/WebCore/Modules/mediasource/MediaSource.h:123
> +    void setReadyState(const ReadyState);

No const please.

> Source/WebCore/Modules/mediasource/MediaSource.h:124
> +    void onReadyStateChange(const ReadyState oldState, const ReadyState newState);

No const please.

[Nael Ouedraogo]

Created attachment 309490 [details]
Patch

[Nael Ouedraogo]

I made the corrections you suggested in uploaded patch. Thanks for the reviews.

[WebKit Commit Bot]

Comment on attachment 309490 [details]
Patch

Clearing flags on attachment: 309490

Committed r216509: <http://trac.webkit.org/changeset/216509>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.