Settings: Network: Merge Tether networks into Mobile subpage

Settings: Network: Merge Tether networks into Mobile subpage

BUG=726380
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
For ui/webui/help:
TBR=msw@chromium.org

Review-Url: https://codereview.chromium.org/2913323003
Cr-Commit-Position: refs/heads/master@{#476820}
Committed: https://chromium.googlesource.com/chromium/src/+/04968fe41e04b5f33305d9584561909760d9c3c7

[stevenjb, 2017-06-01 00:36:14]

Description was changed from

==========
Settings: Network: Merge Tether networks into Mobile subpage

BUG=726380
==========

to

==========
Settings: Network: Merge Tether networks into Mobile subpage

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

[stevenjb, 2017-06-01 17:18:03]

Patchset #2 (id:20001) has been deleted

[stevenjb, 2017-06-01 17:33:50]

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

[stevenjb, 2017-06-01 17:33:50]

Here you go! I tried a couple of approaches and went with what I think is the
simplest:

* If only Cellular is available, behave as before (no subpage).
* If only Tether is available, just create a Tether section / subpage like
before but use the 'Mobile data' label.
* If both are available do some slight-of-hand to combine the section / subpage
(which I commented liberally).

[Ryan Hansberry, 2017-06-01 18:12:59]

hansberry@chromium.org changed reviewers:
+ hansberry@chromium.org

[Ryan Hansberry, 2017-06-01 18:13:00]

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

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_subpage.js:443:
tetherDeviceState.State != CrOnc.DeviceState.UNINITIALIZED;
[Apologies in advance for being a noob here] 

Does this mean that if the Tether technology state is UNINITIALIZED that the
toggle will not be visible? Right now Tether technology state is UNINITIALIZED
when Bluetooth or Cellular are disabled, but it's expected that the Tether
toggle be greyed out as opposed to invisible when that's the case.

[Ryan Hansberry, 2017-06-01 18:14:10]

Description was changed from

==========
Settings: Network: Merge Tether networks into Mobile subpage

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

to

==========
Settings: Network: Merge Tether networks into Mobile subpage

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

[Kyle Horimoto, 2017-06-01 20:15:22]

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_subpage.html
(right):

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_subpage.html:138:
aria-describedby="tetherSecondary"
Just making sure: this property makes it so that the toggle is toggled when the
text in tetherSecondary is clicked, right?

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

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_subpage.js:193: if
(this.deviceState.Type == CrOnc.Type.CELLULAR &&
What about the opposite case when type == TETHER? Don't we have to do the
reverse and look up the CELLULAR networks?

For reference, we open that page here:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/net/tether_notif...

Alternatively, we could change the entry point to go Cellular instead of Tether.

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_subpage.js:440:
tetherToggleIsVisible_: function(deviceState, tetherDeviceState) {
Edge case I thought of: What if cellular is not present and there are no tether
networks in the list? Wouldn't this result in a top toggle and a bottom toggle
with no networks in between? Should we still show the tether toggle in this
case?

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

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary.js:312: if
(!device)
Is this if() necessary? It seems like we should always have a DeviceState, even
if it is unavailable.

As a corollary, would newDeviceStates[CELLULAR] below ever be falsey? Should we
be checking the state instead?

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary.js:344: * If
there is no 'Celular' network, return the active 'Tether' network if
Cellular

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

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:175:
deviceState.State != CrOnc.DeviceState.UNINITIALIZED;
If Type == CELLULAR and neither Cellular or Tether networks are available,
shouldn't we hide the toggle?

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:232: if
(type == CrOnc.Type.CELLULAR && this.tetherDeviceState)
nit: Shouldn't this be !!this.tetherDeviceState?

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:235:
type == CrOnc.Type.TETHER) ?
Isn't it impossible for type to be TETHER now that you eliminated the TETHER
type from being passed here in NetworkStateSummary?

https://codereview.chromium.org/2913323003/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/internet_page_tests.js (right):

https://codereview.chromium.org/2913323003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/internet_page_tests.js:137: test('Tether',
function() {
nit: Can we also test cases where only one of the two types is enabled?

Also, do you think it makes sense to verify that the actual network metadata is
present in the DOM instead of only verifying that there are 3 networks? I'm not
sure if this is valuable or not.

[stevenjb, 2017-06-01 21:55:04]

PTAL

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_subpage.html
(right):

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_subpage.html:138:
aria-describedby="tetherSecondary"
On 2017/06/01 20:15:21, Kyle Horimoto wrote:
> Just making sure: this property makes it so that the toggle is toggled when
the
> text in tetherSecondary is clicked, right?

No, this is for accessibility. I need to add 'on-tap' for the parent div for
that, thanks! Done.

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

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_subpage.js:193: if
(this.deviceState.Type == CrOnc.Type.CELLULAR &&
On 2017/06/01 20:15:21, Kyle Horimoto wrote:
> What about the opposite case when type == TETHER? Don't we have to do the
> reverse and look up the CELLULAR networks?
> 
> For reference, we open that page here:
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/net/tether_notif...
> 
> Alternatively, we could change the entry point to go Cellular instead of
Tether.

Ah, good catch. Rather than complicate this code, I added a small change to
always pass the Cellular device state to the subpage if both technologies are
available and we navigate directly to the Tether subpage. (That data binding is
in internet_page.html.)

I also update the comment for |deviceState|.

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_subpage.js:440:
tetherToggleIsVisible_: function(deviceState, tetherDeviceState) {
On 2017/06/01 20:15:21, Kyle Horimoto wrote:
> Edge case I thought of: What if cellular is not present and there are no
tether
> networks in the list? Wouldn't this result in a top toggle and a bottom toggle
> with no networks in between? Should we still show the tether toggle in this
> case?

Correct. I believe we still need access to both toggles. (If we decide to change
this or add some additional text we can do that later).

I think there may be some additional edge cases, but I'm not actually set up to
test on a device, so I leave those as an exercise for the reader :)

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_subpage.js:443:
tetherDeviceState.State != CrOnc.DeviceState.UNINITIALIZED;
On 2017/06/01 18:13:00, Ryan Hansberry wrote:
> [Apologies in advance for being a noob here] 
> 
> Does this mean that if the Tether technology state is UNINITIALIZED that the
> toggle will not be visible? Right now Tether technology state is UNINITIALIZED
> when Bluetooth or Cellular are disabled, but it's expected that the Tether
> toggle be greyed out as opposed to invisible when that's the case.

OK. That is not made clear in the UX design doc I have to reference. Is there
another document where this is mentioned that we can link to the issue?

I will add a tetherToggleIsEnabled_ and add the UNINITIALIZED test there.

Is there some other message we should show in that case to explain that? Again,
a design doc would be nice.

Note: There are actually *TWO* places where we might show this toggle. Here in
the subpage, and in network-summary-item (on the main page) when there is no
Cellular technology available. I will modify both.

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

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary.js:312: if
(!device)
On 2017/06/01 20:15:21, Kyle Horimoto wrote:
> Is this if() necessary? It seems like we should always have a DeviceState,
even
> if it is unavailable.
> 
> As a corollary, would newDeviceStates[CELLULAR] below ever be falsey? Should
we
> be checking the state instead?

Incorrect. We have no "unavailable" device.State, instead we just do not provide
a device. (If there is no cellular or wimax modem, we do not burden the UI with
extra empty data).

(The reason we have this test at all is so that the UI can iterate over all
possible device types in the order it wants to present them, rather than relying
on whatever order the model provides or the arbitrary order of a map).

I'll add a small comment here.

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary.js:344: * If
there is no 'Celular' network, return the active 'Tether' network if
On 2017/06/01 20:15:21, Kyle Horimoto wrote:
> Cellular

Done.

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

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:175:
deviceState.State != CrOnc.DeviceState.UNINITIALIZED;
On 2017/06/01 20:15:21, Kyle Horimoto wrote:
> If Type == CELLULAR and neither Cellular or Tether networks are available,
> shouldn't we hide the toggle?

Enable/Disable is separate for networks existing, at least for Cellular.

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:232: if
(type == CrOnc.Type.CELLULAR && this.tetherDeviceState)
On 2017/06/01 20:15:21, Kyle Horimoto wrote:
> nit: Shouldn't this be !!this.tetherDeviceState?

That's only necessary when you are returning a boolean type; undefined or null
objects are falsy in JS (and vice versa).

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:235:
type == CrOnc.Type.TETHER) ?
On 2017/06/01 20:15:21, Kyle Horimoto wrote:
> Isn't it impossible for type to be TETHER now that you eliminated the TETHER
> type from being passed here in NetworkStateSummary?

We combine Tether with Cellular *IF* Cellular exists (i.e. is available), but if
Cellular is unavailable, type can be Tether.

https://codereview.chromium.org/2913323003/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/internet_page_tests.js (right):

https://codereview.chromium.org/2913323003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/internet_page_tests.js:137: test('Tether',
function() {
On 2017/06/01 20:15:21, Kyle Horimoto wrote:
> nit: Can we also test cases where only one of the two types is enabled?
> 
> Also, do you think it makes sense to verify that the actual network metadata
is
> present in the DOM instead of only verifying that there are 3 networks? I'm
not
> sure if this is valuable or not.

1) Yes.
2) No, I don't think that is especially valuable. We're generating the fake data
in the test itself, and in my experience, UI tests that are too specific are
more likely to generate false positives than catch real regression. I prefer to
just ensure that critical elements exist and are receiving data from the model.

[Kyle Horimoto, 2017-06-01 22:21:46]

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

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_subpage.js:443:
tetherDeviceState.State != CrOnc.DeviceState.UNINITIALIZED;
On 2017/06/01 21:55:03, stevenjb wrote:
> On 2017/06/01 18:13:00, Ryan Hansberry wrote:
> > [Apologies in advance for being a noob here] 
> > 
> > Does this mean that if the Tether technology state is UNINITIALIZED that the
> > toggle will not be visible? Right now Tether technology state is
UNINITIALIZED
> > when Bluetooth or Cellular are disabled, but it's expected that the Tether
> > toggle be greyed out as opposed to invisible when that's the case.
> 
> OK. That is not made clear in the UX design doc I have to reference. Is there
> another document where this is mentioned that we can link to the issue?
> 
> I will add a tetherToggleIsEnabled_ and add the UNINITIALIZED test there.
> 
> Is there some other message we should show in that case to explain that?
Again,
> a design doc would be nice.
> 
> Note: There are actually *TWO* places where we might show this toggle. Here in
> the subpage, and in network-summary-item (on the main page) when there is no
> Cellular technology available. I will modify both.
> 

Did you make this change? I don't think I see it, but maybe I'm missing it?

https://codereview.chromium.org/2913323003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/internet_page/internet_subpage.html
(right):

https://codereview.chromium.org/2913323003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/internet_subpage.html:130:
on-tap="onTetherEnabledTap_">
nit: Now that you added this, do you still need the <paper-toggle-button>'s
on-tap property?

https://codereview.chromium.org/2913323003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/internet_page/internet_subpage.js
(right):

https://codereview.chromium.org/2913323003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/internet_subpage.js:365:
onDeviceEnabledTap_: function(event) {
If we're at the TETHER page (i.e., CELLULAR is unavailable) and this is tapped,
what do we expect to happen? Wouldn't this cause the main (top) toggle and the
lower ("tether-only") toggle to be out of sync?

[stevenjb, 2017-06-01 22:37:02]

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

https://codereview.chromium.org/2913323003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_subpage.js:443:
tetherDeviceState.State != CrOnc.DeviceState.UNINITIALIZED;
On 2017/06/01 22:21:46, Kyle Horimoto wrote:
> On 2017/06/01 21:55:03, stevenjb wrote:
> > On 2017/06/01 18:13:00, Ryan Hansberry wrote:
> > > [Apologies in advance for being a noob here] 
> > > 
> > > Does this mean that if the Tether technology state is UNINITIALIZED that
the
> > > toggle will not be visible? Right now Tether technology state is
> UNINITIALIZED
> > > when Bluetooth or Cellular are disabled, but it's expected that the Tether
> > > toggle be greyed out as opposed to invisible when that's the case.
> > 
> > OK. That is not made clear in the UX design doc I have to reference. Is
there
> > another document where this is mentioned that we can link to the issue?
> > 
> > I will add a tetherToggleIsEnabled_ and add the UNINITIALIZED test there.
> > 
> > Is there some other message we should show in that case to explain that?
> Again,
> > a design doc would be nice.
> > 
> > Note: There are actually *TWO* places where we might show this toggle. Here
in
> > the subpage, and in network-summary-item (on the main page) when there is no
> > Cellular technology available. I will modify both.
> > 
> 
> Did you make this change? I don't think I see it, but maybe I'm missing it?

I did... this function no longer tests 'etherDeviceState.State !=
CrOnc.DeviceState.UNINITIALIZED' and see tetherToggleIsEnabled_ in this file...

https://codereview.chromium.org/2913323003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/internet_page/internet_subpage.html
(right):

https://codereview.chromium.org/2913323003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/internet_subpage.html:130:
on-tap="onTetherEnabledTap_">
On 2017/06/01 22:21:46, Kyle Horimoto wrote:
> nit: Now that you added this, do you still need the <paper-toggle-button>'s
> on-tap property?

I suppose I don't. Done.

https://codereview.chromium.org/2913323003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/internet_page/internet_subpage.js
(right):

https://codereview.chromium.org/2913323003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/internet_subpage.js:365:
onDeviceEnabledTap_: function(event) {
On 2017/06/01 22:21:46, Kyle Horimoto wrote:
> If we're at the TETHER page (i.e., CELLULAR is unavailable) and this is
tapped,
> what do we expect to happen? Wouldn't this cause the main (top) toggle and the
> lower ("tether-only") toggle to be out of sync?

We do not show the lower "tether-only" toggle unless deviceState.Type ==
CrOnc.Type.CELLULAR (see tetherToggleIsVisible_() below). Even if we did show it
they would both point to the same state so toggline one would change the other.

[Kyle Horimoto, 2017-06-01 22:57:41]

lgtm

[stevenjb, 2017-06-02 00:45:07]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2017-06-02 00:45: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, 2017-06-02 00:56:08]

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

[commit-bot: I haz the power, 2017-06-02 00:56:09]

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

[stevenjb, 2017-06-02 16:13:37]

Description was changed from

==========
Settings: Network: Merge Tether networks into Mobile subpage

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

to

==========
Settings: Network: Merge Tether networks into Mobile subpage

BUG=726380
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
For ui/webui/help:
TBR=msw@chromium.org
==========

[stevenjb, 2017-06-02 16:49:23]

Fix unit tests

[stevenjb, 2017-06-02 16:49:42]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-06-02 16:49:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
khorimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/2913323003/#ps140001
(title: "Fix unit tests")

[commit-bot: I haz the power, 2017-06-02 16:49:55]

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-06-02 18:44:04]

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

[commit-bot: I haz the power, 2017-06-02 18:44:05]

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

[stevenjb, 2017-06-02 19:47:53]

Fix browser_tests

[stevenjb, 2017-06-02 20:40:10]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-06-02 20:40:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
khorimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/2913323003/#ps160001
(title: "Fix browser_tests")

[commit-bot: I haz the power, 2017-06-02 20:40:46]

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-06-02 22:34:03]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496436010714070,
"parent_rev": "12ef9b2c2f3fffda52ed1f1505c33bf174f19a1e", "commit_rev":
"04968fe41e04b5f33305d9584561909760d9c3c7"}

[commit-bot: I haz the power, 2017-06-02 22:34:18]

Description was changed from

==========
Settings: Network: Merge Tether networks into Mobile subpage

BUG=726380
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
For ui/webui/help:
TBR=msw@chromium.org
==========

to

==========
Settings: Network: Merge Tether networks into Mobile subpage

BUG=726380
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
For ui/webui/help:
TBR=msw@chromium.org

Review-Url: https://codereview.chromium.org/2913323003
Cr-Commit-Position: refs/heads/master@{#476820}
Committed:
https://chromium.googlesource.com/chromium/src/+/04968fe41e04b5f33305d9584561...
==========

[commit-bot: I haz the power, 2017-06-02 22:34:20]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/04968fe41e04b5f33305d9584561...