Bug 73130

Created attachment 116611 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

Created attachment 116724 [details] Patch

Comment on attachment 116724 [details] Patch Attachment 116724 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10643443

Created attachment 116739 [details] Patch

Comment on attachment 116739 [details] Patch Looks reasonable to me, but we need fishd to review.

Comment on attachment 116739 [details] Patch Attachment 116739 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10640672

Created attachment 117189 [details] Patch

Comment on attachment 117189 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117189&action=review > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:339 > + virtual WebMediaStreamCenter* mediaStreamCenter(WebMediaStreamCenterClient*) { return 0; } you are passing a Client to this function, which suggests that this is a factory method. factory methods should have the 'create' prefix, so it is clear to the caller that they are getting an instance object that they will need to remember to delete. > Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:44 > + virtual void didSetMediaStreamTrackEnabled(const WebMediaStreamDescriptor&, size_t trackIndex) = 0; I'm having a hard time imagining how didSetMediaStreamTrackEnabled, didStopLocalMediaStream and WebMediaStreamCenterClient::endLocalMediaStream relate to one another. It is not clear what these functions mean either. > Source/WebKit/chromium/public/platform/WebMediaStreamDescriptor.h:55 > + WebMediaStreamDescriptor(WebCore::MediaStreamDescriptor*); usually the PassRefPtr variant is enough. do we really need this one too? (same question applies to the conversion operators below) > Source/WebKit/chromium/public/platform/WebMediaStreamSourcesQueryClient.h:46 > +class WebMediaStreamSourcesQueryClient { "Client" is usually used when naming interfaces. This class appears to be more like WebUserMediaRequest. It provides readonly properties that customize the request and it has the method the embedder should call to complete the request. I don't think *Client is the right name for this class. > Source/WebKit/chromium/public/platform/WebMediaStreamSourcesQueryClient.h:57 > + WEBKIT_EXPORT void mediaStreamSourcesQueryCompleted(const WebVector<WebMediaStreamSource>&); can the query fail? WebUserMediaRequest has methods for success and failure. we don't need that here?

MediaStreamCenter is a catch-all interface for MediaStream related objects that just have one or two functions that needs to be routed to the embedder (or the other direction). Instead of creating a whole array of *Handlers etc they are grouped together. WebMediaStreamCenter/MediaStreamCenter are singleton objects since there shouldn't respective can't be more than one per process. * WebMediaStreamCenter::queryMediaStreamSources This is the first part of the getUserMedia() stream creation. The request to enumerate all video and audio devices first goes out through the platform to the embedder. If no devices are found or if the embedder chooses to do the scanning later an empty list is returned through the WebMediaStreamSourcesQueryClient, otherwise a populated list is sent back. The UserMediaRequest is then send out through the page client for user approval and specific device selection. * WebMediaStreamCenter::didSetMediaStreamTrackEnabled MediaStreamTrack.enabled was changed and the embedder should pause/resume the stream. Instead of sending just the track down to the embedder the entire MediaStream representation is sent for convenience. * WebMediaStreamCenter::didStopLocalMediaStream LocalMediaStream::stop() was called and the embedder should close the camera(s) and/or microphone(s). * WebMediaStreamCenterClient::endLocalMediaStream This is called from the embedder up to to WebKit when a LocalMediaStream has ended.

Comment on attachment 117189 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117189&action=review >> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:339 >> + virtual WebMediaStreamCenter* mediaStreamCenter(WebMediaStreamCenterClient*) { return 0; } > > you are passing a Client to this function, which suggests that this is a factory method. > factory methods should have the 'create' prefix, so it is clear to the caller that they > are getting an instance object that they will need to remember to delete. This should return an singleton-like object and thus I didn't add the create prefix. >> Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:44 >> + virtual void didSetMediaStreamTrackEnabled(const WebMediaStreamDescriptor&, size_t trackIndex) = 0; > > I'm having a hard time imagining how didSetMediaStreamTrackEnabled, didStopLocalMediaStream and > WebMediaStreamCenterClient::endLocalMediaStream relate to one another. It is not clear what these > functions mean either. See my comment on the patch. >> Source/WebKit/chromium/public/platform/WebMediaStreamDescriptor.h:55 >> + WebMediaStreamDescriptor(WebCore::MediaStreamDescriptor*); > > usually the PassRefPtr variant is enough. do we really need this one too? > > (same question applies to the conversion operators below) Sort of yes, for convenience. Without the MediaStreamDescriptor* constructor it isn't possible to go from a MediaStreamDescriptor* to a WebMediaStreamDesctructor without manually introducing temporary variables. void MediaStreamCenterInternal::didSetMediaStreamTrackEnabled(Mediastreamdescriptor* stream, size_t trackIndex) { if (m_private) m_private->didSetMediaStreamTrackEnabled(stream, trackIndex); } The PassRefPtr constructor is needed for the automatic array conversion from WebCore::Vector to WebKit::WebVector Similarly the conversion operators are to avoid temporary variables.

