[DevTools] A setting for TCP discovery enablement

[DevTools] A setting for TCP discovery enablement

This change introduces a preference that toggles TCP debug targets
discovery enablement/disablement. This preference works together with
the TCP discovery config preference introduced earlier.

By default, TCP discovery is disabled. Once toggled, it will scan
targets specified by the config preference. Default setting is to scan
localhost:9222 and localhost:9229.

R=dgozman@chromium.org, pfeldman@chromium.org
BUG=636084
Committed: https://crrev.com/6380c181f2dd0111c810da675ecce705701de822
Cr-Commit-Position: refs/heads/master@{#415090}

[eostroukhov, 2016-08-26 23:18:16]

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

[commit-bot: I haz the power, 2016-08-26 23:19:05]

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

[eostroukhov, 2016-08-26 23:20:06]

Please take a look. This preference is not yet exposed from UI, that will come
in a separate CL.

[dgozman, 2016-08-26 23:31:43]

https://codereview.chromium.org/2284083002/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/device/devtools_android_bridge.cc (right):

https://codereview.chromium.org/2284083002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/device/devtools_android_bridge.cc:738:
target_discovery->AppendString("localhost:9222");
Named constants at the top of the file, please.

https://codereview.chromium.org/2284083002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/device/devtools_android_bridge.cc:742:
target_discovery);
style: 4 spaces indent

https://codereview.chromium.org/2284083002/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc
(right):

