Bug 171672 - MediaSource.readyState should use an IDL enum
Summary: MediaSource.readyState should use an IDL enum
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nael Ouedraogo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-04 10:15 PDT by Nael Ouedraogo
Modified: 2017-05-09 09:00 PDT (History)
4 users (show)

See Also:


Attachments
Patch (10.25 KB, patch)
2017-05-04 23:59 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Patch (10.09 KB, patch)
2017-05-09 05:41 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nael Ouedraogo 2017-05-04 10:15:55 PDT
MediaSource.readyState should use an IDL enum as per specification (https://www.w3.org/TR/2016/CR-media-source-20160503/#idl-def-ReadyState).
Comment 1 Nael Ouedraogo 2017-05-04 23:59:09 PDT
Created attachment 309147 [details]
Patch
Comment 2 Eric Carlson 2017-05-05 08:02:32 PDT
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.
Comment 3 Chris Dumez 2017-05-05 08:39:14 PDT
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.
Comment 4 Nael Ouedraogo 2017-05-09 05:41:03 PDT
Created attachment 309490 [details]
Patch
Comment 5 Nael Ouedraogo 2017-05-09 05:50:10 PDT
I made the corrections you suggested in uploaded patch. Thanks for the reviews.
Comment 6 WebKit Commit Bot 2017-05-09 09:00:26 PDT
Comment on attachment 309490 [details]
Patch

Clearing flags on attachment: 309490

Committed r216509: <http://trac.webkit.org/changeset/216509>
Comment 7 WebKit Commit Bot 2017-05-09 09:00:27 PDT
All reviewed patches have been landed.  Closing bug.