WebUI: Add cr_policy_network_indicator_tests.js

WebUI: Add cr_policy_network_indicator_tests.js

This also makes indicatorType private for cr-policy-network-indicator
and switches existing usage to cr-policy-pref-indicator (which now
correctly supports indicator-type).

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

Review-Url: https://codereview.chromium.org/2625213002
Cr-Commit-Position: refs/heads/master@{#455830}
Committed: https://chromium.googlesource.com/chromium/src/+/0c3256d22e6966598c8d5cd101cd7e827b0f42b9

[stevenjb, 2017-01-11 22:52:04]

Description was changed from

==========
WebUI: Add cr_policy_network_indicator_tests.js

This also makes indicatorType private for cr-policy-network-indicator
and switches existing usage to cr-policy-pref-indicator (which now
correctly supports indicator-type).

BUG=679561
==========

to

==========
WebUI: Add cr_policy_network_indicator_tests.js

This also makes indicatorType private for cr-policy-network-indicator
and switches existing usage to cr-policy-pref-indicator (which now
correctly supports indicator-type).

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

[stevenjb, 2017-01-12 17:13:33]

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

[Dan Beam, 2017-01-12 18:16:37]

Description was changed from

==========
WebUI: Add cr_policy_network_indicator_tests.js

This also makes indicatorType private for cr-policy-network-indicator
and switches existing usage to cr-policy-pref-indicator (which now
correctly supports indicator-type).

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

to

==========
WebUI: Add cr_policy_network_indicator_tests.js

This also makes indicatorType private for cr-policy-network-indicator
and switches existing usage to cr-policy-pref-indicator (which now
correctly supports indicator-type).

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

[Dan Beam, 2017-01-12 18:18:18]

I'm super glad we're adding this test, but a hard dep on settings from ui/ is a
little funky. where are these elements instantiated again? always in
chrome/browser/resources?

https://codereview.chromium.org/2625213002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_elements_browsertest.js (right):

https://codereview.chromium.org/2625213002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:174:
'../../../../../third_party/closure_compiler/externs/settings_private.js',
actually running externs is a poor practice and is not guaranteed to work in the
future.

for example, chrome_extensions.js does this:

if (Math.random() < 1) {  // always succeeds
  throw "don't load externs as script";
}

https://cs.chromium.org/chromium/src/third_party/closure_compiler/externs/chr...

this seems like a good practice and might come to a externs generator near you
soon ;)

