MD Settings: Internet: Add/fix remaining policy items

MD Settings: Internet: Add/fix remaining policy items

This CL:
* Adds a policy indicator to cr-network-list-item
* Adds a policy indicator in the top line of the detailed view
* Replaces the proxy indicator for extensions with
  extension-controlled-indicator
* Includes a small amount of additional layout cleanup

BUG=668228
For existing I18nBehavior in cr_network*:
NOPRESUBMIT=true
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/814ce188d59121f142274c44629521d99d27c570
Cr-Commit-Position: refs/heads/master@{#436403}

[stevenjb, 2016-11-23 23:25:10]

Description was changed from

==========
MD Settings: Internet: Add/fix remaining policy items

This CL:
* Adds a policy indicator to cr-network-list-item
* Adds a policy indicator in the top line of the detailed view
* Replaces the proxy indicator for extensions with
  extension-controlled-indicator
* Includes a small amount of additional layout cleanup

BUG=668228
==========

to

==========
MD Settings: Internet: Add/fix remaining policy items

This CL:
* Adds a policy indicator to cr-network-list-item
* Adds a policy indicator in the top line of the detailed view
* Replaces the proxy indicator for extensions with
  extension-controlled-indicator
* Includes a small amount of additional layout cleanup

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

[stevenjb, 2016-11-23 23:37:30]

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

[Dan Beam, 2016-11-29 05:25:35]

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

https://codereview.chromium.org/2528803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:98:
networkProperties.Source)]]">
indent off, I think we've generally lined these up like if it were a JavaScript
call

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

https://codereview.chromium.org/2528803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_property_list.html:28:
padding: 0 10px;
where are you getting this 10px from?

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

https://codereview.chromium.org/2528803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.html:65: <div
class="settings-box first single-column"
why does this go into a new settings-box?  doesn't that add a border?  that's
not what we want, no?

https://codereview.chromium.org/2528803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.html:66:
hidden$="[[!getShowExtensionPolicyIndicator_(networkProperties)]]">
arguable nit: use dom-if instead of hidden binding

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

https://codereview.chromium.org/2528803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.js:349:
getShowExtensionPolicyIndicator_: function() {
why does this use 2 verbs? "get" AND "show"?  why not
shouldShowExtensionIndicator()?  or just isExtensionControlled?

also, why both "Policy" and "Extension"?  that's really confusing...

https://codereview.chromium.org/2528803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.js:392: return
!property || this.isEditable_(property);
can you test this logic?

https://codereview.chromium.org/2528803002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right):

https://codereview.chromium.org/2528803002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list_item.html:88: padding: 0
5px;
where are you getting this value from?

https://codereview.chromium.org/2528803002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/policy/cr_policy_network_behavior.js
(right):

https://codereview.chromium.org/2528803002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/policy/cr_policy_network_behavior.js:77:
isPolicySource(source) {
please use

isPolicySource: function(source) {

syntax instead of the enhanced object literals until it's been approved for ES6
use in Chrome.

right now it will prevent you from vulcanizing

https://codereview.chromium.org/2528803002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/policy/cr_policy_network_indicator.js
(left):

https://codereview.chromium.org/2528803002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/policy/cr_policy_network_indicator.js:80: if
(type == CrPolicyIndicatorType.NONE || typeof property != 'object')
wait, how could this possibly not be an object according to the @param type?

[stevenjb, 2016-11-30 22:58:19]

PTAL

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

https://codereview.chromium.org/2528803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:98:
networkProperties.Source)]]">
On 2016/11/29 05:25:34, Dan Beam wrote:
> indent off, I think we've generally lined these up like if it were a
JavaScript
> call

Meh. My editor likes to do it this way. I'm sure what your suggestion is, 4
spaces in from 'indicator-type seems the most html consistent?

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

https://codereview.chromium.org/2528803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_property_list.html:28:
padding: 0 10px;
On 2016/11/29 05:25:34, Dan Beam wrote:
> where are you getting this 10px from?

