Bug 96023 – Expose ability to create WebUserMediaRequest from chromium side

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Platform

[Justin Lin]

We want to be able to create a WebUserMediaClient from an extension and use it to request different types of media sterams.

[Justin Lin]

Created attachment 162583 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Justin Lin]

Created attachment 162589 [details]
Patch

[Justin Lin]

Created attachment 162607 [details]
Patch

[Hin-Chung Lam]

Comment on attachment 162589 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162589&action=review

> Source/WebKit/chromium/ChangeLog:8
> +        Chromium implements the WebUserMediaClient getDeviceStream method that takes a WebUserMediaRequest to provide other types of media streams to extensions.

it's requestDeviceMedia.

Also need more details here. It is unclear how these methods are connected.

e.g.

This change is to allow Chromium port to make a WebUserMediaRequest for accessing a MediaStream by for example Chrome Extensions API. WebFrame::createWebUserMediaRequest() is added such that Chromium can construct this request and then submit to WebUserMediaClient which Chromium also implements.

> Source/WebKit/chromium/ChangeLog:16
> +        (WebKit::WebUserMediaClient::requestTabMedia):

requestDeviceMedia here again.

[Adam Barth]

Comment on attachment 162607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162607&action=review

> Source/WebKit/chromium/public/WebFrame.h:327
> +    virtual WebUserMediaRequest *createWebUserMediaRequest(v8::Isolate*,

WebUserMediaRequest *  ->    WebUserMediaRequest*

Do we need to pass in the isolate here?

Also, we should take v8::Handle as a parameter rather than v8::Local

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1002
> +WebKit::WebUserMediaRequest *WebFrameImpl::createWebUserMediaRequest(v8::Isolate *isolate,

In WebKit style, the * goes on the other side.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1006
> +    UserMediaController* userMedia = UserMediaController::from(m_frame ? m_frame->page() : 0);

We should just return early if there's no m_frame.  There's not much point in continuing.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1008
> +      return 0;

Four-space indent.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1013
> +      requestErrorCallback = V8NavigatorUserMediaErrorCallback::create(errorCallback, getScriptExecutionContext());

Four-space indent.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1015
> +    Dictionary requestOptions = Dictionary(options, isolate);

You can just use v8::Isolate::current() here rather than taking the isolate as a parameter.

[Adam Barth]

I'm not sure I quite understand how this integrates with the extension system.  Is there some code or a diagram that I can look at?

[Peter Beverloo (cr-android ews)]

Comment on attachment 162589 [details]
Patch

Attachment 162589 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13777371

[Justin Lin]

Created attachment 162631 [details]
Patch

[Adam Barth]

Comment on attachment 162631 [details]
Patch

Your *s are still on the wrong side :)

[Justin Lin]

Created attachment 162632 [details]
Patch

[Justin Lin]

(In reply to comment #10)
> (From update of attachment 162631 [details])
> Your *s are still on the wrong side :)

Yes, nice catch :)
Thanks for the reviews so far!

Clarified a little bit the ChangeLog with Alpha's comment. It isn't extension specific, but we'd like to be able to make create this object and make a device media stream request from for example an extensions API. See this change:
http://codereview.chromium.org/10928043/

[Peter Beverloo (cr-android ews)]

Comment on attachment 162632 [details]
Patch

Attachment 162632 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13765855

[Adam Barth]

Comment on attachment 162632 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162632&action=review

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1009
> +    RefPtr<WebCore::NavigatorUserMediaSuccessCallback> requestSuccessCallback = WebCore::V8NavigatorUserMediaSuccessCallback::create(v8::Local<v8::Value>::New(successCallback), getScriptExecutionContext());

There's no need to call getScriptExecutionContext.  We can get that object directly from m_frame->document()

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1015
> +    RefPtr<WebCore::UserMediaRequest> request = WebCore::UserMediaRequest::create(m_frame->document(), userMedia, requestOptions, requestSuccessCallback, requestErrorCallback);

Notice that you're using m_frame->document() here.

[Adam Barth]

For my own reference, the client of this API is here:

http://codereview.chromium.org/10928043/diff/2001/chrome/renderer/extensions/tab_capture_custom_bindings.cc?column_width=80

[Adam Barth]

Have you considered any other approaches?  This feels pretty hacky.  The WebKit layer shouldn't be involved in low-level things like V8NavigatorUserMediaErrorCallback::create.

[Tommy Widenflycht]

Hmm, I don't particularly like this patch either since it is too much of a hack. You are trying to squeese in functionality using classes not designed for that purpose which can lead to trouble later on. In WebKit the philosophy is that we try to do a proper design even if it takes much more time, of which I btw have personal experience, since it leads to better code.

You also break the "contract" that all mediastream Web* classes corresponds to a WebCore/platform object. This is a relatively minor objection though.

