Bug 121935

Summary: [MediaStream] Cleanup platform interface
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: WebCore Misc.Assignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, esprehn+autocc, glenn, gyuyoung.kim, hta, jer.noble, kangil.han, rakuco, thiago.lacerda, tommyw
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 124288    
Attachments:
Description Flags
Proposed patch
none
Updated patch none

Eric Carlson
Reported 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.
Attachments
Proposed patch (113.36 KB, patch)
2013-09-26 07:53 PDT, Eric Carlson
no flags
Updated patch (113.39 KB, patch)
2013-09-26 08:09 PDT, Eric Carlson
no flags
Eric Carlson
Comment 1 2013-09-26 07:53:33 PDT
Created attachment 212708 [details] Proposed patch Patch for the bots to chew on.
Eric Carlson
Comment 2 2013-09-26 08:09:57 PDT
Created attachment 212712 [details] Updated patch Rebased.
Jer Noble
Comment 3 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.
Eric Carlson
Comment 4 2013-09-26 10:42:37 PDT
Note You need to log in before you can comment on or make changes to this bug.