Bug 121935 - [MediaStream] Cleanup platform interface
Summary: [MediaStream] Cleanup platform interface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks: 124288
  Show dependency treegraph
 
Reported: 2013-09-25 19:08 PDT by Eric Carlson
Modified: 2013-11-14 07:39 PST (History)
11 users (show)

See Also:


Attachments
Proposed patch (113.36 KB, patch)
2013-09-26 07:53 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch (113.39 KB, patch)
2013-09-26 08:09 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2013-09-25 19:08:20 PDT
The platform interface needs to be cleaned up, we don't need some left over chrome-isms, and some of the code can stand a bit of polishing.
Comment 1 Eric Carlson 2013-09-26 07:53:33 PDT
Created attachment 212708 [details]
Proposed patch

Patch for the bots to chew on.
Comment 2 Eric Carlson 2013-09-26 08:09:57 PDT
Created attachment 212712 [details]
Updated patch

Rebased.
Comment 3 Jer Noble 2013-09-26 09:20:18 PDT
Comment on attachment 212712 [details]
Updated patch

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

r=me, with nits.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:61
> +static MediaStreamCenter*& mediaStreamCenterOverride()
> +{
> +    static MediaStreamCenter* override;
> +    return override;
> +}
> +
> +MediaStreamCenter& MediaStream::sharedStreamCenter()
> +{
> +    MediaStreamCenter* override = mediaStreamCenterOverride();
> +    if (override)
> +        return *override;
> +
> +    return MediaStreamCenter::shared();
> +}
> +    
> +void MediaStream::setSharedStreamCenter(MediaStreamCenter* center)
> +{
> +    mediaStreamCenterOverride() = center;
> +}
> +

Seems like this should be in MediaStreamCenter and should override the return of MediaStreamCenter::instance().

> Source/WebCore/Modules/mediastream/MediaStream.cpp:91
> -    MediaStreamCenter::instance().didCreateMediaStream(descriptor.get());
> +    MediaStream::sharedStreamCenter().didCreateMediaStream(descriptor.get());

Then, this wouldn't be necessary.

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:182
> -    if (!MediaStreamCenter::instance().getMediaStreamTrackSources(request.release()))
> +    if (!MediaStream::sharedStreamCenter().getMediaStreamTrackSources(request.release()))

Ditto.

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:214
> -void MediaStreamTrack::didEndTrack()
> +void MediaStreamTrack::trackEnded()

Why not MediaStreamTrack::trackDidEnd()? Especially since the client method was renamed from trackEnded() to trackDidEnd().

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:217
> +    // FIXME: this is wrong, the track shouldn't have to call the descriptor's client!
> +    MediaStreamDescriptorClient* client = m_source ? m_source->stream()->client() : 0;

Bug #? :-)

> Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp:57
> -    MediaStreamCenter::instance().didCreateMediaStream(m_stream->descriptor());
> +    MediaStream::sharedStreamCenter().didCreateMediaStream(m_stream->descriptor());

One more.
Comment 4 Eric Carlson 2013-09-26 10:42:37 PDT
Committed r156473: https://trac.webkit.org/r156473