Stub for WebRTCDeviceProvider

Device provider for chrome://inspect that connects to DevTools bridge (components/devtools_bridge).

It hosts a WebContents to run WebRTC and GCD related code

BUG=383418
Committed: https://crrev.com/91af3ad699d1067d8398b42a6f1fad737b95b12b
Cr-Commit-Position: refs/heads/master@{#306755}

[SeRya, 2014-11-21 16:19:48]

serya@chromium.org changed reviewers:
+ tsepez@chromium.org

[SeRya, 2014-11-21 16:31:14]

Hi Tom,

Please, review general approach to implementing the client. Especially if it is
appropriate to host this code in the "chrome-devtools" schema.

Change in content/renderer/web_ui_extension.cc doesn't enables WebUI for
chrome-sevtools://devtools/... because ender_view->GetEnabledBindings() &
BINDINGS_POLICY_WEB_UI) is false for it.

[pfeldman, 2014-11-21 16:45:39]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2014-11-21 16:45:39]

Hm. I am not sure this is ready for the security review. I think you need a
round of internal reviews and a design doc before you move further.

[Tom Sepez, 2014-11-21 18:49:16]

On 2014/11/21 16:45:39, pfeldman wrote:
> Hm. I am not sure this is ready for the security review. I think you need a
> round of internal reviews and a design doc before you move further.

Yes, can you give us the big picture on what it is you are trying to do? 
Thanks.

[Tom Sepez, 2014-11-21 18:51:38]

https://codereview.chromium.org/746663002/diff/20001/content/renderer/web_ui_...
File content/renderer/web_ui_extension.cc (right):

https://codereview.chromium.org/746663002/diff/20001/content/renderer/web_ui_...
content/renderer/web_ui_extension.cc:46:
frame_url.SchemeIs(kChromeDevToolsScheme) ||
No.  This violates the principle of least privilege, granting to all of devtools
privileges that are beyond what is needed.  You'll need to find another
approach.

[mnaganov (inactive), 2014-11-21 21:19:37]

mnaganov@chromium.org changed reviewers:
+ mnaganov@chromium.org

