MD Settings: Fix timezone autodetect policy indicator.

MD Settings: Fix timezone autodetect policy indicator.

The cr-policy-pref-indicator was referring to a non-existing property, but the
corresponding tests did not catch the bug, because they were asserting on a
condition that was not sufficient. Moreover, the tests did not fire a
'time-zone-auto-detect-policy' WebUI event which is necessary to initialize the
fakeTimeZonePolicyPref_ property, which controls the pref indicator.

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

Review-Url: https://codereview.chromium.org/2683613003
Cr-Commit-Position: refs/heads/master@{#449524}
Committed: https://chromium.googlesource.com/chromium/src/+/74b7a0510c3aca53680a8d65fdc65d384c1d04b6

[dpapad, 2017-02-07 18:10:59]

Description was changed from

==========
MD Settings: Fix timezone autodetect policy indicator.

The cr-policy-pref-indicator was referring to a non-existing property, but the
corresponding tests did not catch the bug, because they were asserting on a
condition that was not sufficient.

BUG=689302
==========

to

==========
MD Settings: Fix timezone autodetect policy indicator.

The cr-policy-pref-indicator was referring to a non-existing property, but the
corresponding tests did not catch the bug, because they were asserting on a
condition that was not sufficient.

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

[dpapad, 2017-02-07 18:11:53]

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

[commit-bot: I haz the power, 2017-02-07 18:12:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-02-07 18:20:12]

Patchset #2 (id:20001) has been deleted

[dpapad, 2017-02-07 18:20:28]

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

[commit-bot: I haz the power, 2017-02-07 18:20:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-02-07 18:27:10]

Description was changed from

==========
MD Settings: Fix timezone autodetect policy indicator.

The cr-policy-pref-indicator was referring to a non-existing property, but the
corresponding tests did not catch the bug, because they were asserting on a
condition that was not sufficient.

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

to

==========
MD Settings: Fix timezone autodetect policy indicator.

The cr-policy-pref-indicator was referring to a non-existing property, but the
corresponding tests did not catch the bug, because they were asserting on a
condition that was not sufficient. Moreover, the tests did not fire a
'time-zone-auto-detect-policy' WebUI event which is necessary to initialize the
fakeTimeZonePolicyPref_ property, which controls the pref indicator.

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

[dpapad, 2017-02-07 18:30:25]

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

[dpapad, 2017-02-07 18:30:26]

https://codereview.chromium.org/2683613003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/date_time_page/date_time_page.html
(right):

https://codereview.chromium.org/2683613003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/date_time_page/date_time_page.html:35:
<cr-policy-pref-indicator pref="[[fakeTimeZonePolicyPref_]]">
We should probably replace this usage of
cr-policy-pref-indicator+paper-togggle-button with a settings-toggle-button, see
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro....
Either way, I'll leave that for a future cleanup, since my priority is to fix
the tests and unblock Vulcanization.

We could also use a browser proxy from date_time_page.js instead of directly
calling chrome.send, but I'll leave that for a future cleanup as well.

[dpapad, 2017-02-07 18:30:59]

On 2017/02/07 at 18:30:26, dpapad wrote:
>
https://codereview.chromium.org/2683613003/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/date_time_page/date_time_page.html
(right):
> 
>
https://codereview.chromium.org/2683613003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/date_time_page/date_time_page.html:35:
<cr-policy-pref-indicator pref="[[fakeTimeZonePolicyPref_]]">
> We should probably replace this usage of
cr-policy-pref-indicator+paper-togggle-button with a settings-toggle-button, see
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro....
Either way, I'll leave that for a future cleanup, since my priority is to fix
the tests and unblock Vulcanization.
> 
> We could also use a browser proxy from date_time_page.js instead of directly
calling chrome.send, but I'll leave that for a future cleanup as well.

After screenshots: http://imgur.com/a/fYW2B.

[commit-bot: I haz the power, 2017-02-07 19:26:55]

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

[commit-bot: I haz the power, 2017-02-07 19:26:55]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2017-02-08 00:07:12]

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/date_time_page_tests.js (left):

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/date_time_page_tests.js:124:
assertGT(indicator.clientHeight, 0);
would indicator.$$('iron-icon').clientHeight be sufficient?

