[CrOS Tether] Add tether settings section to the MD settings page.

[CrOS Tether] Add tether settings section to the MD settings page.

Currently, tether settings appear as part of their own subpage. In the future, these settings will be merged with cellular networks under the "Mobile" subpage.

BUG=672263
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2838323002
Cr-Commit-Position: refs/heads/master@{#467898}
Committed: https://chromium.googlesource.com/chromium/src/+/3643dc94bde2e194b0ce548ad4235ccad5bf06b1

[Kyle Horimoto, 2017-04-26 01:47:10]

Description was changed from

==========
[CrOS Tether] Add tether settings section to the MD settings page.

Currently, tether settings appear as part of their own subpage. In the future,
these settings will be merged with cellular networks under the "Mobile" subpage.

BUG=672263
==========

to

==========
[CrOS Tether] Add tether settings section to the MD settings page.

Currently, tether settings appear as part of their own subpage. In the future,
these settings will be merged with cellular networks under the "Mobile" subpage.

BUG=672263
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Kyle Horimoto, 2017-04-26 01:49:16]

khorimoto@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Dan Beam, 2017-04-26 02:00:46]

do you have screenshots of this UI?

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_property_list.js
(right):

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_property_list.js:167: if
(this.tetherPropertyRegex_.test(key))
why not: key.startsWith('Tether.')?

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_property_list.js:167: if
(this.tetherPropertyRegex_.test(key))
needs curlies

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_property_list.js:189: if
(key == 'Tether.BatteryPercentage') {
no curlies

[Kyle Horimoto, 2017-04-26 17:05:05]

Screenshots here: https://goo.gl/photos/kPLYKQCMXk5qAfocA

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_property_list.js
(right):

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_property_list.js:167: if
(this.tetherPropertyRegex_.test(key))
On 2017/04/26 02:00:46, Dan Beam wrote:
> why not: key.startsWith('Tether.')?

Done.

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_property_list.js:167: if
(this.tetherPropertyRegex_.test(key))
On 2017/04/26 02:00:46, Dan Beam wrote:
> needs curlies

Done.

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_property_list.js:189: if
(key == 'Tether.BatteryPercentage') {
On 2017/04/26 02:00:46, Dan Beam wrote:
> no curlies

Done.

[stevenjb, 2017-04-26 17:05:08]

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_property_list.js
(right):

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_property_list.js:169:
key, /** @type {string|number} */ (value));
Alternately, how about:

valueStr = this.getCustomPropertyVale_(key, value);
if (valueStr)
  return valueStr;

Then you don't need an extra regex or prefix check (neither of which are likely
to improve performance any).

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_property_list.js:209: //
not available. See host_scanner.cc.
Not sure this comment is really helpful, that seems pretty clear from the code
and 'unknown-carrier' is unique enough to search for in the code.

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_summary.js (right):

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_summary.js:324:
CrOnc.Type.ETHERNET, CrOnc.Type.TETHER, CrOnc.Type.WI_FI,
Let's place TETHER after CELLULAR for now.

[Kyle Horimoto, 2017-04-26 17:17:41]

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_property_list.js
(right):

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_property_list.js:169:
key, /** @type {string|number} */ (value));
On 2017/04/26 17:05:08, stevenjb wrote:
> Alternately, how about:
> 
> valueStr = this.getCustomPropertyVale_(key, value);
> if (valueStr)
>   return valueStr;
> 
> Then you don't need an extra regex or prefix check (neither of which are
likely
> to improve performance any).

Done.

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_property_list.js:209: //
not available. See host_scanner.cc.
On 2017/04/26 17:05:08, stevenjb wrote:
> Not sure this comment is really helpful, that seems pretty clear from the code
> and 'unknown-carrier' is unique enough to search for in the code.

Done.

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_summary.js (right):

https://codereview.chromium.org/2838323002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_summary.js:324:
CrOnc.Type.ETHERNET, CrOnc.Type.TETHER, CrOnc.Type.WI_FI,
On 2017/04/26 17:05:08, stevenjb wrote:
> Let's place TETHER after CELLULAR for now.

Done.

[stevenjb, 2017-04-26 17:26:56]

lgtm w/ nits

https://codereview.chromium.org/2838323002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_property_list.js
(right):

https://codereview.chromium.org/2838323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_property_list.js:162:
return /* @type {string} */ (customValue);
nit: Cast shouldn't be required since you are testing customValue above.

https://codereview.chromium.org/2838323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_property_list.js:178: *
@return {string|null} The text to display for the property value, or null
IIRC we prefer 'undefined' to 'null' for non-references. Alternately we could
just return an empty string when unhandled.

[Kyle Horimoto, 2017-04-26 17:37:00]

https://codereview.chromium.org/2838323002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_property_list.js
(right):

https://codereview.chromium.org/2838323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_property_list.js:162:
return /* @type {string} */ (customValue);
On 2017/04/26 17:26:56, stevenjb wrote:
> nit: Cast shouldn't be required since you are testing customValue above.

Done.

https://codereview.chromium.org/2838323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_property_list.js:178: *
@return {string|null} The text to display for the property value, or null
On 2017/04/26 17:26:56, stevenjb wrote:
> IIRC we prefer 'undefined' to 'null' for non-references. Alternately we could
> just return an empty string when unhandled.

Done.

[Kyle Horimoto, 2017-04-26 17:37:47]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-04-26 17:37:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2838323002/#ps80001
(title: "stevenjb@ comments.")

[commit-bot: I haz the power, 2017-04-26 17:38:36]

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, 2017-04-26 17:56:30]

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

[commit-bot: I haz the power, 2017-04-26 17:56:32]

Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Kyle Horimoto, 2017-04-27 20:38:30]

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

[commit-bot: I haz the power, 2017-04-27 20:39:19]

Dry run: 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, 2017-04-27 20:58:22]

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

[commit-bot: I haz the power, 2017-04-27 20:58:23]

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

[Kyle Horimoto, 2017-04-28 02:02:35]

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

[commit-bot: I haz the power, 2017-04-28 02:03:33]

Dry run: CQ is trying da patch.

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

[Kyle Horimoto, 2017-04-28 02:35:33]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-04-28 02:35:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2838323002/#ps140001
(title: "Fix Closure compiler error.")

[commit-bot: I haz the power, 2017-04-28 02:36:25]

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, 2017-04-28 05:24:17]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493346933008150,
"parent_rev": "b42016cc0c9f244e44a437ca060f50ff0edab189", "commit_rev":
"3643dc94bde2e194b0ce548ad4235ccad5bf06b1"}

[commit-bot: I haz the power, 2017-04-28 05:25:12]

Description was changed from

==========
[CrOS Tether] Add tether settings section to the MD settings page.

Currently, tether settings appear as part of their own subpage. In the future,
these settings will be merged with cellular networks under the "Mobile" subpage.

BUG=672263
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[CrOS Tether] Add tether settings section to the MD settings page.

Currently, tether settings appear as part of their own subpage. In the future,
these settings will be merged with cellular networks under the "Mobile" subpage.

BUG=672263
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2838323002
Cr-Commit-Position: refs/heads/master@{#467898}
Committed:
https://chromium.googlesource.com/chromium/src/+/3643dc94bde2e194b0ce548ad423...
==========

[commit-bot: I haz the power, 2017-04-28 05:25:13]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/3643dc94bde2e194b0ce548ad423...