MD Settings: Internet: Enable SIM unlock with no cellular network

MD Settings: Internet: Enable SIM unlock with no cellular network

This also fixes the keyboard navigation for network summary items.

BUG=609156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
For trivial comment changes to extensions/common/api
TBR=rdevlin.cronin@chromium.org,

Committed: https://crrev.com/230c1b488307a9667a5bb8c63f77707fc56f6439
Cr-Commit-Position: refs/heads/master@{#419088}

[stevenjb, 2016-09-01 22:54:37]

Description was changed from

==========
MD Settings: Internet: Enable SIM unlock with no cellular network

This also fixes the keyboard navigation for network summary items.

BUG=609156
==========

to

==========
MD Settings: Internet: Enable SIM unlock with no cellular network

This also fixes the keyboard navigation for network summary items.

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

[stevenjb, 2016-09-02 19:58:08]

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

[michaelpg, 2016-09-06 21:10:39]

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_summary_item.html
(right):

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:32:
#details[noflex] {
nit: no-flex (2 words)

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:33:
flex: 0;
should this be "flex: none" (for flex-shrink: 0)?

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:46:
<div id="details" selectable class="start layout horizontal center"
This is confusing because some of these classes are redundant, and then the
noflex attribute is basically used as a class to override parts of other classes
(start and layout.horizontal).

It's 4 classes + an optional attribute that sets a total of 2 properties AFAICT
(display and align-items [flex-direction is initially row anyway]). I had to
look in a lot of places to figure that out. Can you simplify this?

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:66:
<paper-icon-button icon="cr:settings" on-tap="onShowDetailsTap_">
import cr_elements/icons.html

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

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:216: *
Event triggered when the details div is tapped.
document that <Enter> on this element triggers this too

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:217: *
@param {Event} event The enable button event.
!Event while you're here

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:236: *
@param {Event} event The enable button event.
!Event

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:241:
this.fire('show-detail', this.activeNetworkState);
optional: early return?

https://codereview.chromium.org/2303093003/diff/20001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/2303093003/diff/20001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_chromeos.cc:202:
const chromeos::DeviceState* GetCellularDeviceState(const std::string& guid) {
chromeos::DeviceState is using'd at the top of the file, dunno if that means you
ought to drop the ns here though

https://codereview.chromium.org/2303093003/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/shared_style_css.html (right):

https://codereview.chromium.org/2303093003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/shared_style_css.html:65: [selectable] :focus{
space before {

https://codereview.chromium.org/2303093003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/shared_style_css.html:65: [selectable] :focus{
can this be limited to "[selectable] > :focus" (similar to below)?

[stevenjb, 2016-09-12 19:46:42]

PTAL

Note: I added a scanningIsVisible_(deviceState) to remove some extra padding
that was bothering me.

Note: I added a section to settings_vars_css.html when I merged with changes
from https://codereview.chromium.org/2315033002

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_summary_item.html
(right):

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:32:
#details[noflex] {
On 2016/09/06 21:10:38, michaelpg (OOO til Tuesday) wrote:
> nit: no-flex (2 words)

Done.

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:33:
flex: 0;
On 2016/09/06 21:10:38, michaelpg (OOO til Tuesday) wrote:
> should this be "flex: none" (for flex-shrink: 0)?
flex: none. Done.

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:46:
<div id="details" selectable class="start layout horizontal center"
On 2016/09/06 21:10:38, michaelpg (OOO til Tuesday) wrote:
> This is confusing because some of these classes are redundant, and then the
> noflex attribute is basically used as a class to override parts of other
classes
> (start and layout.horizontal).
> 
> It's 4 classes + an optional attribute that sets a total of 2 properties
AFAICT
> (display and align-items [flex-direction is initially row anyway]). I had to
> look in a lot of places to figure that out. Can you simplify this?

Yeah, the layout classes can be tricky to decode, although personally I find
them pretty readable once used to them.

Still, I went ahead and merged this into two classes specific to "details". Not
entirely convinced it's more readable but don't feel strongly either way.

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:66:
<paper-icon-button icon="cr:settings" on-tap="onShowDetailsTap_">
On 2016/09/06 21:10:38, michaelpg (OOO til Tuesday) wrote:
> import cr_elements/icons.html

Done.

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

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:216: *
Event triggered when the details div is tapped.
On 2016/09/06 21:10:38, michaelpg (OOO til Tuesday) wrote:
> document that <Enter> on this element triggers this too

Done.

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:217: *
@param {Event} event The enable button event.
On 2016/09/06 21:10:38, michaelpg (OOO til Tuesday) wrote:
> !Event while you're here

Done.

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:236: *
@param {Event} event The enable button event.
On 2016/09/06 21:10:38, michaelpg (OOO til Tuesday) wrote:
> !Event

Done.

https://codereview.chromium.org/2303093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:241:
this.fire('show-detail', this.activeNetworkState);
On 2016/09/06 21:10:38, michaelpg (OOO til Tuesday) wrote:
> optional: early return?

Done.

https://codereview.chromium.org/2303093003/diff/20001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/2303093003/diff/20001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_chromeos.cc:202:
const chromeos::DeviceState* GetCellularDeviceState(const std::string& guid) {
On 2016/09/06 21:10:38, michaelpg (OOO til Tuesday) wrote:
> chromeos::DeviceState is using'd at the top of the file, dunno if that means
you
> ought to drop the ns here though

I remove the 'using'.

https://codereview.chromium.org/2303093003/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/shared_style_css.html (right):

https://codereview.chromium.org/2303093003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/shared_style_css.html:65: [selectable] :focus{
On 2016/09/06 21:10:39, michaelpg (OOO til Tuesday) wrote:
> can this be limited to "[selectable] > :focus" (similar to below)?
I think so. Done.

[michaelpg, 2016-09-15 02:24:28]

lgtm with nits

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

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:46: *
settings-box:first-of-type */
nit: end sentence with period

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

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_property_list.html:23:
.settings-box:first-of-type {
I suppose I've already argued my case, but it would be nice if you didn't have
to do this :-(
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...

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

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.html:26: /* Use
.middle spacing but no flex */
period

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

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:53: *
@type {NetworkingPrivate}
!NetworkingPrivate perhaps?

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:139:
LockEnabled: device.SimLockType != CrOnc.LockType.NONE
nit: trailing comma for consistency

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_vars_css.html (right):

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_vars_css.html:54:
--checkbox-margin-start: 2px;
--checkbox-* are Settings vars, not Polymer vars

[stevenjb, 2016-09-15 17:42:17]

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

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:46: *
settings-box:first-of-type */
On 2016/09/15 02:24:28, michaelpg wrote:
> nit: end sentence with period

Done.

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

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_property_list.html:23:
.settings-box:first-of-type {
On 2016/09/15 02:24:28, michaelpg wrote:
> I suppose I've already argued my case, but it would be nice if you didn't have
> to do this :-(
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...

While I completely agree that we should not do this globally, or when we know
what the first entry is, in this case it is exactly what we need since we have
multiple potential first settings boxes depending on multiple conditions. If you
have an alternative suggestion that is not isThisTheFirstSection(a, b, c, d), I
am all ears :)

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

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.html:26: /* Use
.middle spacing but no flex */
On 2016/09/15 02:24:28, michaelpg wrote:
> period

Done.

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

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:53: *
@type {NetworkingPrivate}
On 2016/09/15 02:24:28, michaelpg wrote:
> !NetworkingPrivate perhaps?
Done. (I thought this would need to become !NetworkingPrivate|undefined, but
apparently not...)

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:139:
LockEnabled: device.SimLockType != CrOnc.LockType.NONE
On 2016/09/15 02:24:28, michaelpg wrote:
> nit: trailing comma for consistency

Done.

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_vars_css.html (right):

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_vars_css.html:54:
--checkbox-margin-start: 2px;
On 2016/09/15 02:24:28, michaelpg wrote:
> --checkbox-* are Settings vars, not Polymer vars

I will rename and move these and put all of these changes in a separate CL.

[michaelpg, 2016-09-15 21:35:16]

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

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_property_list.html:23:
.settings-box:first-of-type {
On 2016/09/15 17:42:17, stevenjb wrote:
> On 2016/09/15 02:24:28, michaelpg wrote:
> > I suppose I've already argued my case, but it would be nice if you didn't
have
> > to do this :-(
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...
> 
> While I completely agree that we should not do this globally, or when we know
> what the first entry is, in this case it is exactly what we need since we have
> multiple potential first settings boxes depending on multiple conditions. If
you
> have an alternative suggestion that is not isThisTheFirstSection(a, b, c, d),
I
> am all ears :)

No, this is my preferred solution! I was just lamenting that I *do* think we
should do this globally (but have already conceded that).

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

https://codereview.chromium.org/2303093003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:53: *
@type {NetworkingPrivate}
On 2016/09/15 17:42:17, stevenjb wrote:
> On 2016/09/15 02:24:28, michaelpg wrote:
> > !NetworkingPrivate perhaps?
> Done. (I thought this would need to become !NetworkingPrivate|undefined, but
> apparently not...)
> 

Polymer properties are a weird case. It's possible this is undefined (if the
networking-private attribute isn't set, or when this is created imperatively in
a test) but that's an abnormal condition we don't want to account for. So
telling Closure this should never be undefined seems cool.

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

https://codereview.chromium.org/2303093003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:208: let
type = this.deviceState.Type;
nit: be consistent with let/var. There's really no reason to prefer one or the
other here.

[michaelpg, 2016-09-15 21:35:19]

lgtm

[stevenjb, 2016-09-16 00:04:44]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-09-16 00:04:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2303093003/#ps100001
(title: "No settings_vars_css cleanup")

[commit-bot: I haz the power, 2016-09-16 00:05:12]

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-16 00:14:33]

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

[commit-bot: I haz the power, 2016-09-16 00:14:35]

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, 2016-09-16 02:44:55]

Description was changed from

==========
MD Settings: Internet: Enable SIM unlock with no cellular network

This also fixes the keyboard navigation for network summary items.

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

to

==========
MD Settings: Internet: Enable SIM unlock with no cellular network

This also fixes the keyboard navigation for network summary items.

BUG=609156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
For trivial comment changes to extensions/common/api
TBR=rdevlin.cronin@chromium.org,
==========

[stevenjb, 2016-09-16 02:45:21]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2016-09-16 02:45:42]

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-16 03:15:13]

Description was changed from

==========
MD Settings: Internet: Enable SIM unlock with no cellular network

This also fixes the keyboard navigation for network summary items.

BUG=609156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
For trivial comment changes to extensions/common/api
TBR=rdevlin.cronin@chromium.org,
==========

to

==========
MD Settings: Internet: Enable SIM unlock with no cellular network

This also fixes the keyboard navigation for network summary items.

BUG=609156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
For trivial comment changes to extensions/common/api
TBR=rdevlin.cronin@chromium.org,
==========

[commit-bot: I haz the power, 2016-09-16 03:15:14]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-16 03:18:10]

Description was changed from

==========
MD Settings: Internet: Enable SIM unlock with no cellular network

This also fixes the keyboard navigation for network summary items.

BUG=609156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
For trivial comment changes to extensions/common/api
TBR=rdevlin.cronin@chromium.org,
==========

to

==========
MD Settings: Internet: Enable SIM unlock with no cellular network

This also fixes the keyboard navigation for network summary items.

BUG=609156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
For trivial comment changes to extensions/common/api
TBR=rdevlin.cronin@chromium.org,

Committed: https://crrev.com/230c1b488307a9667a5bb8c63f77707fc56f6439
Cr-Commit-Position: refs/heads/master@{#419088}
==========

[commit-bot: I haz the power, 2016-09-16 03:18:12]

Patchset 6 (id:??) landed as
https://crrev.com/230c1b488307a9667a5bb8c63f77707fc56f6439
Cr-Commit-Position: refs/heads/master@{#419088}