DevTools: allow to pass list of remote debugging targets as command line argument

DevTools: allow to pass list of remote debugging targets as command line argument

This CL introduces --remote-debugging-targets flag that allows to pass list of <ip address>:<port> pairs which will be polled by chrome:inspect page to discover devtools targets that can be inspected remotely with Chrome DevTools.

As discussed before, serial parameter is reused for the remote target IP address.

BUG=435243
Committed: https://crrev.com/4e413a1094ee04c1c56241fb020ecde8963c1bea
Cr-Commit-Position: refs/heads/master@{#337578}

[yurys, 2015-07-06 08:56:25]

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

[yurys, 2015-07-06 09:30:33]

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

[commit-bot: I haz the power, 2015-07-06 09:30:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224743002/20001

[dgozman, 2015-07-06 10:08:34]

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

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge.cc:917: if
(command_line->HasSwitch(switches::kRemoteDebuggingTargets)) {
Could you please extract a method?

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/port_forwarding_browsertest.cc (right):

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/port_forwarding_browsertest.cc:86:
targets.insert(net::HostPortPair("127.0.0.1", kDefaultDebuggingPort));
Can we make a static method in SelfAsDeviceProvider to avoid "127.0.0.1" all
over the code?

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/self_device_provider.cc (right):

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.cc:18: const char
kDeviceModel[] = "Local Chrome";
It's not Chrome anymore, let's update.

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.cc:19: const char
kBrowserName[] = "Chrome";
ditto

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.cc:38: if
(std::find(result.begin(), result.end(), host) != result.end()) {
nit: we don't usually wrap one-liners with {}

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.cc:75: LOG(ERROR) << "Failed
to parse IP address: " << serial;
We should |callback(nullptr)| probably.

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/self_device_provider.h (right):

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.h:14: class
SelfAsDeviceProvider : public AndroidDeviceManager::DeviceProvider {
This is not a "SelfAsDeviceProvider" anymore, let's rename to something like
TCPDeviceProvider.

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.h:16: using RemoteTargets =
std::set<net::HostPortPair>;
Using RemoteTargets for this if confusing, as we have very specific meaning for
"target" in the code around. Let's rename to Hosts or HostPortSet.

https://codereview.chromium.org/1224743002/diff/20001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1224743002/diff/20001/content/public/common/c...
content/public/common/content_switches.cc:695: // The format is <ip
address>:<port>,...,<ip address>:port
nit: full stop please.

[pfeldman, 2015-07-06 10:10:05]

lgtm

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

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge.cc:939: if
(!targets.empty()) {
s/{}//

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/self_device_provider.cc (right):

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.cc:53: if (serial !=
target.host()) {
ditto

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.cc:74: if
(!net::ParseIPLiteralToNumber(serial, &ip_number)) {
ditto

[commit-bot: I haz the power, 2015-07-06 11:25:28]

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

[commit-bot: I haz the power, 2015-07-06 11:25:29]

Dry run: This issue passed the CQ dry run.

[yurys, 2015-07-06 12:49:01]

Added host->ip resolution. PTAL

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

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge.cc:939: if
(!targets.empty()) {
On 2015/07/06 10:10:04, pfeldman wrote:
> s/{}//

Done.

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/self_device_provider.cc (right):

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.cc:18: const char
kDeviceModel[] = "Local Chrome";
On 2015/07/06 10:08:34, dgozman wrote:
> It's not Chrome anymore, let's update.

Done.

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.cc:19: const char
kBrowserName[] = "Chrome";
On 2015/07/06 10:08:34, dgozman wrote:
> ditto

Done.

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.cc:38: if
(std::find(result.begin(), result.end(), host) != result.end()) {
On 2015/07/06 10:08:34, dgozman wrote:
> nit: we don't usually wrap one-liners with {}

Done.

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.cc:53: if (serial !=
target.host()) {
On 2015/07/06 10:10:04, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.cc:74: if
(!net::ParseIPLiteralToNumber(serial, &ip_number)) {
On 2015/07/06 10:10:04, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.cc:75: LOG(ERROR) << "Failed
to parse IP address: " << serial;
On 2015/07/06 10:08:34, dgozman wrote:
> We should |callback(nullptr)| probably.

Done.

[yurys, 2015-07-06 13:57:07]

PTAL

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

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge.cc:917: if
(command_line->HasSwitch(switches::kRemoteDebuggingTargets)) {
On 2015/07/06 10:08:33, dgozman wrote:
> Could you please extract a method?

Done.

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/port_forwarding_browsertest.cc (right):

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/port_forwarding_browsertest.cc:86:
targets.insert(net::HostPortPair("127.0.0.1", kDefaultDebuggingPort));
On 2015/07/06 10:08:34, dgozman wrote:
> Can we make a static method in SelfAsDeviceProvider to avoid "127.0.0.1" all
> over the code?

Done.

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/self_device_provider.h (right):

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.h:14: class
SelfAsDeviceProvider : public AndroidDeviceManager::DeviceProvider {
On 2015/07/06 10:08:34, dgozman wrote:
> This is not a "SelfAsDeviceProvider" anymore, let's rename to something like
> TCPDeviceProvider.

Done.

https://codereview.chromium.org/1224743002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/self_device_provider.h:16: using RemoteTargets =
std::set<net::HostPortPair>;
On 2015/07/06 10:08:34, dgozman wrote:
> Using RemoteTargets for this if confusing, as we have very specific meaning
for
> "target" in the code around. Let's rename to Hosts or HostPortSet.

Done.

https://codereview.chromium.org/1224743002/diff/20001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1224743002/diff/20001/content/public/common/c...
content/public/common/content_switches.cc:695: // The format is <ip
address>:<port>,...,<ip address>:port
On 2015/07/06 10:08:34, dgozman wrote:
> nit: full stop please.

Done.

[yurys, 2015-07-06 13:57:15]

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

[yurys, 2015-07-06 13:57:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1224743002/#ps60001
(title: "Renamed SelfAsDeviceProvider to TCPDeviceProvider")

[commit-bot: I haz the power, 2015-07-06 13:57:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224743002/60001

[dgozman, 2015-07-06 14:10:56]

lgtm

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

https://codereview.chromium.org/1224743002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge.cc:946: if
(scoped_refptr<TCPDeviceProvider> provider = CreateTCPDeviceProvider()) {
nit: no {}

https://codereview.chromium.org/1224743002/diff/60001/chrome/browser/devtools...
File chrome/browser/devtools/device/tcp_device_provider.cc (right):

https://codereview.chromium.org/1224743002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/device/tcp_device_provider.cc:24: static void
RunSocketCallback(
inline it

[yurys, 2015-07-06 14:14:52]

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

https://codereview.chromium.org/1224743002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge.cc:946: if
(scoped_refptr<TCPDeviceProvider> provider = CreateTCPDeviceProvider()) {
On 2015/07/06 14:10:55, dgozman wrote:
> nit: no {}

Done.

https://codereview.chromium.org/1224743002/diff/60001/chrome/browser/devtools...
File chrome/browser/devtools/device/tcp_device_provider.cc (right):

https://codereview.chromium.org/1224743002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/device/tcp_device_provider.cc:24: static void
RunSocketCallback(
On 2015/07/06 14:10:55, dgozman wrote:
> inline it

I cannot inline this as it is called asynchronously in socket->Connect

[yurys, 2015-07-06 14:15:46]

yurys@chromium.org changed reviewers:
+ jochen@chromium.org

[yurys, 2015-07-06 14:15:47]

@jochen: please review changes in  content/public/common/content_switches.*

[commit-bot: I haz the power, 2015-07-06 15:52:27]

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

[commit-bot: I haz the power, 2015-07-06 15:52:28]

Dry run: This issue passed the CQ dry run.

[yurys, 2015-07-07 06:12:10]

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

[yurys, 2015-07-07 06:12:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1224743002/#ps80001
(title: "Removed {}")

[commit-bot: I haz the power, 2015-07-07 06:12:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224743002/80001

[commit-bot: I haz the power, 2015-07-07 06:27:53]

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

[commit-bot: I haz the power, 2015-07-07 06:27:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

(exceeded global retry quota)

[jochen (gone - plz use gerrit), 2015-07-07 09:16:06]

that should be a chrome switch, no?

[yurys, 2015-07-07 09:24:35]

On 2015/07/07 09:16:06, jochen wrote:
> that should be a chrome switch, no?

You are right! Thank you!

[yurys, 2015-07-07 09:24:44]

The CQ bit was checked by yurys@chromium.org

[yurys, 2015-07-07 09:24:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1224743002/#ps120001
(title: "Moved flag definition to chrome_switches")

[commit-bot: I haz the power, 2015-07-07 09:24:52]

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

[commit-bot: I haz the power, 2015-07-07 10:15:05]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-07 10:15:50]

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