Created --cr-icon-padding in shared_vars_css

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

https://codereview.chromium.org/2528803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.html:65: <div
class="settings-box first single-column"
On 2016/11/29 05:25:34, Dan Beam wrote:
> why does this go into a new settings-box?  doesn't that add a border?  that's
> not what we want, no?

Only one or the other will be shown. Also, 'first' or 'continuation' hides the
border. I'll use 'continuation for both of these to match 'Allow shared
proxies'.

https://codereview.chromium.org/2528803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.html:66:
hidden$="[[!getShowExtensionPolicyIndicator_(networkProperties)]]">
On 2016/11/29 05:25:34, Dan Beam wrote:
> arguable nit: use dom-if instead of hidden binding

Done.

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

https://codereview.chromium.org/2528803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.js:349:
getShowExtensionPolicyIndicator_: function() {
On 2016/11/29 05:25:34, Dan Beam wrote:
> why does this use 2 verbs? "get" AND "show"?  why not
> shouldShowExtensionIndicator()?  or just isExtensionControlled?
> 
> also, why both "Policy" and "Extension"?  that's really confusing...

-> shouldShowExtensionIndicator

https://codereview.chromium.org/2528803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.js:392: return
!property || this.isEditable_(property);
On 2016/11/29 05:25:34, Dan Beam wrote:
> can you test this logic?

We *should* test all of 'internet'. We currently test *none* of it. The testing
is... daunting. I have been suggesting that adding tests would be a great way
for someone else to become familiar with this rather large amount of code for a
while now...

https://codereview.chromium.org/2528803002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right):

https://codereview.chromium.org/2528803002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list_item.html:88: padding: 0
5px;
On 2016/11/29 05:25:34, Dan Beam wrote:
> where are you getting this value from?
Created --cr-icon-padding in shared_vars_css

https://codereview.chromium.org/2528803002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/policy/cr_policy_network_behavior.js
(right):

https://codereview.chromium.org/2528803002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/policy/cr_policy_network_behavior.js:77:
isPolicySource(source) {
On 2016/11/29 05:25:34, Dan Beam wrote:
> please use
> 
> isPolicySource: function(source) {
> 
> syntax instead of the enhanced object literals until it's been approved for
ES6
> use in Chrome.
> 
> right now it will prevent you from vulcanizing

I'm aware. Old habits. Thanks for catching. Done.

https://codereview.chromium.org/2528803002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/policy/cr_policy_network_indicator.js
(left):

https://codereview.chromium.org/2528803002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/policy/cr_policy_network_indicator.js:80: if
(type == CrPolicyIndicatorType.NONE || typeof property != 'object')
On 2016/11/29 05:25:34, Dan Beam wrote:
> wait, how could this possibly not be an object according to the @param type?

Oh, it can't, I should have realized that. This probably evolved/migrated from
code where it could have been {CrOnc.ManagedProperty|CrOnc.NetworkPropertyType}.
Removed the check.

[Dan Beam, 2016-12-02 00:23:50]

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

https://codereview.chromium.org/2528803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.js:392: return
!property || this.isEditable_(property);
On 2016/11/30 22:58:19, stevenjb wrote:
> On 2016/11/29 05:25:34, Dan Beam wrote:
> > can you test this logic?
> 
> We *should* test all of 'internet'. We currently test *none* of it. The
testing
> is... daunting. I have been suggesting that adding tests would be a great way
> for someone else to become familiar with this rather large amount of code for
a
> while now...

now's a great time to start.

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

https://codereview.chromium.org/2528803002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.html:69: <div
class="settings-box continuation single-column">
we generally don't show only an <extension-controlled-indicator> in a box.  can
you send me screenshots of this CL?

[stevenjb, 2016-12-02 00:54:49]

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

https://codereview.chromium.org/2528803002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.html:69: <div
class="settings-box continuation single-column">
On 2016/12/02 00:23:50, Dan Beam wrote:
> we generally don't show only an <extension-controlled-indicator> in a box. 
can
> you send me screenshots of this CL?

The box has no border ('continuation') it just provides consistent spacing. I
could define a var to match the settings-box margin instead if you prefer.

Within a settings-box:
https://screenshot.googleplex.com/PLcNNxMywek.png

With -webkit-margin-start: 20px:
https://screenshot.googleplex.com/6TzSQSiswXi.png

[Dan Beam, 2016-12-02 00:59:49]

lgtm

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

https://codereview.chromium.org/2528803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.js:392: return
!property || this.isEditable_(property);
On 2016/12/02 00:23:50, Dan Beam wrote:
> On 2016/11/30 22:58:19, stevenjb wrote:
> > On 2016/11/29 05:25:34, Dan Beam wrote:
> > > can you test this logic?
> > 
> > We *should* test all of 'internet'. We currently test *none* of it. The
> testing
> > is... daunting. I have been suggesting that adding tests would be a great
way
> > for someone else to become familiar with this rather large amount of code
for
> a
> > while now...
> 
> now's a great time to start.

let me expound on this: the more code you write, the more daunting this will be.
 i don't think "there's no tests and we wrote a lot of code that's not tested,
and will likely continue to add more untested code" is a good excuse.

anyways, as this will ultimately be your burden to bear, i'll leave you to
decide.

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

https://codereview.chromium.org/2528803002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.html:69: <div
class="settings-box continuation single-column">
On 2016/12/02 00:54:49, stevenjb wrote:
> On 2016/12/02 00:23:50, Dan Beam wrote:
> > we generally don't show only an <extension-controlled-indicator> in a box. 
> can
> > you send me screenshots of this CL?
> 
> The box has no border ('continuation') it just provides consistent spacing. I
> could define a var to match the settings-box margin instead if you prefer.
> 
> Within a settings-box:
> https://screenshot.googleplex.com/PLcNNxMywek.png
> 
> With -webkit-margin-start: 20px:
> https://screenshot.googleplex.com/6TzSQSiswXi.png

oh yeah, sorry, saw your last reply but didn't remove this comment.  my bad!

[stevenjb, 2016-12-02 17:36:58]

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

https://codereview.chromium.org/2528803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.js:392: return
!property || this.isEditable_(property);
On 2016/12/02 00:59:49, Dan Beam wrote:
> On 2016/12/02 00:23:50, Dan Beam wrote:
> > On 2016/11/30 22:58:19, stevenjb wrote:
> > > On 2016/11/29 05:25:34, Dan Beam wrote:
> > > > can you test this logic?
> > > 
> > > We *should* test all of 'internet'. We currently test *none* of it. The
> > testing
> > > is... daunting. I have been suggesting that adding tests would be a great
> way
> > > for someone else to become familiar with this rather large amount of code
> for
> > a
> > > while now...
> > 
> > now's a great time to start.
> 
> let me expound on this: the more code you write, the more daunting this will
be.
>  i don't think "there's no tests and we wrote a lot of code that's not tested,
> and will likely continue to add more untested code" is a good excuse.
> 
> anyways, as this will ultimately be your burden to bear, i'll leave you to
> decide.

I am not unaware of any of this. The question has always been, "is it worth
taking a week or two to write some tests for this section that might actually
catch regressions?" So far, my response to myself (and I though we had also
discussed this) has been, "this would be a great way to get someone else up to
speed on this code so that I am not the sole maintainer" (since writing useful
tests will require understanding the model API that the UI is built on). Also,
"we really need to ship this sooner than late,r and tests are more valuable (in
my experience) for long term maintenance than short term stability (which is not
to say that tests don't catch short term bugs, but I find that to be more true
with complex model code, which in this case is pretyt well tested, than with UI
controller code).

