Created attachment 115608 [details] Proposed patch

Comment on attachment 115608 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=115608&action=review > Source/WebCore/platform/mediastream/MediaStreamCenter.cpp:57 > +void MediaStreamCenter::endLocalMediaStream(MediaStreamDescriptor* streamDescriptor) > +{ > + MediaStream* stream = streamDescriptor->owner(); > + if (stream) > + stream->streamEnded(); > + else > + streamDescriptor->setEnded(); > +} This function doesn't appear to have any callers. Why wouldn't the callers just do this work directly? What's the value in having a static MediaStreamCenter to help? > Source/WebCore/platform/mediastream/MediaStreamCenter.h:44 > +class MediaStreamCenter { Can you help me understand what role the MediaStreamCenter plays?

(In reply to comment #2) > (From update of attachment 115608 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115608&action=review > > > Source/WebCore/platform/mediastream/MediaStreamCenter.cpp:57 > > +void MediaStreamCenter::endLocalMediaStream(MediaStreamDescriptor* streamDescriptor) > > +{ > > + MediaStream* stream = streamDescriptor->owner(); > > + if (stream) > > + stream->streamEnded(); > > + else > > + streamDescriptor->setEnded(); > > +} > > This function doesn't appear to have any callers. Why wouldn't the callers just do this work directly? What's the value in having a static MediaStreamCenter to help? It works as a utility function, but its main purpose is layering so that the platform doesn't have to be aware of DOM objects (MediaStream). > > Source/WebCore/platform/mediastream/MediaStreamCenter.h:44 > > +class MediaStreamCenter { > > Can you help me understand what role the MediaStreamCenter plays? DOM objects: | Browser chrome: | +--------------+ +-------------------+ | +-------------------+ +----------------+ | MediaStream: | | LocalMediaStream: | | | Mute Video Button | | getUserMedia | | stream1 | | stream2 | | +-------------------+ | Selector UI | +--------------+ +-------------------+ | | (user consent) | | +----------------+ | | Requests from DOM objects and the WebKit API V +-------------------+ ==============================| MediaStreamCenter |==================================== +-------------------+ ^ | Requests from platform layer to higher layers Platform: +=================+ | Active streams: | +=================+ | stream1 | +-----------------+ | stream2 | +-----------------+ MediaStreamCenter responsibilities: * Expose media stream source probing functionality to higher layers * Notify the platform that: - A MediaStreamTrack has been enabled/disabled (from JavaScript) - A LocalMediaStream has been stopped (from JavaScript) - A local MediaStreamSource has been muted (via mute button in browser chrome) * Notify a MediaStream that the corresponding stream in the platfom has ended (e.g. USB camera was unplugged) Our platform specific implementation of MediaStreamCenter is responsible for all the plumbing of the media pipelines (e.g. from local media stream sources to player sinks and network transport sinks).

Does MediaStreamCenter keep any state? If not, perhaps it should be a collection of free functions.

Comment on attachment 115608 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=115608&action=review > Source/WebCore/platform/mediastream/MediaStreamCenter.cpp:38 > +#include "MediaStream.h" Another issue is that MediaStream.h is outside the Platform directory and Platform isn't allowed to include headers from the rest of WebCore. > Source/WebCore/platform/mediastream/MediaStreamCenter.cpp:48 > +MediaStreamCenter& MediaStreamCenter::instance() > +{ > + ASSERT(isMainThread()); > + DEFINE_STATIC_LOCAL(MediaStreamCenter, center, ()); > + return center; > +} The reason I ask these questions is that "static" is almost certainly the wrong scope for any state in WebKit.

Comment on attachment 115608 [details] Proposed patch Marking r- for now.

Created attachment 116089 [details] Updated patch (In reply to comment #4) > Does MediaStreamCenter keep any state? If not, perhaps it should be a collection of free functions. Yes, e.g., each media capture device is only represented by a single MediaStreamSource instance so the MediaStreamCenter must keep track of the MediaStreamSource instances it has created. (In reply to comment #5) > (From update of attachment 115608 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115608&action=review > > > Source/WebCore/platform/mediastream/MediaStreamCenter.cpp:38 > > +#include "MediaStream.h" > > Another issue is that MediaStream.h is outside the Platform directory and Platform isn't allowed to include headers from the rest of WebCore. Fixed (introduced MediaStreamDescriptorOwner on the platform level). Fixed similar issue with UserMediaRequest. > > Source/WebCore/platform/mediastream/MediaStreamCenter.cpp:48 > > +MediaStreamCenter& MediaStreamCenter::instance() > > +{ > > + ASSERT(isMainThread()); > > + DEFINE_STATIC_LOCAL(MediaStreamCenter, center, ()); > > + return center; > > +} > > The reason I ask these questions is that "static" is almost certainly the wrong scope for any state in WebKit. One reason it's static is to allow MediaStreamSource instances (which correspond to specific media capture devices) to out-live everything else. A MediaStreamSource can be used by several unrelated pages and muting a source takes effect in all pages.

I see, so there really is global static state in this API. That's unfortunate, but it sounds unavoidable. Thanks for answering my questions.

Comment on attachment 116089 [details] Updated patch Thank you for the review.

Comment on attachment 116089 [details] Updated patch Clearing flags on attachment: 116089 Committed r100997: <http://trac.webkit.org/changeset/100997>

All reviewed patches have been landed. Closing bug.