DevTools: discover default port node servers for debugging (behind experiment)

DevTools: discover default port node servers for debugging (behind experiment)

Committed: https://crrev.com/13e84ff7d78fda8b69372cc68e957fd4df4bbaae
Cr-Commit-Position: refs/heads/master@{#420754}

[pfeldman, 2016-09-23 17:40:53]

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

[pfeldman, 2016-09-23 17:41:22]

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

[pfeldman, 2016-09-23 20:27:30]

The CQ bit was checked by pfeldman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 20:27:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgozman, 2016-09-23 20:41:48]

https://codereview.chromium.org/2370483002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2370483002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:57:
DevToolsAgentHost::List result = DevToolsAgentHost::GetOrCreateAll();
Why add all of these?

https://codereview.chromium.org/2370483002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:67: bool
ChromeDevToolsManagerDelegate::DiscoverTargets(
This method is never called.

https://codereview.chromium.org/2370483002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:70: device_manager_
= AndroidDeviceManager::Create();
I think we should cleanup on disable/detach.

https://codereview.chromium.org/2370483002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:95: namespace
browser = ::chrome::devtools::Browser;
There is a single usage, can inline.

https://codereview.chromium.org/2370483002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:210:
static_cast<base::DictionaryValue*>(item.get());
There should be a method which does cast for you.

[pfeldman, 2016-09-23 20:52:48]

https://codereview.chromium.org/2370483002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2370483002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:57:
DevToolsAgentHost::List result = DevToolsAgentHost::GetOrCreateAll();
What do you mean? Otherwise they won't be reported.

https://codereview.chromium.org/2370483002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:67: bool
ChromeDevToolsManagerDelegate::DiscoverTargets(
On 2016/09/23 20:41:48, dgozman wrote:
> This method is never called.

Surely it is called - it is implementation of the manager delegate.

https://codereview.chromium.org/2370483002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:70: device_manager_
= AndroidDeviceManager::Create();
On 2016/09/23 20:41:48, dgozman wrote:
> I think we should cleanup on disable/detach.

There are no means of doing it at this point, so it is leaky.

https://codereview.chromium.org/2370483002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:95: namespace
browser = ::chrome::devtools::Browser;
On 2016/09/23 20:41:48, dgozman wrote:
> There is a single usage, can inline.

Done.

https://codereview.chromium.org/2370483002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:210:
static_cast<base::DictionaryValue*>(item.get());
On 2016/09/23 20:41:48, dgozman wrote:
> There should be a method which does cast for you.

Not without side-effects.

[dgozman, 2016-09-23 20:57:22]

lgtm

https://codereview.chromium.org/2370483002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2370483002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:67: bool
ChromeDevToolsManagerDelegate::DiscoverTargets(
On 2016/09/23 20:52:48, pfeldman wrote:
> On 2016/09/23 20:41:48, dgozman wrote:
> > This method is never called.
> 
> Surely it is called - it is implementation of the manager delegate.

My bad :-)

https://codereview.chromium.org/2370483002/diff/40001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2370483002/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:82: return true;
Let's skip all the work and callback synchronously when tcp_locations_ is empty.

[pfeldman, 2016-09-23 21:05:21]

https://codereview.chromium.org/2370483002/diff/40001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2370483002/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:82: return true;
On 2016/09/23 20:57:22, dgozman wrote:
> Let's skip all the work and callback synchronously when tcp_locations_ is
empty.

Done.

[pfeldman, 2016-09-23 21:05:25]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-09-23 21:05:25]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2370483002/#ps60001
(title: "for landing")

[commit-bot: I haz the power, 2016-09-23 21:05:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 22:45:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-23 22:50:47]

Description was changed from

==========
DevTools: discover default port node servers for debugging (behind experiment)
==========

to

==========
DevTools: discover default port node servers for debugging (behind experiment)

Committed: https://crrev.com/13e84ff7d78fda8b69372cc68e957fd4df4bbaae
Cr-Commit-Position: refs/heads/master@{#420754}
==========

[commit-bot: I haz the power, 2016-09-23 22:50:49]

Patchset 4 (id:??) landed as
https://crrev.com/13e84ff7d78fda8b69372cc68e957fd4df4bbaae
Cr-Commit-Position: refs/heads/master@{#420754}