Anyway, your suggestion is duly noted.

[stevenjb, 2016-12-02 17:39:49]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-12-02 17:39:49]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2528803002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-12-02 17:40:09]

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-12-02 17:47:24]

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

[commit-bot: I haz the power, 2016-12-02 17:47:25]

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-12-02 18:11:36]

Hey Dan, this CL adds a behavior to cr_network_list_item.js, which has the
Known Issue that it is using I18nBehavior but should not be (
crbug.com/670767).

Is there a way to bypass this presubmit for this CL? Or would you prefer I
prioritize fixing crbug.com/670767 first? I estimate it will take 1-3 days,
longer with proper testing (which will be especially important here since
the components are shared).


On Fri, Dec 2, 2016 at 10:47 AM, commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> 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_presubmit/builds/317527)
>
> https://codereview.chromium.org/2528803002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2016-12-05 20:01:10]

Description was changed from

==========
MD Settings: Internet: Add/fix remaining policy items

This CL:
* Adds a policy indicator to cr-network-list-item
* Adds a policy indicator in the top line of the detailed view
* Replaces the proxy indicator for extensions with
  extension-controlled-indicator
* Includes a small amount of additional layout cleanup

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

to

==========
MD Settings: Internet: Add/fix remaining policy items

This CL:
* Adds a policy indicator to cr-network-list-item
* Adds a policy indicator in the top line of the detailed view
* Replaces the proxy indicator for extensions with
  extension-controlled-indicator
