Enabling WebRTCDeviceProvider under enable-devtools-experiments flag

Enabling WebRTCDeviceProvider under enable-devtools-experiments flag

BUG=383418
Committed: https://crrev.com/0a02e5a9d355354ec8c588bd1ab4c3f1c16aa521
Cr-Commit-Position: refs/heads/master@{#307240}

[SeRya, 2014-12-05 12:24:58]

Patchset #1 (id:1) has been deleted

[SeRya, 2014-12-05 12:25:05]

Patchset #1 (id:20001) has been deleted

[SeRya, 2014-12-05 13:24:00]

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

[SeRya, 2014-12-05 13:24:01]

PTAL

[SeRya, 2014-12-05 18:10:27]

Patchset #3 (id:80001) has been deleted

[SeRya, 2014-12-05 18:12:10]

Object composition as we discussed.

[dgozman, 2014-12-06 13:06:34]

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
File chrome/browser/devtools/device/devtools_android_bridge.cc (right):

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge.cc:264:
DependsOn(ProfileOAuth2TokenServiceFactory::GetInstance());
Can we guard this with experiment as well?

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge.cc:272: Profile* profile
= static_cast<Profile*>(context);
Profile::FromBrowserContext

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

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:31: using
content::WebUIMessageHandler;
Using so much content classes may be confusing.

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:42: private
NotificationObserver,
Why use private inheritance?

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:58: //
implementation of IdentityProvider::Observer.
nit (here and below): Implementation

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:115:
WebRTCDeviceProvider::DevToolsBridgeClient::Create(
This method seems to be called from handler thread. How does it create client
which lives on UI?

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:123: :
profile_(profile),
nit: indentation

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:139:
identity_provider_->RemoveObserver(this);
Is this provider still alive when NOTIFICATION_PROFILE_DESTROYED is fired?

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:142: void
WebRTCDeviceProvider::DevToolsBridgeClient::DeleteSelf() {
Can we have 2 tests, which examine "DeleteSelf, then ProfileDestoryed" and
"ProfileDestroyed, then DeleteSelf"?

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:188:
WebRTCDeviceProvider::MessageHandler::MessageHandler(
Why has this changed to an inner class?

[SeRya, 2014-12-08 07:44:32]

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
File chrome/browser/devtools/device/devtools_android_bridge.cc (right):

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge.cc:264:
DependsOn(ProfileOAuth2TokenServiceFactory::GetInstance());
On 2014/12/06 13:06:33, dgozman wrote:
> Can we guard this with experiment as well?

Done.

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge.cc:272: Profile* profile
= static_cast<Profile*>(context);
On 2014/12/06 13:06:33, dgozman wrote:
> Profile::FromBrowserContext

Done.

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

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:31: using
content::WebUIMessageHandler;
On 2014/12/06 13:06:33, dgozman wrote:
> Using so much content classes may be confusing.

So what you suggest? "using namespace content;"?

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:42: private
NotificationObserver,
On 2014/12/06 13:06:33, dgozman wrote:
> Why use private inheritance?

Like with private members: if there is no need to be public it should be
private.

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:58: //
implementation of IdentityProvider::Observer.
On 2014/12/06 13:06:33, dgozman wrote:
> nit (here and below): Implementation

Done.

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:115:
WebRTCDeviceProvider::DevToolsBridgeClient::Create(
On 2014/12/06 13:06:33, dgozman wrote:
> This method seems to be called from handler thread. How does it create client
> which lives on UI?

All providers are created on the UI thread from DevToolsAndroidBridge
constructor.

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:123: :
profile_(profile),
On 2014/12/06 13:06:33, dgozman wrote:
> nit: indentation

Done.

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:139:
identity_provider_->RemoveObserver(this);
On 2014/12/06 13:06:33, dgozman wrote:
> Is this provider still alive when NOTIFICATION_PROFILE_DESTROYED is fired?

It is. And it's why NOTIFICATION_PROFILE_DESTROYED needed even if would
destroyed synchronously on the UI thread.

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:142: void
WebRTCDeviceProvider::DevToolsBridgeClient::DeleteSelf() {
On 2014/12/06 13:06:33, dgozman wrote:
> Can we have 2 tests, which examine "DeleteSelf, then ProfileDestoryed" and
> "ProfileDestroyed, then DeleteSelf"?

Done.

https://codereview.chromium.org/762903004/diff/100001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:188:
WebRTCDeviceProvider::MessageHandler::MessageHandler(
On 2014/12/06 13:06:33, dgozman wrote:
> Why has this changed to an inner class?

To have access to the private member DevToolsBridgeClient

[dgozman, 2014-12-08 12:22:43]

lgtm

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

https://codereview.chromium.org/762903004/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:17: #include
"google_apis/gaia/identity_provider.h"
nit: let's remove this.

https://codereview.chromium.org/762903004/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:28: using
content::Source;
Let's drop this one.

https://codereview.chromium.org/762903004/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:133:
identity_provider_(signin_manager, token_service, NULL),
nit: nullptr

https://codereview.chromium.org/762903004/diff/120001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider_browsertest.cc
(right):

https://codereview.chromium.org/762903004/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider_browsertest.cc:53:
runner));
nit: if you use PostTaskAndReply with runner->QuitClosure callback, the code
will be simpler.

[SeRya, 2014-12-08 13:02:26]

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

https://codereview.chromium.org/762903004/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:17: #include
"google_apis/gaia/identity_provider.h"
On 2014/12/08 12:22:43, dgozman wrote:
> nit: let's remove this.

Done.

https://codereview.chromium.org/762903004/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:28: using
content::Source;
On 2014/12/08 12:22:43, dgozman wrote:
> Let's drop this one.

Done.

https://codereview.chromium.org/762903004/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider.cc:133:
identity_provider_(signin_manager, token_service, NULL),
On 2014/12/08 12:22:43, dgozman wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/762903004/diff/120001/chrome/browser/devtools...
File chrome/browser/devtools/device/webrtc/webrtc_device_provider_browsertest.cc
(right):

https://codereview.chromium.org/762903004/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/device/webrtc/webrtc_device_provider_browsertest.cc:53:
runner));
On 2014/12/08 12:22:43, dgozman wrote:
> nit: if you use PostTaskAndReply with runner->QuitClosure callback, the code
> will be simpler.

Done.

[SeRya, 2014-12-08 13:02:32]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-12-08 13:02:59]

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

[commit-bot: I haz the power, 2014-12-08 14:04:19]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2014-12-08 14:05:54]

Patchset 5 (id:??) landed as
https://crrev.com/0a02e5a9d355354ec8c588bd1ab4c3f1c16aa521
Cr-Commit-Position: refs/heads/master@{#307240}