Created attachment 118036 [details] Patch rebase

Comment on attachment 118036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118036&action=review > Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:41 > + virtual ~WebMediaStreamCenter() { } It looks like you intend the user of this interface to delete the object through this interface. If so, then WebKitPlatformSupport::mediaStreamCenter() is mis-named. It should be prefixed with "create". Otherwise, this interface should have its destructor hidden (made protected). > Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:44 > + virtual void didSetMediaStreamTrackEnabled(const WebMediaStreamDescriptor&, size_t trackIndex) = 0; nit: perhaps this function could just be named didEnableMediaStreamTrack? > Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:45 > + virtual void didStopLocalMediaStream(const WebMediaStreamDescriptor&) = 0; nit: this function says "Local" in its name, but the parameter type does not have "Local" in its name. perhaps this function should just be called didStopMediaStream? > Source/WebKit/chromium/public/platform/WebMediaStreamCenterClient.h:42 > + virtual void endLocalMediaStream(const WebMediaStreamDescriptor&) = 0; nit: here you use the term "end" but on WebMediaStreamCenter, I see the term "stop". Is there a relationship between ending a stream and it being reported as stopped? Should we try to use the same term here if they are related? > Source/WebKit/chromium/public/platform/WebMediaStreamDescriptor.h:55 > + WebMediaStreamDescriptor(WebCore::MediaStreamDescriptor*); nit: are you sure you need both constructors? > Source/WebKit/chromium/public/platform/WebMediaStreamSourcesQueryClient.h:57 > + WEBKIT_EXPORT void mediaStreamSourcesQueryCompleted(const WebVector<WebMediaStreamSource>&); nit: It seems redundant to include the class name in this method. This method is already scoped to a class that has in its name "media stream sources query", so why repeat that here? it seems like you could just call this method didComplete. WebUserMediaRequest is fairly similar to this interface. In that case, we used the term "request" in the method name, which was slightly redundant but seemed to work nicely. So, following that we could call this method here queryCompleted or didCompleteQuery. I'm also curious about the name of this class. Is it really a Client? Or, is it perhaps more of a Request interface along the lines of WebUserMediaRequest? Should we call this WebMediaStreamSourcesRequest? Is the WebCore version similarly mis-named? it would be nice to be consistent with how things are named.