* Includes a small amount of additional layout cleanup

BUG=668228
For existing I18nBehavior in cr_network*:
NOPRESUBMIT=true
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2016-12-05 20:03:11]

After a fair bit of poking at this, I'm not entirely convinced that eliminating
i18n related calls from cr_elements/ makes sense. I'm going to go ahead and
submit this as-is (since it doesn't introduce any additional i18n dependencies)
and we can discuss the best way to address this separately.

[stevenjb, 2016-12-05 20:03:28]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2016-12-05 20:03: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, 2016-12-05 21:14:19]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480968208704180,
"parent_rev": "ea5309e2da8de068cbe4fb2bb3519aa370018f34", "commit_rev":
"609f57f07619833fa2c0778409a4c00becf8219d"}

[commit-bot: I haz the power, 2016-12-05 21:14:53]

Description was changed from

==========
MD Settings: Internet: Add/fix remaining policy items

This CL:
* Adds a policy indicator to cr-network-list-item
* Adds a policy indicator in the top line of the detailed view
* Replaces the proxy indicator for extensions with
  extension-controlled-indicator
* Includes a small amount of additional layout cleanup

BUG=668228
For existing I18nBehavior in cr_network*:
NOPRESUBMIT=true
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Internet: Add/fix remaining policy items

This CL:
* Adds a policy indicator to cr-network-list-item
* Adds a policy indicator in the top line of the detailed view
* Replaces the proxy indicator for extensions with
  extension-controlled-indicator
* Includes a small amount of additional layout cleanup

BUG=668228
For existing I18nBehavior in cr_network*:
NOPRESUBMIT=true
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-12-05 21:14:55]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-05 21:18:12]

Description was changed from

==========
MD Settings: Internet: Add/fix remaining policy items

This CL:
* Adds a policy indicator to cr-network-list-item
* Adds a policy indicator in the top line of the detailed view
* Replaces the proxy indicator for extensions with
  extension-controlled-indicator
* Includes a small amount of additional layout cleanup

BUG=668228
For existing I18nBehavior in cr_network*:
NOPRESUBMIT=true
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Internet: Add/fix remaining policy items

This CL:
* Adds a policy indicator to cr-network-list-item
* Adds a policy indicator in the top line of the detailed view
* Replaces the proxy indicator for extensions with
  extension-controlled-indicator
* Includes a small amount of additional layout cleanup

BUG=668228
For existing I18nBehavior in cr_network*:
NOPRESUBMIT=true
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/814ce188d59121f142274c44629521d99d27c570
Cr-Commit-Position: refs/heads/master@{#436403}
==========

[commit-bot: I haz the power, 2016-12-05 21:18:13]

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