[mnaganov (inactive), 2014-11-21 21:19:38]

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
File
components/devtools_bridge/android/client/javatests/src/org/chromium/components/devtools_bridge/browsertests/TestApplication.java
(right):

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/android/client/javatests/src/org/chromium/components/devtools_bridge/browsertests/TestApplication.java:29:
"snapshot_blob.bin",
Extra trailing comma.

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
File components/devtools_bridge/client/js/client_session.js (right):

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/client/js/client_session.js:25: '_sessionId':
this.id_,
I don't see initialization of 'id_' field.

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/client/js/client_session.js:27: '_offer': offer.sdp
Missing quotes around the RHS value?

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/client/js/client_session.js:33: console.log("Offer
failed: " + error);
Please prefer using single quotes.

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
File components/devtools_bridge/client/js/gcd_client.js (right):

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/client/js/gcd_client.js:10: console.log("Sending
command");
Please prefer using single quotes.

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
File components/devtools_bridge/client/js/web_client.js (right):

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/client/js/web_client.js:10: this.sessions_ = {};
Wrong indentation.

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/client/js/web_client.js:15: console.log("Starting
session " + deviceId);
Please prefer using single quotes.

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
File components/devtools_bridge/test/android/client/web_client_android.cc
(right):

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/test/android/client/web_client_android.cc:40: if
(!base::JSONWriter::Write(command, &json)) {
I think it makes sense to serialize to JSON much earlier -- in
JSProxy::HandleSendCommand, as it kind of scary to drag the pointer owned by
someone else this far. Also, the error reporting will be better.

[Tom Sepez, 2014-11-21 22:10:37]

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
File components/devtools_bridge/client/web_client_controller.cc (right):

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/client/web_client_controller.cc:21:
content::WebUIDataSource* source =
Ah, here's the part I was worried about.  Does this work?  I didn't think you
could get this registered without enabling webui.

[SeRya, 2014-11-24 10:37:51]

Patchset #3 (id:40001) has been deleted

[SeRya, 2014-11-24 11:10:06]

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
File
components/devtools_bridge/android/client/javatests/src/org/chromium/components/devtools_bridge/browsertests/TestApplication.java
(right):

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/android/client/javatests/src/org/chromium/components/devtools_bridge/browsertests/TestApplication.java:29:
"snapshot_blob.bin",
On 2014/11/21 21:19:38, mnaganov (cr) wrote:
> Extra trailing comma.

Done.

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
File components/devtools_bridge/client/js/client_session.js (right):

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/client/js/client_session.js:25: '_sessionId':
this.id_,
On 2014/11/21 21:19:38, mnaganov (cr) wrote:
> I don't see initialization of 'id_' field.

Done.

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/client/js/client_session.js:27: '_offer': offer.sdp
On 2014/11/21 21:19:38, mnaganov (cr) wrote:
> Missing quotes around the RHS value?

Don't see anything wrong. Key is quotes, value shouldn't be quotes once it
refers to a string property.

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/client/js/client_session.js:33: console.log("Offer
failed: " + error);
On 2014/11/21 21:19:38, mnaganov (cr) wrote:
> Please prefer using single quotes.

Done.

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
File components/devtools_bridge/client/js/gcd_client.js (right):

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/client/js/gcd_client.js:10: console.log("Sending
command");
On 2014/11/21 21:19:38, mnaganov (cr) wrote:
> Please prefer using single quotes.

Done.

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
File components/devtools_bridge/client/js/web_client.js (right):

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/client/js/web_client.js:10: this.sessions_ = {};
On 2014/11/21 21:19:38, mnaganov (cr) wrote:
> Wrong indentation.

Done.

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
File components/devtools_bridge/client/web_client_controller.cc (right):

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/client/web_client_controller.cc:21:
content::WebUIDataSource* source =
On 2014/11/21 22:10:37, Tom Sepez wrote:
> Ah, here's the part I was worried about.  Does this work?  I didn't think you
> could get this registered without enabling webui.

It works in the test app because I do registering WebUI  in
https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
(line 63).

To make it working in Chrome needed registration l like this
https://codereview.chromium.org/720133002/diff/40001/chrome/browser/ui/webui/...

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
File components/devtools_bridge/test/android/client/web_client_android.cc
(right):

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/test/android/client/web_client_android.cc:40: if
(!base::JSONWriter::Write(command, &json)) {
On 2014/11/21 21:19:38, mnaganov (cr) wrote:
> I think it makes sense to serialize to JSON much earlier -- in
> JSProxy::HandleSendCommand, as it kind of scary to drag the pointer owned by
> someone else this far. Also, the error reporting will be better.

Good point. Done.

https://codereview.chromium.org/746663002/diff/20001/content/renderer/web_ui_...
File content/renderer/web_ui_extension.cc (right):

https://codereview.chromium.org/746663002/diff/20001/content/renderer/web_ui_...
content/renderer/web_ui_extension.cc:46:
frame_url.SchemeIs(kChromeDevToolsScheme) ||
On 2014/11/21 18:51:38, Tom Sepez wrote:
> No.  This violates the principle of least privilege, granting to all of
devtools
> privileges that are beyond what is needed.  You'll need to find another
> approach.

As we discussed, this change does the following thing: It lets 'chrome.send'
pass to the browser process. It only works for 'chrome-devtools://brindge/...'
because 'chrome-devtools://devtools' disables by removing the
BINDINGS_POLICY_WEB_UI flag (because devtools frontend used own bindings).

Than sends may only be handled in:
1. components/devtools_bridge/client/web_client.cc (this WebUIMessageHandler
added deliberately)
2. content/browser/webui/generic_handler.cc (because WebContents adds it).

[pfeldman, 2014-11-24 11:23:21]

Placing some of devtools code to under components and other into chrome/browser
does not sound good. Also, lets figure out the proper transport. As noted above,
using chrome.send is not lgtm.

[mnaganov (inactive), 2014-11-24 11:47:20]

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
File components/devtools_bridge/client/js/client_session.js (right):

https://codereview.chromium.org/746663002/diff/20001/components/devtools_brid...
components/devtools_bridge/client/js/client_session.js:27: '_offer': offer.sdp
On 2014/11/24 11:10:06, SeRya wrote:
> On 2014/11/21 21:19:38, mnaganov (cr) wrote:
> > Missing quotes around the RHS value?
> 
> Don't see anything wrong. Key is quotes, value shouldn't be quotes once it
> refers to a string property.

Oh, sorry. I didn't notice that 'offer' is the argument to the function.

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
File
components/devtools_bridge/android/client/javatests/src/org/chromium/components/devtools_bridge/WebClientTest.java
(right):

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
components/devtools_bridge/android/client/javatests/src/org/chromium/components/devtools_bridge/WebClientTest.java:22:
public class WebClientTest extends InstrumentationTestCase implements
WebClient.Delegate {
I think test classes usually serve as containers only. Can you create an inner
class for this, please?

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
components/devtools_bridge/android/client/javatests/src/org/chromium/components/devtools_bridge/WebClientTest.java:34:
super.tearDown();
super's tearDown() should be called last, I guess.

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
File components/devtools_bridge/client/js/gcd_client.js (right):

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
components/devtools_bridge/client/js/gcd_client.js:19: console.log('Sending
command', command)
Do you need this? In the next file, there is no "else" branch for the case when
there is no "chrome.send" function.

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
File components/devtools_bridge/client/web_client.cc (right):

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
components/devtools_bridge/client/web_client.cc:70: 
nit: extra empty line

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
File components/devtools_bridge/client/web_client.html (right):

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
components/devtools_bridge/client/web_client.html:3: -- Copyright 2014444 The
Chromium Authors. All rights reserved.
2014?

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
File components/devtools_bridge/client/web_client_controller.cc (right):

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
components/devtools_bridge/client/web_client_controller.cc:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
components/devtools_bridge/client/web_client_controller.cc:7: #include
"base/memory/ref_counted_memory.h"
The number of includes here looks suspicious. I don't see usages for most of
them. Can you please revise?

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
components/devtools_bridge/client/web_client_controller.cc:22:
content::WebUIDataSource::Create("bridge");
indentation

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
File components/devtools_bridge/client/web_client_controller.h (right):

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
components/devtools_bridge/client/web_client_controller.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
File components/devtools_bridge/test/android/client/javatests/jni/jni_onload.cc
(right):

https://codereview.chromium.org/746663002/diff/60001/components/devtools_brid...
components/devtools_bridge/test/android/client/javatests/jni/jni_onload.cc:52:
static TestFactory s_factory;
We don't favor statics in Chromium (see
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/p6h3HC8Wro4...)

Please use LazyInstance instead.

[SeRya, 2014-11-24 17:16:13]

serya@chromium.org changed reviewers:
+ dgozman@chromium.org

[SeRya, 2014-11-24 17:16:13]

This CL is not yet ready. I'd like to come to an agreement on overall code
structure before polishing it. Please have a look only on it yet.

[dgozman, 2014-11-26 11:33:25]

Overall, hosting this in WebUI sounds good to me.

It would be nice to see at least some of the interface between
DevToolsBridgeClient and the rest of DevTools (perhaps the DeviceProvider?).

Also, I'd name the whole thing {Wireless,Cloud}DeviceProvider.

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
File chrome/browser/devtools/device/cloud/devtools_bridge_client.cc (right):

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/cloud/devtools_bridge_client.cc:78:
"WebClient.instance.startSession",
Why WebClient? The naming should be consistent.

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/cloud/devtools_bridge_client.cc:127:
web_contents_->GetWebUI()->AddMessageHandler(&dispatcher_);
I'd move this call to DevToolsBridgeUI class.

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/cloud/devtools_bridge_client.cc:160: void
DevToolsBridgeClient::AddResources(
I think this belongs to DevToolsBridgeUI.

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:453: if (url.host()
== chrome::kChromeUIDevToolsBridgeClientHost)
kChromeUIDevToolsBridgeHost?

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/devtools_bridge_ui.cc (right):

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/devtools_bridge_ui.cc:18:
source->DisableContentSecurityPolicy();
I think we could do without this.

[pfeldman, 2014-11-26 11:56:43]

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
File chrome/browser/devtools/device/cloud/devtools_bridge_client.cc (right):

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/cloud/devtools_bridge_client.cc:21: public:
This looks like too many layers of indirection. You should consider inlining
MessageDispatcher into DevToolsBridgeClientImpl.

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/cloud/devtools_bridge_client.cc:40: class
DevToolsBridgeClientImpl : public DevToolsBridgeClient,
Also needs a new name, typically this is SomethingUI

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/cloud/devtools_bridge_client.cc:160: void
DevToolsBridgeClient::AddResources(
On 2014/11/26 11:33:25, dgozman wrote:
> I think this belongs to DevToolsBridgeUI.

I think this whole thing is DevToolsBridgeUI, or better yet
DevToolsWebRTCTransportUI.

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
File chrome/browser/devtools/device/cloud/devtools_bridge_client.h (right):

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/cloud/devtools_bridge_client.h:17: class
DevToolsBridgeClient {
DevToolsBridge name is too generic. What exactly is this class doing?

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/cloud/devtools_bridge_client.h:31: virtual void
StartSession(const std::string& device_id) = 0;
What is session and where do i get device_id?

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
File chrome/browser/devtools/device/cloud/resources.grd (right):

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/cloud/resources.grd:1: <?xml version="1.0"
encoding="UTF-8"?>
You probably want device/webrtc/webui folder name.

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:453: if (url.host()
== chrome::kChromeUIDevToolsBridgeClientHost)
On 2014/11/26 11:33:25, dgozman wrote:
> kChromeUIDevToolsBridgeHost?

kChromeUIDevToolsWebRTCTransportHost

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/devtools_bridge_ui.cc (right):

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/devtools_bridge_ui.cc:20:
DevToolsBridgeClient::AddResources(source);
Merge these all.

[SeRya, 2014-11-26 13:09:14]

Need some clarifications

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
File chrome/browser/devtools/device/cloud/devtools_bridge_client.cc (right):

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/cloud/devtools_bridge_client.cc:40: class
DevToolsBridgeClientImpl : public DevToolsBridgeClient,
On 2014/11/26 11:56:42, pfeldman wrote:
> Also needs a new name, typically this is SomethingUI

SomethigUI is typically content::WebUIController. I do have one:
https://codereview.chromium.org/746663002/diff/80001/chrome/browser/ui/webui/...

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/cloud/devtools_bridge_client.cc:78:
"WebClient.instance.startSession",
On 2014/11/26 11:33:25, dgozman wrote:
> Why WebClient? The naming should be consistent.

Will rename.

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/cloud/devtools_bridge_client.cc:127:
web_contents_->GetWebUI()->AddMessageHandler(&dispatcher_);
On 2014/11/26 11:33:25, dgozman wrote:
> I'd move this call to DevToolsBridgeUI class.

Why? Here dispatcher_ is a member field easily accessible. DevToolsUIBindings
indeed do it in DevToolsBridgeUI. But it costs having a global map
WebContents->DevToolsUIBindings.

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
File chrome/browser/devtools/device/cloud/devtools_bridge_client.h (right):

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/cloud/devtools_bridge_client.h:17: class
DevToolsBridgeClient {
On 2014/11/26 11:56:42, pfeldman wrote:
> DevToolsBridge name is too generic. What exactly is this class doing?

This class intended to run sessions with remote DevToolsBridgeServer's. It
doesn't have own
signaling channel. Instead it delegates sending commands. It help to:
1. move out some complexity.
2. unit testing: signaling could be mocked.
3. isolation abstraction. It described in terms sessions rather than
DeviceProvider implementation.

https://codereview.chromium.org/746663002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/cloud/devtools_bridge_client.h:31: virtual void
StartSession(const std::string& device_id) = 0;
On 2014/11/26 11:56:42, pfeldman wrote:
> What is session and where do i get device_id?

It's GCD device ID. It's opaque to the client. Client uses it to create command.

[SeRya, 2014-11-26 18:01:12]

@dgozman, @pfeldman please take a look at structure (I'm still working on
details).

[dgozman, 2014-11-26 18:52:12]

Architecture looks good. Not sure about naming - this can be confused with
something else. Maybe add DevTools to the name?

[SeRya, 2014-11-26 19:24:25]

On 2014/11/26 18:52:12, dgozman wrote:
> Architecture looks good. Not sure about naming - this can be confused with
> something else. Maybe add DevTools to the name?

Like DevToolsCloudDeviceProvider? Wouldn't it be inconsistent with other
*DeviceProvider's? Also 'devtools-cloud-device-provider' for the host name?

[SeRya, 2014-11-27 10:41:19]

PTAL. Instead of renaming the class I added comments to it with explanation.

[SeRya, 2014-11-27 13:22:21]

Renamed "cloud" to "webrtc" as we discussed.

[dgozman, 2014-11-27 13:45:27]

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/resources.grd (right):

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/resources.grd:14: <include
name="IDR_MAIN_JS" file="js/webrtc_device_provider.js" type="BINDATA" />
IDR_WEBRTC_DEVICE_PROVIDER_JS

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right):

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:16: class
WebRTCDeviceProvider::MessageHandler
This class needs a pointer to device provider to delegate stuff.

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:68:
GURL(chrome::kChromeUIWebRTCDeviceProviderURL),
I think the |SetDefaultResource()| call allows you to just use the host here and
remove the URL constant.

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:73: handler_ =
static_cast<WebUI*>(
Why don't you just AddMessageHandler from here instead of WebUIController?

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.h (right):

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:16: // Provides
remote Clank instances using WebRTC data channel and GCD.
Not Clank, but any components/devtools_bridge embedder.

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:23: * is a
windowless page what implements most of functionality of the
typo: what->that

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider_browsertest.js
(right):

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider_browsertest.js:16:
* Browse to the settings sub-frame.
Wrong comment.

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
File chrome/browser/devtools/webrtc_device_provider_resources.gyp (right):

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/webrtc_device_provider_resources.gyp:8: # GN version:
//chrome/browser/devtools:webrtc_device_provider_resources
I'd merge this file with devtools_protocol_constants.gyp and rename it to
"devtools_resources.gyp".

https://codereview.chromium.org/746663002/diff/160001/chrome/common/url_const...
File chrome/common/url_constants.h (right):

https://codereview.chromium.org/746663002/diff/160001/chrome/common/url_const...
chrome/common/url_constants.h:90: 
nit: extra empty line

[SeRya, 2014-11-27 14:15:09]

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/resources.grd (right):

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/resources.grd:14: <include
name="IDR_MAIN_JS" file="js/webrtc_device_provider.js" type="BINDATA" />
On 2014/11/27 13:45:27, dgozman wrote:
> IDR_WEBRTC_DEVICE_PROVIDER_JS

Done.

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right):

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:16: class
WebRTCDeviceProvider::MessageHandler
On 2014/11/27 13:45:27, dgozman wrote:
> This class needs a pointer to device provider to delegate stuff.

Sure. Will be added on first use.

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:68:
GURL(chrome::kChromeUIWebRTCDeviceProviderURL),
On 2014/11/27 13:45:27, dgozman wrote:
> I think the |SetDefaultResource()| call allows you to just use the host here
and
> remove the URL constant.

Lets remove SetDefaultResource. This URL is not intended to be typed, it removes
one generated file.

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:68:
GURL(chrome::kChromeUIWebRTCDeviceProviderURL),
On 2014/11/27 13:45:27, dgozman wrote:
> I think the |SetDefaultResource()| call allows you to just use the host here
and
> remove the URL constant.

Let's remove SetDefaultResource with webrtc_device_provider_resources.h instead.

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:73: handler_ =
static_cast<WebUI*>(
On 2014/11/27 13:45:27, dgozman wrote:
> Why don't you just AddMessageHandler from here instead of WebUIController?

Problem is the following. If I manually type the URL (for testing purpose)
chrome fails because can't find a handler for 'loaded' message (because in this
way handler not set). It is not a problem (because page loaded manually useless)
but may cause confusion.

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.h (right):

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:16: // Provides
remote Clank instances using WebRTC data channel and GCD.
On 2014/11/27 13:45:27, dgozman wrote:
> Not Clank, but any components/devtools_bridge embedder.

So far our primary goal is Clank. I think specific comment easier to understand
than abstract once.

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:23: * is a
windowless page what implements most of functionality of the
On 2014/11/27 13:45:27, dgozman wrote:
> typo: what->that

Done.

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider_browsertest.js
(right):

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider_browsertest.js:16:
* Browse to the settings sub-frame.
On 2014/11/27 13:45:27, dgozman wrote:
> Wrong comment.

Done.

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
File chrome/browser/devtools/webrtc_device_provider_resources.gyp (right):

https://codereview.chromium.org/746663002/diff/160001/chrome/browser/devtools...
chrome/browser/devtools/webrtc_device_provider_resources.gyp:8: # GN version:
//chrome/browser/devtools:webrtc_device_provider_resources
On 2014/11/27 13:45:27, dgozman wrote:
> I'd merge this file with devtools_protocol_constants.gyp and rename it to
> "devtools_resources.gyp".

It's a useless improvement once we migrating to 'gn' and going to get rid of
both files.

https://codereview.chromium.org/746663002/diff/160001/chrome/common/url_const...
File chrome/common/url_constants.h (right):

https://codereview.chromium.org/746663002/diff/160001/chrome/common/url_const...
chrome/common/url_constants.h:90: 
On 2014/11/27 13:45:27, dgozman wrote:
> nit: extra empty line

Done.

[dgozman, 2014-11-27 14:50:03]

https://codereview.chromium.org/746663002/diff/180001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right):

https://codereview.chromium.org/746663002/diff/180001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:66:
GURL(chrome::kChromeUIWebRTCDeviceProviderURL),
Let's make this a local constant.

[SeRya, 2014-11-27 16:53:27]

Patchset #10 (id:200001) has been deleted

[SeRya, 2014-11-27 17:23:01]

https://codereview.chromium.org/746663002/diff/180001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right):

https://codereview.chromium.org/746663002/diff/180001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:66:
GURL(chrome::kChromeUIWebRTCDeviceProviderURL),
On 2014/11/27 14:50:03, dgozman wrote:
> Let's make this a local constant.

Done.

[SeRya, 2014-11-27 17:46:03]

serya@chromium.org changed reviewers:
+ jhawkins@chromium.org

[SeRya, 2014-11-27 17:46:04]

+jhawkins@ for:
chrome/BUILD.gn
chrome/browser/devtools/BUILD.gn
chrome/browser/devtools/webrtc_device_provider_resources.gyp
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
chrome/chrome_resources.gyp

[dgozman, 2014-11-28 12:17:24]

https://codereview.chromium.org/746663002/diff/220001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right):

https://codereview.chromium.org/746663002/diff/220001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:122: if
(handler_ != NULL)
I think |handler_| must be destroyed as a part of |background_worker_|, and this
is unnecessary.

[SeRya, 2014-11-28 12:31:18]

https://codereview.chromium.org/746663002/diff/220001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right):

https://codereview.chromium.org/746663002/diff/220001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:122: if
(handler_ != NULL)
On 2014/11/28 12:17:24, dgozman wrote:
> I think |handler_| must be destroyed as a part of |background_worker_|, and
this
> is unnecessary.

Done.

[dgozman, 2014-11-28 12:40:34]

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right):

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:21: 
nit: extra empty line.

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:28: void
HandleLoaded(const base::ListValue* args);
If you just add a check |if (!owner_) return;| to all handlers, you will not
need 2 methods per each message and 2 classes.

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:56: // Sets a
stub message handler. If web contents load from WebRTCDeviceProvider
nit: load from -> was created by

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:63:
WebRTCDeviceProvider::WebUI::~WebUI() {}
nit: } on a separate line

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:82:
owner_->handler_ = this;
This is unnecessary.

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:100:
DCHECK_EQ(chrome::kChromeUIWebRTCDeviceProviderHost, url.host());
Nice one!

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.h (right):

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:19: class
MessageHandler;
Let's move this to private.

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:33: friend class
WebRTCDeviceProvider;
I think we can drop this.

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:49: friend class
MessageHandlerImpl;
ditto.

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:54:
MessageHandlerImpl* handler_;
There is no need to have this pointer.

[SeRya, 2014-11-28 12:48:02]

Patchset #12 (id:260001) has been deleted

[SeRya, 2014-11-28 12:50:32]

Patchset #12 (id:280001) has been deleted

[SeRya, 2014-11-28 12:53:05]

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right):

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:21: 
On 2014/11/28 12:40:34, dgozman wrote:
> nit: extra empty line.

Done.

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:28: void
HandleLoaded(const base::ListValue* args);
On 2014/11/28 12:40:34, dgozman wrote:
> If you just add a check |if (!owner_) return;| to all handlers, you will not
> need 2 methods per each message and 2 classes.

Done.

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:56: // Sets a
stub message handler. If web contents load from WebRTCDeviceProvider
On 2014/11/28 12:40:34, dgozman wrote:
> nit: load from -> was created by

Done.

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:63:
WebRTCDeviceProvider::WebUI::~WebUI() {}
On 2014/11/28 12:40:34, dgozman wrote:
> nit: } on a separate line

Done.

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:82:
owner_->handler_ = this;
On 2014/11/28 12:40:34, dgozman wrote:
> This is unnecessary.

Done.

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.h (right):

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:19: class
MessageHandler;
On 2014/11/28 12:40:34, dgozman wrote:
> Let's move this to private.

Done.

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:33: friend class
WebRTCDeviceProvider;
On 2014/11/28 12:40:34, dgozman wrote:
> I think we can drop this.

Done.

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:49: friend class
MessageHandlerImpl;
On 2014/11/28 12:40:34, dgozman wrote:
> ditto.

Done.

https://codereview.chromium.org/746663002/diff/240001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:54:
MessageHandlerImpl* handler_;
On 2014/11/28 12:40:34, dgozman wrote:
> There is no need to have this pointer.

Done.

[dgozman, 2014-11-28 12:58:07]

lgtm

https://codereview.chromium.org/746663002/diff/300001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right):

https://codereview.chromium.org/746663002/diff/300001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:47:
web_ui->AddMessageHandler(new MessageHandler(NULL));
nit: nullptr

https://codereview.chromium.org/746663002/diff/300001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.h (right):

https://codereview.chromium.org/746663002/diff/300001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:33: void
QueryDevices(const SerialsCallback& callback) override;
nit: we usually mark override sections like this:
// AndroidDeviceManager::DeviceProvider implementation.

https://codereview.chromium.org/746663002/diff/300001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:45:
~WebRTCDeviceProvider();
override

[SeRya, 2014-11-28 13:07:17]

https://codereview.chromium.org/746663002/diff/300001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right):

https://codereview.chromium.org/746663002/diff/300001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:47:
web_ui->AddMessageHandler(new MessageHandler(NULL));
On 2014/11/28 12:58:06, dgozman wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/746663002/diff/300001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.h (right):

https://codereview.chromium.org/746663002/diff/300001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:33: void
QueryDevices(const SerialsCallback& callback) override;
On 2014/11/28 12:58:06, dgozman wrote:
> nit: we usually mark override sections like this:
> // AndroidDeviceManager::DeviceProvider implementation.

Done.

https://codereview.chromium.org/746663002/diff/300001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:45:
~WebRTCDeviceProvider();
On 2014/11/28 12:58:06, dgozman wrote:
> override

Done.

[pfeldman, 2014-11-28 15:52:18]

https://codereview.chromium.org/746663002/diff/320001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right):

https://codereview.chromium.org/746663002/diff/320001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:18: class
WebRTCDeviceProvider::MessageHandler
anonymous namespace for all of the implementation detail classes please.

https://codereview.chromium.org/746663002/diff/320001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:44: // Sets a
stub message handler. If web contents was created by
Why is this necessary?

https://codereview.chromium.org/746663002/diff/320001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:73: //
WebRTCDeviceProvider
See devtools_window.cc for class separator comments.

https://codereview.chromium.org/746663002/diff/320001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.h (right):

https://codereview.chromium.org/746663002/diff/320001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:25: class WebUI :
public content::WebUIController {
This should be a private detail of the provider.

https://codereview.chromium.org/746663002/diff/320001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:25: class WebUI :
public content::WebUIController {
Seems like it should be a sibling class.

https://codereview.chromium.org/746663002/diff/320001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:44: class
MessageHandler;
Why is it defined here?

[pfeldman, 2014-11-28 16:16:09]

lgtm

[SeRya, 2014-11-28 16:18:32]

https://codereview.chromium.org/746663002/diff/320001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc (right):

https://codereview.chromium.org/746663002/diff/320001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:18: class
WebRTCDeviceProvider::MessageHandler
On 2014/11/28 15:52:17, pfeldman wrote:
> anonymous namespace for all of the implementation detail classes please.

Done.

https://codereview.chromium.org/746663002/diff/320001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:44: // Sets a
stub message handler. If web contents was created by
On 2014/11/28 15:52:17, pfeldman wrote:
> Why is this necessary?

If this page is loaded not from WebRTCDeviceProvider (byped by user or from a
browser test) any attempt to send a message (like 'loaded') causes failure in
debug build on "unhandled message";

https://codereview.chromium.org/746663002/diff/320001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:73: //
WebRTCDeviceProvider
On 2014/11/28 15:52:18, pfeldman wrote:
> See devtools_window.cc for class separator comments.

Done.

https://codereview.chromium.org/746663002/diff/320001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider.h (right):

https://codereview.chromium.org/746663002/diff/320001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.h:44: class
MessageHandler;
On 2014/11/28 15:52:18, pfeldman wrote:
> Why is it defined here?

Removed.

[SeRya, 2014-12-02 13:17:40]

serya@chromium.org changed reviewers:
- jhawkins@chromium.org

[SeRya, 2014-12-02 13:24:36]

serya@chromium.org changed reviewers:
+ cpu@chromium.org

[SeRya, 2014-12-02 13:24:38]

-jhawkins@ (on vacation).
+cpu@

[cpu_(ooo_6.6-7.5), 2014-12-03 22:58:48]

lgtm but I don't see Tom's so wait for him.

[Tom Sepez, 2014-12-03 23:01:24]

lgtm

[SeRya, 2014-12-03 23:27:45]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-12-03 23:29:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/746663002/340001

[commit-bot: I haz the power, 2014-12-03 23:34:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-03 23:34:23]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/89587)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[SeRya, 2014-12-04 03:03:34]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-12-04 03:04:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/746663002/360001

[commit-bot: I haz the power, 2014-12-04 03:06:46]

Committed patchset #15 (id:360001)

[commit-bot: I haz the power, 2014-12-04 03:07:30]

Patchset 15 (id:??) landed as
https://crrev.com/91af3ad699d1067d8398b42a6f1fad737b95b12b
Cr-Commit-Position: refs/heads/master@{#306755}