Comment on attachment 118036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118036&action=review >> Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:41 >> + virtual ~WebMediaStreamCenter() { } > > It looks like you intend the user of this interface to delete the object through this interface. > If so, then WebKitPlatformSupport::mediaStreamCenter() is mis-named. It should be prefixed with > "create". Otherwise, this interface should have its destructor hidden (made protected). Moved to protected. >> Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:44 >> + virtual void didSetMediaStreamTrackEnabled(const WebMediaStreamDescriptor&, size_t trackIndex) = 0; > > nit: perhaps this function could just be named didEnableMediaStreamTrack? The method on MediaStreamTrack is called setEnabled() and the name didSetMediaStreamTrackEnabled reflects that. Maybe didSetEnabledOnMediaStream would be longer but clearer? >> Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:45 >> + virtual void didStopLocalMediaStream(const WebMediaStreamDescriptor&) = 0; > > nit: this function says "Local" in its name, but the parameter type does not > have "Local" in its name. perhaps this function should just be called > didStopMediaStream? No, only LocalMediaStreams have a stop() method. In the embedder layer we din't distinguish between LocalMS and MS. >> Source/WebKit/chromium/public/platform/WebMediaStreamCenterClient.h:42 >> + virtual void endLocalMediaStream(const WebMediaStreamDescriptor&) = 0; > > nit: here you use the term "end" but on WebMediaStreamCenter, I see the term "stop". > Is there a relationship between ending a stream and it being reported as stopped? > Should we try to use the same term here if they are related? Good catch, changed to stop. >> Source/WebKit/chromium/public/platform/WebMediaStreamDescriptor.h:55 >> + WebMediaStreamDescriptor(WebCore::MediaStreamDescriptor*); > > nit: are you sure you need both constructors? Yes, otherwise we would need temporary variables. >> Source/WebKit/chromium/public/platform/WebMediaStreamSourcesQueryClient.h:57 >> + WEBKIT_EXPORT void mediaStreamSourcesQueryCompleted(const WebVector<WebMediaStreamSource>&); > > nit: It seems redundant to include the class name in this method. This method > is already scoped to a class that has in its name "media stream sources query", > so why repeat that here? it seems like you could just call this method > didComplete. > > WebUserMediaRequest is fairly similar to this interface. In that case, we > used the term "request" in the method name, which was slightly redundant > but seemed to work nicely. So, following that we could call this method here > queryCompleted or didCompleteQuery. > > I'm also curious about the name of this class. Is it really a Client? Or, is > it perhaps more of a Request interface along the lines of WebUserMediaRequest? > Should we call this WebMediaStreamSourcesRequest? Is the WebCore version > similarly mis-named? it would be nice to be consistent with how things are > named. Changed name to didCompleteQuery. Regarding if this is a client or not I leave up to you to decide. MediaStreamSourcesQueryClient is an interface that got introduced in the platform refactoring, and is only implemented by UserMediaRequest.

Created attachment 122415 [details] Patch

Comment on attachment 122415 [details] Patch Attachment 122415 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11115939

Created attachment 122416 [details] Patch

Created attachment 122748 [details] Patch