I thought checking for clientHeight was a more robust way to detect whether or
not an element is hidden, because there are a bunch of ways to do so (display:
none, hidden = true, etc.)

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/date_time_page_tests.js (right):

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/date_time_page_tests.js:124:
assertNotEquals('none', indicator.style.display);
generally we should check getComputedStyle, because e.g.

    <cr-policy-pref-indicator hidden>

would result in a computed display of none, but the element's "display" style
property would still be empty.

[dpapad, 2017-02-08 01:53:50]

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/date_time_page_tests.js (left):

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/date_time_page_tests.js:124:
assertGT(indicator.clientHeight, 0);
On 2017/02/08 at 00:07:12, michaelpg wrote:
> would indicator.$$('iron-icon').clientHeight be sufficient?
> 
> I thought checking for clientHeight was a more robust way to detect whether or
not an element is hidden, because there are a bunch of ways to do so (display:
none, hidden = true, etc.)

Actually it is not at all robust. Here is why:

The [hidden] attribute works in custom Polymer elements, only because some
transitive dependency is leaking the following CSS rule 
html /deep/ [hidden] { display: none !important;}
(one of those two
https://cs.chromium.org/search/?q=%22html+/deep/+%5Bhidden%5D%22+file:%5Esrc/...).

You can verify this from the devtools, see http://imgur.com/a/HnzDM, as well as
from the discussion at
https://github.com/Polymer/polymer/issues/3711#issuecomment-226172158.

Our test is loading a specific URL that does not depend on those elements, which
means that applying the hidden property does really nothing in terms of hiding
the iron-icon, and its clientHeight is still 20px (you can also verify this by
applying this CL locally, reverting the one line fix in date_time_page.js and
use console.log here).

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/date_time_page_tests.js (right):

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/date_time_page_tests.js:124:
assertNotEquals('none', indicator.style.display);
On 2017/02/08 at 00:07:12, michaelpg wrote:
> generally we should check getComputedStyle, because e.g.
> 
>     <cr-policy-pref-indicator hidden>
> 
> would result in a computed display of none, but the element's "display" style
property would still be empty.

Tried getComputedStyle() and it throws 

TypeError: Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not
of type 'Element'.
    at value
(chrome://resources/polymer/v1_0/web-animations-js/web-animations-next-lite.min.js:16:12769)
    at verifyPolicy (date_time_page_tests.js:134:32)
    at Context.<anonymous> (date_time_page_tests.js:157:7)
", source: mocha_adapter.js (48)

which is very odd, since |indicator| is a Polymer element. I tried the following
to see
console.log(indicator instanceof Element); // false
console.log(indicator instanceof HTMLElement); // false
console.log(indicator.appendChild) // function appendChild() { [native code] }

I don't know why this is happening, but I'd rather not block this CL on it.

[Dan Beam, 2017-02-08 02:14:21]

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

[Dan Beam, 2017-02-08 02:14:23]

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/date_time_page_tests.js (right):

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/date_time_page_tests.js:125:
assertTrue(!!indicator.$$('iron-icon'));
why are we looking into indicator itself?

[michaelpg, 2017-02-08 02:24:25]

lgtm

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/date_time_page_tests.js (left):

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/date_time_page_tests.js:124:
assertGT(indicator.clientHeight, 0);
On 2017/02/08 01:53:50, dpapad wrote:
> On 2017/02/08 at 00:07:12, michaelpg wrote:
> > would indicator.$$('iron-icon').clientHeight be sufficient?
> > 
> > I thought checking for clientHeight was a more robust way to detect whether
or
> not an element is hidden, because there are a bunch of ways to do so (display:
> none, hidden = true, etc.)
> 
> Actually it is not at all robust. Here is why:
> 
> The [hidden] attribute works in custom Polymer elements, only because some
> transitive dependency is leaking the following CSS rule 
> html /deep/ [hidden] { display: none !important;}
> (one of those two
>
https://cs.chromium.org/search/?q=%22html+/deep/+%5Bhidden%5D%22+file:%5Esrc/...).
> 
> You can verify this from the devtools, see http://imgur.com/a/HnzDM, as well
as
> from the discussion at
> https://github.com/Polymer/polymer/issues/3711#issuecomment-226172158.
> 
> Our test is loading a specific URL that does not depend on those elements,
which
> means that applying the hidden property does really nothing in terms of hiding
> the iron-icon, and its clientHeight is still 20px (you can also verify this by
> applying this CL locally, reverting the one line fix in date_time_page.js and
> use console.log here).

Jeez. Can't wait to get rid of those iron-flex global hidden attributes.

Can you add a TODO to replace these checks with clientHeight-type checks,
blocked on 635633 and/or 498405?

Thanks.

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/date_time_page_tests.js (right):

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/date_time_page_tests.js:124:
assertNotEquals('none', indicator.style.display);
On 2017/02/08 01:53:50, dpapad wrote:
> On 2017/02/08 at 00:07:12, michaelpg wrote:
> > generally we should check getComputedStyle, because e.g.
> > 
> >     <cr-policy-pref-indicator hidden>
> > 
> > would result in a computed display of none, but the element's "display"
style
> property would still be empty.
> 
> Tried getComputedStyle() and it throws 
> 
> TypeError: Failed to execute 'getComputedStyle' on 'Window': parameter 1 is
not
> of type 'Element'.
>     at value
>
(chrome://resources/polymer/v1_0/web-animations-js/web-animations-next-lite.min.js:16:12769)
>     at verifyPolicy (date_time_page_tests.js:134:32)
>     at Context.<anonymous> (date_time_page_tests.js:157:7)
> ", source: mocha_adapter.js (48)
> 
> which is very odd, since |indicator| is a Polymer element. I tried the
following
> to see
> console.log(indicator instanceof Element); // false
> console.log(indicator instanceof HTMLElement); // false
> console.log(indicator.appendChild) // function appendChild() { [native code] }
> 
> I don't know why this is happening, but I'd rather not block this CL on it.

Yeah I'm at a loss there.

[dpapad, 2017-02-08 02:26:35]

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/date_time_page_tests.js (right):

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/date_time_page_tests.js:125:
assertTrue(!!indicator.$$('iron-icon'));
On 2017/02/08 at 02:14:23, Dan Beam wrote:
> why are we looking into indicator itself?

In lack of a better way to verify if the on/off state of the policy pref had any
effect on the DOM. cr-policy-pref-indicator does not expose that info (for
example via an attribute) AND does not have unit tests.

Summarizing offline discussions we have 3 options
1) Update cr-policy-pref-indicator to expose and |active| HTML attribute. Assert
on that. Add unit tests for cr-policy-pref-indicator.
2) Migrate to settings-toggle (if possible), which already exposes a |checked|
attribute (and has unit tests). Assert on the |checked| attribute here, avoiding
querying any private state.
3) Let this CL through, since it fixes a bug, improves broken tests, and allows
me to stay focused on the vulcanized stuff, instead of going of course.

[dpapad, 2017-02-08 19:18:35]

Addressing dbeam's testing concerns. PTAL. Thanks.

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/date_time_page_tests.js (right):

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/date_time_page_tests.js:125:
assertTrue(!!indicator.$$('iron-icon'));
> Summarizing offline discussions we have 3 options
> 1) Update cr-policy-pref-indicator to expose and |active| HTML attribute.
Assert on that. Add unit tests for cr-policy-pref-indicator.
> 2) Migrate to settings-toggle (if possible), which already exposes a |checked|
attribute (and has unit tests). Assert on the |checked| attribute here, avoiding
querying any private state.
> 3) Let this CL through, since it fixes a bug, improves broken tests, and
allows me to stay focused on the vulcanized stuff, instead of going of course.