additionally, this means that the code relies on settingsPrivate tightly :(

https://codereview.chromium.org/2625213002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_policy_network_indicator_tests.js
(right):

https://codereview.chromium.org/2625213002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_policy_network_indicator_tests.js:7:
function registerTests() {
can you just drop these 2 lines? pretty much all new tests don't do this

https://codereview.chromium.org/2625213002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_policy_network_indicator_tests.js:31:
test('none', function() {
can we name this 'hidden by default' or something slightly more descriptive?

https://codereview.chromium.org/2625213002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_policy_network_indicator_tests.js:32:
assertTrue(indicator.$.indicator.hidden);
can you create an accessor to this.$.indicator or specific attributes like
.hidden and .icon

this.$.* is basically internal state, and tests should strive to test public
state where possible. this makes refactoring easier and tests less brittle.

[stevenjb, 2017-01-12 18:41:32]

Dan could you review the dependent CL,
https://codereview.chromium.org/2624003003, first? It has similar issues but is
slightly more constrained.

The policy code relies on settingsPrivate (and networkingPrivate), but these are
extension APIs, and while settingsPrivate has the name "settings" in it, it's
not specific to the "settings UI" per-se (i.e. we may use it in oobe/login and
someday maybe 'help' on chrome os). That's why these elements were placed in
cr_elements.

That said, is there a correct way to include these? We need access to the enums
defined in the externs.

https://codereview.chromium.org/2625213002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_policy_network_indicator_tests.js
(right):

https://codereview.chromium.org/2625213002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_policy_network_indicator_tests.js:7:
function registerTests() {
On 2017/01/12 18:18:18, Dan Beam wrote:
> can you just drop these 2 lines? pretty much all new tests don't do this

Done.

https://codereview.chromium.org/2625213002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_policy_network_indicator_tests.js:31:
test('none', function() {
On 2017/01/12 18:18:18, Dan Beam wrote:
> can we name this 'hidden by default' or something slightly more descriptive?

Done.

https://codereview.chromium.org/2625213002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_policy_network_indicator_tests.js:32:
assertTrue(indicator.$.indicator.hidden);
On 2017/01/12 18:18:18, Dan Beam wrote:
> can you create an accessor to this.$.indicator or specific attributes like
> .hidden and .icon
> 
> this.$.* is basically internal state, and tests should strive to test public
> state where possible. this makes refactoring easier and tests less brittle.

Apologies for being dense, but I don't quite follow, can you give me an example?
Would $$('#indicator') be better, or would you like to see that pattern removed
also?

[Dan Beam, 2017-01-18 01:57:57]

https://codereview.chromium.org/2625213002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_policy_network_indicator_tests.js
(right):

https://codereview.chromium.org/2625213002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_policy_network_indicator_tests.js:32:
assertTrue(indicator.$.indicator.hidden);
On 2017/01/12 18:41:31, stevenjb wrote:
> On 2017/01/12 18:18:18, Dan Beam wrote:
> > can you create an accessor to this.$.indicator or specific attributes like
> > .hidden and .icon
> > 
> > this.$.* is basically internal state, and tests should strive to test public
> > state where possible. this makes refactoring easier and tests less brittle.
> 
> Apologies for being dense, but I don't quite follow, can you give me an
example?
> Would $$('#indicator') be better, or would you like to see that pattern
removed
> also? 

Polymer({
  is: '...',
  get icon() {
    return this.$.indicator.icon;
  },
  get indicatorVisible() {
    return !this.$.indicator.hidden;
  },
})

[stevenjb, 2017-03-08 22:49:28]

PTAL

[Dan Beam, 2017-03-09 00:57:45]

i don't really know what changed since last time when i was on about something
with using chrome/ from ui/, but I don't see that now, so lgtm

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

https://codereview.chromium.org/2625213002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:45:
cr-policy-network-indicator {
does this need another element in this selector?

https://codereview.chromium.org/2625213002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_elements_browsertest.js (right):

https://codereview.chromium.org/2625213002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:229: 
we might as well be consistent and add this fluffy @constructor and @extends
stuff or something

[stevenjb, 2017-03-09 01:12:24]

I removed externs/settings_private.js and externs/networking_private.js from
extraLibraries; they are no longer required after some other changes for
CrElementsPolicyPrefIndicatorTest.

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

https://codereview.chromium.org/2625213002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:45:
cr-policy-network-indicator {
On 2017/03/09 00:57:45, Dan Beam wrote:
> does this need another element in this selector?

Ah, yes, thanks. Done.

https://codereview.chromium.org/2625213002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_elements_browsertest.js (right):

https://codereview.chromium.org/2625213002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:229: 
On 2017/03/09 00:57:45, Dan Beam wrote:
> we might as well be consistent and add this fluffy @constructor and @extends
> stuff or something

Oops, I think they got lost in the merge. Done.

[stevenjb, 2017-03-09 01:34:40]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-03-09 01:34:41]

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/2625213002/#ps100001
(title: "Feedback")

[commit-bot: I haz the power, 2017-03-09 01:35:31]

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-03-09 02:43:01]

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

[commit-bot: I haz the power, 2017-03-09 02:43:02]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[stevenjb, 2017-03-09 18:34:02]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-03-09 18:34:03]

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/2625213002/#ps120001
(title: "Disable for not CrOS")

[commit-bot: I haz the power, 2017-03-09 18:34:35]

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-03-09 19:37:46]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489084442749460,
"parent_rev": "fbf043b1bde50e64a940da7afcc6ec3a4abb3dbd", "commit_rev":
"0c3256d22e6966598c8d5cd101cd7e827b0f42b9"}

[commit-bot: I haz the power, 2017-03-09 19:38:29]

Description was changed from

==========
WebUI: Add cr_policy_network_indicator_tests.js

This also makes indicatorType private for cr-policy-network-indicator
and switches existing usage to cr-policy-pref-indicator (which now
correctly supports indicator-type).

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

to

==========
WebUI: Add cr_policy_network_indicator_tests.js

This also makes indicatorType private for cr-policy-network-indicator
and switches existing usage to cr-policy-pref-indicator (which now
correctly supports indicator-type).

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

Review-Url: https://codereview.chromium.org/2625213002
Cr-Commit-Position: refs/heads/master@{#455830}
Committed:
https://chromium.googlesource.com/chromium/src/+/0c3256d22e6966598c8d5cd101cd...
==========

[commit-bot: I haz the power, 2017-03-09 19:38:31]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/0c3256d22e6966598c8d5cd101cd...