(In reply to comment #14) > >> Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:44 > >> + virtual void didSetMediaStreamTrackEnabled(const WebMediaStreamDescriptor&, size_t trackIndex) = 0; > > > > nit: perhaps this function could just be named didEnableMediaStreamTrack? > > The method on MediaStreamTrack is called setEnabled() and the name didSetMediaStreamTrackEnabled reflects that. Maybe didSetEnabledOnMediaStream would be longer but clearer? When you call setEnabled() on MediaStreamTrack you put it into an "enabled" state, right? One way of telling someone that that happened is to say that "the MediaStreamTrack was enabled". In other words, it doesn't seem critical to retain the word "set" when describing how the object became enabled. This is why I thought didEnableMediaStreamTrack makes sense. It tells us that the MediaStreamTrack was enabled, but with the "did" prefix as a way of indicating that the thing being reported already happened. > >> Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:45 > >> + virtual void didStopLocalMediaStream(const WebMediaStreamDescriptor&) = 0; > > > > nit: this function says "Local" in its name, but the parameter type does not > > have "Local" in its name. perhaps this function should just be called > > didStopMediaStream? > > No, only LocalMediaStreams have a stop() method. In the embedder layer we din't distinguish between LocalMS and MS. OK! > >> Source/WebKit/chromium/public/platform/WebMediaStreamDescriptor.h:55 > >> + WebMediaStreamDescriptor(WebCore::MediaStreamDescriptor*); > > > > nit: are you sure you need both constructors? > > Yes, otherwise we would need temporary variables. OK... most of these wrapper classes don't have both forms of constructor, but OK. > >> Source/WebKit/chromium/public/platform/WebMediaStreamSourcesQueryClient.h:57 > >> + WEBKIT_EXPORT void mediaStreamSourcesQueryCompleted(const WebVector<WebMediaStreamSource>&); > > > > nit: It seems redundant to include the class name in this method. This method > > is already scoped to a class that has in its name "media stream sources query", > > so why repeat that here? it seems like you could just call this method > > didComplete. > > > > WebUserMediaRequest is fairly similar to this interface. In that case, we > > used the term "request" in the method name, which was slightly redundant > > but seemed to work nicely. So, following that we could call this method here > > queryCompleted or didCompleteQuery. > > > > I'm also curious about the name of this class. Is it really a Client? Or, is > > it perhaps more of a Request interface along the lines of WebUserMediaRequest? > > Should we call this WebMediaStreamSourcesRequest? Is the WebCore version > > similarly mis-named? it would be nice to be consistent with how things are > > named. > > Changed name to didCompleteQuery. Regarding if this is a client or not I leave up to you to decide. > MediaStreamSourcesQueryClient is an interface that got introduced in the platform refactoring, and is only implemented by UserMediaRequest. OK... I would probably use the Request suffix since this class seems to best fit the emerging pattern of *Request classes.

(In reply to comment #19) > (In reply to comment #14) > > >> Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:44 > > >> + virtual void didSetMediaStreamTrackEnabled(const WebMediaStreamDescriptor&, size_t trackIndex) = 0; > > > > > > nit: perhaps this function could just be named didEnableMediaStreamTrack? > > > > The method on MediaStreamTrack is called setEnabled() and the name didSetMediaStreamTrackEnabled reflects that. Maybe didSetEnabledOnMediaStream would be longer but clearer? > > When you call setEnabled() on MediaStreamTrack you put it into an "enabled" state, right? One way of telling someone that that happened is to say that "the MediaStreamTrack was enabled". In other words, it doesn't seem critical to retain the word "set" when describing how the object became enabled. This is why I thought didEnableMediaStreamTrack makes sense. It tells us that the MediaStreamTrack was enabled, but with the "did" prefix as a way of indicating that the thing being reported already happened. > "Unfortunately" life is not so simple; setEnabled takes an boolean parameter and can thus both enable and disable a track. Will didChangeMediaStreamTrackState work? It is less ambivalent. > > > >> Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:45 > > >> + virtual void didStopLocalMediaStream(const WebMediaStreamDescriptor&) = 0; > > > > > > nit: this function says "Local" in its name, but the parameter type does not > > > have "Local" in its name. perhaps this function should just be called > > > didStopMediaStream? > > > > No, only LocalMediaStreams have a stop() method. In the embedder layer we din't distinguish between LocalMS and MS. > > OK! > > > > >> Source/WebKit/chromium/public/platform/WebMediaStreamDescriptor.h:55 > > >> + WebMediaStreamDescriptor(WebCore::MediaStreamDescriptor*); > > > > > > nit: are you sure you need both constructors? > > > > Yes, otherwise we would need temporary variables. > > OK... most of these wrapper classes don't have both forms of constructor, but OK. > > > > >> Source/WebKit/chromium/public/platform/WebMediaStreamSourcesQueryClient.h:57 > > >> + WEBKIT_EXPORT void mediaStreamSourcesQueryCompleted(const WebVector<WebMediaStreamSource>&); > > > > > > nit: It seems redundant to include the class name in this method. This method > > > is already scoped to a class that has in its name "media stream sources query", > > > so why repeat that here? it seems like you could just call this method > > > didComplete. > > > > > > WebUserMediaRequest is fairly similar to this interface. In that case, we > > > used the term "request" in the method name, which was slightly redundant > > > but seemed to work nicely. So, following that we could call this method here > > > queryCompleted or didCompleteQuery. > > > > > > I'm also curious about the name of this class. Is it really a Client? Or, is > > > it perhaps more of a Request interface along the lines of WebUserMediaRequest? > > > Should we call this WebMediaStreamSourcesRequest? Is the WebCore version > > > similarly mis-named? it would be nice to be consistent with how things are > > > named. > > > > Changed name to didCompleteQuery. Regarding if this is a client or not I leave up to you to decide. > > MediaStreamSourcesQueryClient is an interface that got introduced in the platform refactoring, and is only implemented by UserMediaRequest. > > OK... I would probably use the Request suffix since this class seems to best fit the > emerging pattern of *Request classes. OK, will change to WebMediaStreamSourcesRequest.

Created attachment 123538 [details] Patch

(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #14) > > > >> Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:44 > > > >> + virtual void didSetMediaStreamTrackEnabled(const WebMediaStreamDescriptor&, size_t trackIndex) = 0; > > > > > > > > nit: perhaps this function could just be named didEnableMediaStreamTrack? > > > > > > The method on MediaStreamTrack is called setEnabled() and the name didSetMediaStreamTrackEnabled reflects that. Maybe didSetEnabledOnMediaStream would be longer but clearer? > > > > When you call setEnabled() on MediaStreamTrack you put it into an "enabled" state, right? One way of telling someone that that happened is to say that "the MediaStreamTrack was enabled". In other words, it doesn't seem critical to retain the word "set" when describing how the object became enabled. This is why I thought didEnableMediaStreamTrack makes sense. It tells us that the MediaStreamTrack was enabled, but with the "did" prefix as a way of indicating that the thing being reported already happened. > > > > "Unfortunately" life is not so simple; setEnabled takes an boolean parameter and can thus both enable and disable a track. Will didChangeMediaStreamTrackState work? It is less ambivalent. Oh! Sorry for missing that detail. In that case, you have a couple options. Break it up into two functions: didEnableMediaStreamTrack didDisableMediaStreamTrack Or, you could try to express the changing of an enabled state... Maybe the mouthful didChangeMediaStreamTrackEnabledState would work. I'd probably go with the two methods. > OK, will change to WebMediaStreamSourcesRequest. OK

Created attachment 123927 [details] Patch

Comment on attachment 123927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123927&action=review I had trouble understanding this patch. It seems to be doing lots of different things, and the ChangeLog didn't help me understand. If you'd like me to review this patch, I'm going to need some more help. If you've got another reviewer lined up, please feel free to ignore me. > Source/WebCore/ChangeLog:4 > + [chromium] MediaStream API: Adding the embedding code for MediaStreamCenter > + https://bugs.webkit.org/show_bug.cgi?id=73130 This ChangeLog doesn't tell me anything about this change. :( > Source/WebKit/chromium/bridge/MediaStreamCenterInternal.h:52 > + MediaStreamCenterInternal(MediaStreamCenter*); Please add the explicit keyword > Source/WebKit/chromium/bridge/MediaStreamCenterInternal.h:64 > + WebKit::WebMediaStreamCenter* m_private; > + MediaStreamCenter* m_owner; What is the ownership model here? Why are raw pointers ok?

Created attachment 124100 [details] Patch

(In reply to comment #24) > (From update of attachment 123927 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123927&action=review > > I had trouble understanding this patch. It seems to be doing lots of different things, and the ChangeLog didn't help me understand. If you'd like me to review this patch, I'm going to need some more help. If you've got another reviewer lined up, please feel free to ignore me. > Happy to have you as a reviewer! This patch/feature follows the exact same pattern as PeerHandler that you helped review a while back. The MediaStreamCenter header file and conditionally compiled cpp file are in WebCore/platform/mediastream/MediaStreamCenter.h/cpp. Chromium has its own implementation of most of MediaStreamCenters functions in WebKit/chromium/bridge/MediaStreamCenter.cpp. This implementation creates a MediaStreamCenterInternal object and forwards all calls to it. The MediaStreamCenterInternal class asks webKitPlatformSupport for the WebMediaStreamCenter (WebKit/chromium/public/platform/WebMediaStreamCenter.h) instance and routes messages back and forth. I would say that this patch just does one thing but due to other code changes and review comments the patch has grown. There's very little code that actually does anything other than passing messages.

Comment on attachment 123927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123927&action=review >> Source/WebKit/chromium/bridge/MediaStreamCenterInternal.h:52 >> + MediaStreamCenterInternal(MediaStreamCenter*); > > Please add the explicit keyword Fixed. >> Source/WebKit/chromium/bridge/MediaStreamCenterInternal.h:64 >> + MediaStreamCenter* m_owner; > > What is the ownership model here? Why are raw pointers ok? m_private: The WebKit::WebMediaStreamCenter* is not a object we own, we just get a pointer to it from webKitPlatformSupport. m_owner: Each instance of MediaStreamCenter owns exactly one MediaStreamCenterInternal object and MediaStreamCenterInternal::m_owner just points back to its owner. MediaStreamCenterInternal

> m_private: > The WebKit::WebMediaStreamCenter* is not a object we own, we just get a pointer to it from webKitPlatformSupport. > > m_owner: > Each instance of MediaStreamCenter owns exactly one MediaStreamCenterInternal object and MediaStreamCenterInternal::m_owner just points back to its owner. > MediaStreamCenterInternal Makes sense. Thanks!

Comment on attachment 124100 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124100&action=review > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:339 > + virtual WebMediaStreamCenter* mediaStreamCenter(WebMediaStreamCenterClient*) { return 0; } since mediaStreamCenter is a getter, it seems like you should be able to call it multiple times and get the same WebMediaStreamCenter instance back. however, to call this function you have to pass a Client. what if you pass a different Client each time? does the Client on the shared WebMediaStreamCenter change? my point is that you are using an atypical pattern here. usually, getters like this do not take parameters (e.g., see the clipboard() method). on the other hand, create* functions do take parameters (e.g., see the createLocalStorageNamespace function). a couple options: 1) Rename to createMediaStreamCenter and have WebKit own the WebMediaStreamCenter. 2) Replace WebMediaStreamCenterClient::stopLocalMediaStream() with a method on WebMediaStreamDescriptor (e.g., a stop() method) or a static method on some other class. Yeah, looking at MediaStreamCenter::endLocalMediaStream(), I see that it does not rely on any member variables of MediaStreamCenter. it just operates on the descriptor. > Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:42 > + virtual void queryMediaStreamSources(const WebMediaStreamSourcesQueryClient&) = 0; is this supposed to be WebMediaStreamSourcesRequest instead? > Source/WebKit/chromium/public/platform/WebMediaStreamComponent.h:56 > + WEBKIT_EXPORT bool enabled() const; nit: rename to "isEnabled" since it is asking a question > Source/WebKit/chromium/src/WebMediaStreamSourcesRequest.cpp:59 > + if (!isNull()) would the WebMediaStreamSourcesRequest passed to WebMediaStreamCenter ever really be null? can you instead just assert that it is never null when audio/video/didCompleteQuery methods are called? or, is it possible for it to later become null? > Source/WebKit/chromium/src/WebMediaStreamSourcesRequest.cpp:73 > + if (!isNull()) { nit: if this is really needed, then early return instead so that the bulk of the function can have less indentation.

Created attachment 125120 [details] Patch

Comment on attachment 124100 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124100&action=review >> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:339 >> + virtual WebMediaStreamCenter* mediaStreamCenter(WebMediaStreamCenterClient*) { return 0; } > > since mediaStreamCenter is a getter, it seems like you should be able to call it > multiple times and get the same WebMediaStreamCenter instance back. however, to > call this function you have to pass a Client. what if you pass a different > Client each time? does the Client on the shared WebMediaStreamCenter change? > > my point is that you are using an atypical pattern here. usually, getters > like this do not take parameters (e.g., see the clipboard() method). on the > other hand, create* functions do take parameters (e.g., see the > createLocalStorageNamespace function). > > a couple options: > > 1) Rename to createMediaStreamCenter and have WebKit own the WebMediaStreamCenter. > 2) Replace WebMediaStreamCenterClient::stopLocalMediaStream() with a method on > WebMediaStreamDescriptor (e.g., a stop() method) or a static method on some other > class. Yeah, looking at MediaStreamCenter::endLocalMediaStream(), I see that it > does not rely on any member variables of MediaStreamCenter. it just operates on > the descriptor. This function would only be called once per tab since MediaStreamCenter is a singleton but I have changed it to createMediaStreamCenter so that it follows the general pattern. >> Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:42 >> + virtual void queryMediaStreamSources(const WebMediaStreamSourcesQueryClient&) = 0; > > is this supposed to be WebMediaStreamSourcesRequest instead? Good catch! >> Source/WebKit/chromium/public/platform/WebMediaStreamComponent.h:56 >> + WEBKIT_EXPORT bool enabled() const; > > nit: rename to "isEnabled" since it is asking a question Fixed. >> Source/WebKit/chromium/src/WebMediaStreamSourcesRequest.cpp:59 >> + if (!isNull()) > > would the WebMediaStreamSourcesRequest passed to WebMediaStreamCenter ever really be null? > can you instead just assert that it is never null when audio/video/didCompleteQuery methods > are called? or, is it possible for it to later become null? ASSERTS is indeed much better. Fixed. >> Source/WebKit/chromium/src/WebMediaStreamSourcesRequest.cpp:73 >> + if (!isNull()) { > > nit: if this is really needed, then early return instead so that the bulk of the function can have less indentation. Changed to ASSERT.

Comment on attachment 125120 [details] Patch Clearing flags on attachment: 125120 Committed r106581: <http://trac.webkit.org/changeset/106581>

All reviewed patches have been landed. Closing bug.