https://codereview.chromium.org/2284083002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc:40:
std::array<std::string, sizeof...(values)> array_from_tuple(
Let's use std::vector<>, and push items there manually.

https://codereview.chromium.org/2284083002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc:55: bool
expect_all_tcp_targets(const std::tuple<Strings ...>& expected,
Can we just sort two arrays and compare?

https://codereview.chromium.org/2284083002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc:64:
fprintf(stderr, "[%s] Unexpected: %s\n", __FUNCTION__, value.c_str());
EXPECT_EQ(vector1, vector2) << "Actual targets: " << vector2

https://codereview.chromium.org/2284083002/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2284083002/diff/1/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1538: // debugging targets
Full stop please.

[commit-bot: I haz the power, 2016-08-27 00:09:53]

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

[commit-bot: I haz the power, 2016-08-27 00:09:54]

Dry run: This issue passed the CQ dry run.

[eostroukhov, 2016-08-29 17:43:32]

Addressed the comments

[eostroukhov, 2016-08-29 17:43:34]

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

[commit-bot: I haz the power, 2016-08-29 17:52:13]

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

[eostroukhov, 2016-08-29 22:05:24]

Renamed the preference for consistency with USB discovery

[eostroukhov, 2016-08-29 22:05:26]

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

[eostroukhov, 2016-08-29 22:06:31]

Thank you for the review. I addressed the comments and uploaded a new version.
Please take another look.

https://codereview.chromium.org/2284083002/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/device/devtools_android_bridge.cc (right):

https://codereview.chromium.org/2284083002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/device/devtools_android_bridge.cc:738:
target_discovery->AppendString("localhost:9222");
On 2016/08/26 23:31:43, dgozman wrote:
> Named constants at the top of the file, please.

Done.

https://codereview.chromium.org/2284083002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/device/devtools_android_bridge.cc:742:
target_discovery);
On 2016/08/26 23:31:43, dgozman wrote:
> style: 4 spaces indent

Done.

https://codereview.chromium.org/2284083002/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc
(right):

https://codereview.chromium.org/2284083002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc:40:
std::array<std::string, sizeof...(values)> array_from_tuple(
On 2016/08/26 23:31:43, dgozman wrote:
> Let's use std::vector<>, and push items there manually.

I made if even more boring then you suggested...

https://codereview.chromium.org/2284083002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc:55: bool
expect_all_tcp_targets(const std::tuple<Strings ...>& expected,
On 2016/08/26 23:31:43, dgozman wrote:
> Can we just sort two arrays and compare?

Acknowledged.

https://codereview.chromium.org/2284083002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc:64:
fprintf(stderr, "[%s] Unexpected: %s\n", __FUNCTION__, value.c_str());
On 2016/08/26 23:31:43, dgozman wrote:
> EXPECT_EQ(vector1, vector2) << "Actual targets: " << vector2

Acknowledged.

https://codereview.chromium.org/2284083002/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2284083002/diff/1/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1538: // debugging targets
On 2016/08/26 23:31:43, dgozman wrote:
> Full stop please.

Done.

[dgozman, 2016-08-29 22:28:21]

lgtm

https://codereview.chromium.org/2284083002/diff/40001/chrome/browser/devtools...
File chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc
(right):

https://codereview.chromium.org/2284083002/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc:29: static
std::string all_targets_string(
AllTargetsString

https://codereview.chromium.org/2284083002/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc:107:
std::array<std::string, 2> EXPECTED_TARGETS =
expected_targets

https://codereview.chromium.org/2284083002/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc:122:
EXPECT_EQ(3, matched_mask);
I still don't understand why not just sort them?

[commit-bot: I haz the power, 2016-08-29 22:55:09]

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

[eostroukhov, 2016-08-29 23:17:57]

Comments

[eostroukhov, 2016-08-29 23:17:59]

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

[eostroukhov, 2016-08-29 23:19:36]

Thank you for the review

https://codereview.chromium.org/2284083002/diff/40001/chrome/browser/devtools...
File chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc
(right):

https://codereview.chromium.org/2284083002/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc:29: static
std::string all_targets_string(
On 2016/08/29 22:28:21, dgozman wrote:
> AllTargetsString

Done.

https://codereview.chromium.org/2284083002/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc:107:
std::array<std::string, 2> EXPECTED_TARGETS =
On 2016/08/29 22:28:21, dgozman wrote:
> expected_targets

Done (by removing this line - no longer needed).

https://codereview.chromium.org/2284083002/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/device/devtools_android_bridge_browsertest.cc:122:
EXPECT_EQ(3, matched_mask);
On 2016/08/29 22:28:21, dgozman wrote:
> I still don't understand why not just sort them?

At some point I was outputting indexes... Fixed.

[eostroukhov, 2016-08-29 23:19:46]

The CQ bit was unchecked by eostroukhov@chromium.org

[eostroukhov, 2016-08-29 23:19:49]

The CQ bit was checked by eostroukhov@chromium.org

[eostroukhov, 2016-08-29 23:19:50]

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/2284083002/#ps60001
(title: "Comments")

[commit-bot: I haz the power, 2016-08-29 23:55:18]

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-08-30 04:24:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-30 04:25:48]

Description was changed from

==========
[DevTools] A setting for TCP discovery enablement

This change introduces a preference that toggles TCP debug targets
discovery enablement/disablement. This preference works together with
the TCP discovery config preference introduced earlier.

By default, TCP discovery is disabled. Once toggled, it will scan
targets specified by the config preference. Default setting is to scan
localhost:9222 and localhost:9229.

R=dgozman@chromium.org, pfeldman@chromium.org
BUG=636084
==========

to

==========
[DevTools] A setting for TCP discovery enablement

This change introduces a preference that toggles TCP debug targets
discovery enablement/disablement. This preference works together with
the TCP discovery config preference introduced earlier.

By default, TCP discovery is disabled. Once toggled, it will scan
targets specified by the config preference. Default setting is to scan
localhost:9222 and localhost:9229.

R=dgozman@chromium.org, pfeldman@chromium.org
BUG=636084

Committed: https://crrev.com/6380c181f2dd0111c810da675ecce705701de822
Cr-Commit-Position: refs/heads/master@{#415090}
==========

[commit-bot: I haz the power, 2016-08-30 04:25:49]

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