Have you tried creating your own request class instead of reusing WebUserMediaRequest, and doing a bit of refactoring on the chromium side?

[Adam Barth]

Comment on attachment 162632 [details]
Patch

Yeah, I'm sorry not to have replied earlier.  This doesn't seem to be the right approach.  Adding a bunch of user-media specific code to WebFrame, we should improve the user-media API to help you do what you want.

[Adam Barth]

For example, perhaps we need a create method on WebUserMedia that can take v8 callback objects?

Perhaps you can have the extension JavaScript code call the real createUserMedia JS API?

[Justin Lin]

(In reply to comment #19)
> For example, perhaps we need a create method on WebUserMedia that can take v8 callback objects?

Yes, that was actually what I was working on along with a separate WebUserMediaRequest type, but then got distracted last week. Will update the patch soon.

> 
> Perhaps you can have the extension JavaScript code call the real createUserMedia JS API?

We need some more parameters in our version.

Thanks.

[Justin Lin]

Created attachment 165962 [details]
Patch

[WebKit Review Bot]

Comment on attachment 165962 [details]
Patch

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

[Justin Lin]

Created attachment 165967 [details]
Patch

[WebKit Review Bot]

Comment on attachment 165967 [details]
Patch

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

[Adam Barth]

Comment on attachment 165967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165967&action=review

This still strikes me as the wrong approach.  It's tough to write correct custom bindings in the first place.  It's even tougher to write them in the wrong layer (i.e., the WebKit layer rather than the bindings themselves).

> Source/WebKit/chromium/ChangeLog:6
> +        Reviewed by Adam Barth.

This is not accurate.  I have not reviewed this patch.  Please leave the NOBODY (OOPS!) line in the ChangeLog.  The tools will fill it out with the proper reviewer when landing your patch.

> Source/WebKit/chromium/src/WebDeviceMediaRequest.cpp:54
> +    WebFrame* webFrame = WebFrame::frameForCurrentContext();

It's very unlikely that we should call frameForCurrentContext.

> Source/WebKit/chromium/src/WebDeviceMediaRequest.cpp:61
> +    ScriptExecutionContext* scriptExecutionContext = frame->document()->scriptExecutionContext();

This is really the wrong way to get the current script execution context.  The bindings know how to do this work properly.  Doing this work from the WebKit layer is pretty awkward.

[Adam Barth]

I still don't really understand the problem you're trying to solve, which makes it difficult to suggest a better design.  Maybe we should talk over VC?

[Adam Barth]

One option is to expose a JavaScript API only to extensions.  We typically don't do that since WebKit is mainly focused on providing APIs that are part of the open web platform.  If I had a better understanding of the underlying problem you're trying to solve, I might have more useful suggestions.

I really think the approach of trying to write custom bindings code in the WebKit layer is unworkable.

[Justin Lin]

(In reply to comment #25)
> (From update of attachment 165967 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165967&action=review
> 
> This still strikes me as the wrong approach.  It's tough to write correct custom bindings in the first place.  It's even tougher to write them in the wrong layer (i.e., the WebKit layer rather than the bindings themselves).
> 
> > Source/WebKit/chromium/ChangeLog:6
> > +        Reviewed by Adam Barth.
> 
> This is not accurate.  I have not reviewed this patch.  Please leave the NOBODY (OOPS!) line in the ChangeLog.  The tools will fill it out with the proper reviewer when landing your patch.

Ah, didn't realize the scripts did that.

> 
> > Source/WebKit/chromium/src/WebDeviceMediaRequest.cpp:54
> > +    WebFrame* webFrame = WebFrame::frameForCurrentContext();
> 
> It's very unlikely that we should call frameForCurrentContext.
> 
> > Source/WebKit/chromium/src/WebDeviceMediaRequest.cpp:61
> > +    ScriptExecutionContext* scriptExecutionContext = frame->document()->scriptExecutionContext();
> 
> This is really the wrong way to get the current script execution context.  The bindings know how to do this work properly.  Doing this work from the WebKit layer is pretty awkward.

(In reply to comment #26)
> I still don't really understand the problem you're trying to solve, which makes it difficult to suggest a better design.  Maybe we should talk over VC?

That sounds good! I'll schedule something.

(In reply to comment #27)
> One option is to expose a JavaScript API only to extensions.  We typically don't do that since WebKit is mainly focused on providing APIs that are part of the open web platform.  If I had a better understanding of the underlying problem you're trying to solve, I might have more useful suggestions.
> 

Something like that might work.

> I really think the approach of trying to write custom bindings code in the WebKit layer is unworkable.

[Tommy Widenflycht]

Adam knows the WebRTC code really well and if he's happy I'm happy :)
However if you want me to join the meeting I'll happily do so (but I'm in GMT+2).

[Stephen Chenney]

Given there was a meeting, then no action, I'm assuming this is WontFix.