Bug 73130 – [chromium] MediaStream API: Adding the embedding code for MediaStreamCenter

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Tommy Widenflycht]

Last piece of the embedder infrastructure for the MediaStream feature.

[Tommy Widenflycht]

Created attachment 116611 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Tommy Widenflycht]

Created attachment 116724 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116724 [details]
Patch

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

[Tommy Widenflycht]

Created attachment 116739 [details]
Patch

[Adam Barth]

Comment on attachment 116739 [details]
Patch

Looks reasonable to me, but we need fishd to review.

[Collabora GTK+ EWS bot]

Comment on attachment 116739 [details]
Patch

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

[Tommy Widenflycht]

Created attachment 117189 [details]
Patch

[Darin Fisher (:fishd, Google)]

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?

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

Created attachment 118036 [details]
Patch

rebase

[Darin Fisher (:fishd, Google)]

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.

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

Created attachment 122415 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 122415 [details]
Patch

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

[Tommy Widenflycht]

Created attachment 122416 [details]
Patch

[Tommy Widenflycht]

Created attachment 122748 [details]
Patch

[Darin Fisher (:fishd, Google)]

(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.

[Tommy Widenflycht]

(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.

[Tommy Widenflycht]

Created attachment 123538 [details]
Patch

[Darin Fisher (:fishd, Google)]

(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

[Tommy Widenflycht]

Created attachment 123927 [details]
Patch

[Adam Barth]

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?

[Tommy Widenflycht]

Created attachment 124100 [details]
Patch

[Tommy Widenflycht]

(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.

[Tommy Widenflycht]

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

[Adam Barth]

> 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!

[Darin Fisher (:fishd, Google)]

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.

[Tommy Widenflycht]

Created attachment 125120 [details]
Patch

[Tommy Widenflycht]

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.

[WebKit Review Bot]

Comment on attachment 125120 [details]
Patch

Clearing flags on attachment: 125120

Committed r106581: <http://trac.webkit.org/changeset/106581>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.