[webkit-reviews] review denied: [Bug 58550] [Chromium] Media Stream API: add the Chromium WebKit interfaces : [Attachment 104670] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Tommy Widenflycht
<tommyw at google.com>'s request for review:
Bug 58550: [Chromium] Media Stream API: add the Chromium WebKit interfaces
https://bugs.webkit.org/show_bug.cgi?id=58550

Attachment 104670: Patch
https://bugs.webkit.org/attachment.cgi?id=104670&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104670&action=review


> Source/WebKit/chromium/public/WebMediaStreamClient.h:36
> +enum WebGenerateStreamOptionFlag {

nit: one new header file per type.

nit: the enum name is WebFooOptionFlag, but the values are WebFooRequestX.  we
have a convention of naming enums like so:

  enum WebFoo {
      WebFooX,
      WebFooY,
      WebFooZ
  };

> Source/WebKit/chromium/public/WebMediaStreamClient.h:42
> +typedef unsigned WebGenerateStreamOptionFlags;

we normally just accomplish this with a comment.  i'd give generateStream an
int
parameter with a name like optionFlags.  then, i'd comment that optionFlags can

be a bit-wise combination of WebGenerateStreamOptionFlag values.

note:  If you only intend this enum to be used with this interface, then you
could just define WebMediaStreamClient::OptionFlag.

> Source/WebKit/chromium/public/WebMediaStreamClient.h:48
> +    // The controller is valid until mediaStreamDestroyed() is invoked.

these comments seem to contradict one another:

"The controller is valid until mediaStreamDestroyed() is invoked." says that
the WebMediaStreamClient does not have ownership of the given pointer since
something else is responsible for telling you when it gets invalidated.

"Ownership of the WebMediaStreamController is transferred to the client." says
exactly the opposite.

> Source/WebKit/chromium/public/WebMediaStreamClient.h:51
> +    virtual void mediaStreamDestroyed() = 0;

maybe this method should be named "mediaStreamControllerDestroyed()" to better
indicate that the WebMediaStreamController has been destroyed?

why do we need this method?  why not just have WebKit call setController(0) to
tell the client when the controller is no longer valid?

> Source/WebKit/chromium/public/WebMediaStreamClient.h:53
> +    virtual void generateStream(int requestId, WebGenerateStreamOptionFlags,
const WebSecurityOrigin&) = 0;

what is this "requestId"?

> Source/WebKit/chromium/public/WebMediaStreamClient.h:56
> +    virtual void processSignalingMessage(int peerConnectionId, const
WebString& message) = 0;

what is this "peerConnectionId"?  why do we have identifiers here and not
objects?

do these identifiers exist to support chrome's multi-process architecture?

it seems like perhaps there is a peer connection interface that should be used
here?  won't we have something like these methods for direct peer connection
usage?

> Source/WebKit/chromium/public/WebMediaStreamClient.h:59
> +    virtual void newPeerConnection(int peerConnectionId, const WebString&
configuration) = 0;

what is the format of the "configuration" string, or how is it defined? 
documentation seems to be missing.

> Source/WebKit/chromium/public/WebMediaStreamController.h:45
> +    // Same as WebCore::NavigatorUserMediaError::PERMISSION_DENIED.

we normally avoid comments that mention WebCore implementation details in
WebKit API headers.  no one will think to ever fix these comments when they
become untrue.	WebCore is hacked on by a lot of non-Chromium folks!!

> Source/WebKit/chromium/public/WebMediaStreamController.h:50
> +    WEBKIT_EXPORT void streamGenerated(int requestId, const WebString&
streamLabel, WebMediaStreamTrackList&);

it seems like there is a whole processing model for WebMediaStreamController
and WebMediaStreamClient that could use some documentation.  it is not obvious,

if i am implementing WebMediaStreamClient, how I am to use
WebMediaStreamController.