@dbeam PTAL: I added an isActive() method on cr-policy-pref-indicator. I also
added a "restamp" on the surrounding dom-if so that we don't need to check for
display or the presence of the element. This simplifies things a lot, and
addresses your concerns of querying private state during tests (no need to deal
with iron-icons anymore).

FWIW, I also tried exposing an |active| HTML attribute, instead of the
isActive() method, but the former was problematic for multiple reasons.
 - The attribute is unnecessary calculated even if nobody is querying for it.
 - The attribute is editable from the outside, but the indicator state should
only be dictated by the pref.
For those reasons the isActive() method seems like the best approach.

[Dan Beam, 2017-02-08 21:45:49]

lgtm

[dpapad, 2017-02-08 21:54:06]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2017-02-08 21:54:07]

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/2683613003/#ps60001
(title: "Use getter instead of private state.")

[dpapad, 2017-02-08 21:54:16]

The CQ bit was unchecked by dpapad@chromium.org

[dpapad, 2017-02-08 22:03:40]

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/date_time_page_tests.js (left):

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/date_time_page_tests.js:124:
assertGT(indicator.clientHeight, 0);
 > Can you add a TODO to replace these checks with clientHeight-type checks,
blocked on 635633 and/or 498405?
> 
> Thanks.

Michael do you still this TODO is worth it? The code now uses a dedicated
cr-policy-pref-indicator#isActive() method, so I don't think we need to revert
to checking clientHeight in the future. (We should add unit tests for
cr-pref-policy-indicator though.)

