We want to be able to create a WebUserMediaClient from an extension and use it to request different types of media sterams.
Created attachment 162583 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Created attachment 162589 [details] Patch
Created attachment 162607 [details] Patch
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.
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.
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?
Comment on attachment 162589 [details] Patch Attachment 162589 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13777371
Created attachment 162631 [details] Patch
Comment on attachment 162631 [details] Patch Your *s are still on the wrong side :)
Created attachment 162632 [details] Patch
(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/
Comment on attachment 162632 [details] Patch Attachment 162632 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13765855
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.
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
Have you considered any other approaches? This feels pretty hacky. The WebKit layer shouldn't be involved in low-level things like V8NavigatorUserMediaErrorCallback::create.
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?
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.
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?
(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.
Created attachment 165962 [details] Patch
Comment on attachment 165962 [details] Patch Attachment 165962 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14037751
Created attachment 165967 [details] Patch
Comment on attachment 165967 [details] Patch Attachment 165967 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14018918
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.
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?
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.
(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.
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).
Given there was a meeting, then no action, I'm assuming this is WontFix.