[dpapad, 2017-02-09 18:10:00]

On 2017/02/08 at 22:03:40, dpapad wrote:
>
https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
> File chrome/test/data/webui/settings/date_time_page_tests.js (left):
> 
>
https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
> chrome/test/data/webui/settings/date_time_page_tests.js:124:
assertGT(indicator.clientHeight, 0);
>  > Can you add a TODO to replace these checks with clientHeight-type checks,
blocked on 635633 and/or 498405?
> > 
> > Thanks.
> 
> Michael do you still this TODO is worth it? The code now uses a dedicated
cr-policy-pref-indicator#isActive() method, so I don't think we need to revert
to checking clientHeight in the future. (We should add unit tests for
cr-pref-policy-indicator though.)

Michael, friendly ping about the TODO question above.

[michaelpg, 2017-02-09 22:09:02]

slgtm

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/date_time_page_tests.js (left):

https://codereview.chromium.org/2683613003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/date_time_page_tests.js:124:
assertGT(indicator.clientHeight, 0);
On 2017/02/08 22:03:40, dpapad wrote:
>  > Can you add a TODO to replace these checks with clientHeight-type checks,
> blocked on 635633 and/or 498405?
> > 
> > Thanks.
> 
> Michael do you still this TODO is worth it? The code now uses a dedicated
> cr-policy-pref-indicator#isActive() method, so I don't think we need to revert
> to checking clientHeight in the future. (We should add unit tests for
> cr-pref-policy-indicator though.)

SGTM.

[dpapad, 2017-02-09 23:00:04]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-02-09 23:00: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-02-10 00:59:48]

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

[commit-bot: I haz the power, 2017-02-10 00:59:49]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dpapad, 2017-02-10 01:44:57]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-02-10 01:45:22]

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-02-10 02:29:38]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486691097590990,
"parent_rev": "5321deced002ddaf841dec0b1d797f836495e84f", "commit_rev":
"74b7a0510c3aca53680a8d65fdc65d384c1d04b6"}

[commit-bot: I haz the power, 2017-02-10 02:30:17]

Description was changed from

==========
MD Settings: Fix timezone autodetect policy indicator.

The cr-policy-pref-indicator was referring to a non-existing property, but the
corresponding tests did not catch the bug, because they were asserting on a
condition that was not sufficient. Moreover, the tests did not fire a
'time-zone-auto-detect-policy' WebUI event which is necessary to initialize the
fakeTimeZonePolicyPref_ property, which controls the pref indicator.

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

to

==========
MD Settings: Fix timezone autodetect policy indicator.

The cr-policy-pref-indicator was referring to a non-existing property, but the
corresponding tests did not catch the bug, because they were asserting on a
condition that was not sufficient. Moreover, the tests did not fire a
'time-zone-auto-detect-policy' WebUI event which is necessary to initialize the
fakeTimeZonePolicyPref_ property, which controls the pref indicator.

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

Review-Url: https://codereview.chromium.org/2683613003
Cr-Commit-Position: refs/heads/master@{#449524}
Committed:
https://chromium.googlesource.com/chromium/src/+/74b7a0510c3aca53680a8d65fdc6...
==========

[commit-bot: I haz the power, 2017-02-10 02:30:18]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/74b7a0510c3aca53680a8d65fdc6...