MD Settings: Fix "Check for updates" button regression.

MD Settings: Fix "Check for updates" button regression.

Had to move "if expr" conditional logic to JS from HTML, such that
the visibility of the three buttons can be easily checked by the parent
container (which needs to be hidden when all of its children are hidden).

This CL also fixes the issue where the update status message was
erroneously visible before the user had cliced on the "Check for
updates" button.

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

Committed: https://crrev.com/d47dc0a551b0fedc4cd58db1498426405dd2d044
Cr-Commit-Position: refs/heads/master@{#436851}

[dpapad, 2016-12-01 01:06:24]

Description was changed from

==========
MD Settings: Fix "Check for updates" button regression.

BUG=669259
==========

to

==========
MD Settings: Fix "Check for updates" button regression.

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

[dpapad, 2016-12-01 01:06:41]

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

[commit-bot: I haz the power, 2016-12-01 01:07:51]

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

[dpapad, 2016-12-01 01:12:33]

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

[commit-bot: I haz the power, 2016-12-01 01:13:08]

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

[dpapad, 2016-12-01 01:17:03]

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

[dpapad, 2016-12-01 01:17:03]

The "check for updates" button regressed at
https://codereview.chromium.org/2387053004, because the |hidden| binding was
moved to |relaunchContainer|. A test caught that breakage, but then it was also
updated (I think) incorrectly.

I have reverted those changes in this CL, but it is not clear to me if reverting
them has any effect on the style tweaks originally attempted by 2387053004. Was
the relaunchContainer related changes related to the style tweaks? Did a sanity
check and did not see any obvious issue.

[commit-bot: I haz the power, 2016-12-01 02:17:39]

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

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

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-12-01 21:52:50]

On 2016/12/01 at 01:17:03, dpapad wrote:
> The "check for updates" button regressed at
https://codereview.chromium.org/2387053004, because the |hidden| binding was
moved to |relaunchContainer|. A test caught that breakage, but then it was also
updated (I think) incorrectly.
> 
> I have reverted those changes in this CL, but it is not clear to me if
reverting them has any effect on the style tweaks originally attempted by
2387053004. Was the relaunchContainer related changes related to the style
tweaks? Did a sanity check and did not see any obvious issue.

Friendly ping.

[dschuyler, 2016-12-02 21:26:43]

I'm concerned about moving the hidden: in other cases similar to this change,
there have been weird blank spaces or empty frames left behind when a narrow
thing is hidden. Could you screen shot or demo what happens in the
hidden/non-hidden states? I'd just like to know that there is not odd looking
remnants in the UI.

[dpapad, 2016-12-03 02:27:41]

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

[commit-bot: I haz the power, 2016-12-03 02:27:58]

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

[dpapad, 2016-12-03 02:30:21]

Description was changed from

==========
MD Settings: Fix "Check for updates" button regression.

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

to

==========
MD Settings: Fix "Check for updates" button regression.

Had to move "if expr" conditional logic to JS from HTML, such that
the visibility of the three buttons can be easily checked by the parent
container (which needs to be hidden when all of its children are hidden).

This CL also fixes the issue where the update status message was
erroneously visible before the user had cliced on the "Check for
updates" button.

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

[dpapad, 2016-12-03 02:33:18]

On 2016/12/02 at 21:26:43, dschuyler wrote:
> I'm concerned about moving the hidden: in other cases similar to this change,
there have been weird blank spaces or empty frames left behind when a narrow
thing is hidden. Could you screen shot or demo what happens in the
hidden/non-hidden states? I'd just like to know that there is not odd looking
remnants in the UI.

Done. I had to make quite a few more changes, to ensure that your concern was
addressed. Updated the CL description with more info, to reflect the latest
state of this CL. Expecting green tests (ran them locally already).

[commit-bot: I haz the power, 2016-12-03 02:37:50]

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

[commit-bot: I haz the power, 2016-12-03 02:37:51]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[dpapad, 2016-12-05 22:00:39]

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

[commit-bot: I haz the power, 2016-12-05 22:01:25]

Dry run: 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 22:16:42]

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

[commit-bot: I haz the power, 2016-12-05 22:16:43]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[dpapad, 2016-12-05 22:38:19]

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

[commit-bot: I haz the power, 2016-12-05 22:38:45]

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

[dpapad, 2016-12-05 22:43:26]

Patchset #5 (id:80001) has been deleted

[dpapad, 2016-12-05 22:43:30]

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

[dpapad, 2016-12-05 22:44:04]

Patchset #5 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-12-05 22:44:36]

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

[dpapad, 2016-12-05 22:45:09]

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

[commit-bot: I haz the power, 2016-12-05 22:45:53]

Dry run: 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 23:49:32]

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

[commit-bot: I haz the power, 2016-12-05 23:49:33]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-12-05 23:59:47]

https://codereview.chromium.org/2538283002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2538283002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:52: computed:
(function() {
This is unfortunately necessary to make the Closure compiler happy. During
compilation we simply remove the "if expr" lines, so the compiler starts
returning bogus errors, that do not apply in the code that actually gets
executed at runtime.

[Dan Beam, 2016-12-06 00:16:50]

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

[Dan Beam, 2016-12-06 00:16:51]

https://codereview.chromium.org/2538283002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2538283002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:52: computed:
(function() {
On 2016/12/05 23:59:47, dpapad wrote:
> This is unfortunately necessary to make the Closure compiler happy. During
> compilation we simply remove the "if expr" lines, so the compiler starts
> returning bogus errors, that do not apply in the code that actually gets
> executed at runtime.

why can't you just make different member names, i.e.

showUpdateStatusDesktop_
showUpdateStatusCros_

with different (or the same) handler?

[Dan Beam, 2016-12-06 00:17:26]

https://codereview.chromium.org/2538283002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2538283002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:52: computed:
(function() {
On 2016/12/06 00:16:51, Dan Beam wrote:
> On 2016/12/05 23:59:47, dpapad wrote:
> > This is unfortunately necessary to make the Closure compiler happy. During
> > compilation we simply remove the "if expr" lines, so the compiler starts
> > returning bogus errors, that do not apply in the code that actually gets
> > executed at runtime.
> 
> why can't you just make different member names, i.e.
> 
> showUpdateStatusDesktop_
> showUpdateStatusCros_
> 
> with different (or the same) handler?

we also talked about this:
https://gist.github.com/danbeam/3ed90de431cdbdcdc13207d4b12e72a2

[dpapad, 2016-12-06 00:23:41]

https://codereview.chromium.org/2538283002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2538283002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:52: computed:
(function() {
On 2016/12/06 at 00:17:26, Dan Beam wrote:
> On 2016/12/06 00:16:51, Dan Beam wrote:
> > On 2016/12/05 23:59:47, dpapad wrote:
> > > This is unfortunately necessary to make the Closure compiler happy. During
> > > compilation we simply remove the "if expr" lines, so the compiler starts
> > > returning bogus errors, that do not apply in the code that actually gets
> > > executed at runtime.
> > 
> > why can't you just make different member names, i.e.

If I understand the suggestion, that would defeat the purpose. Explaining below.

Before those <if expr> statements where in HTML, and that worked fine, since
Closer Compiler does not care about HTML files. In this CL I had to move them in
JS, to make it more convenient to use them from HTML, otherwise I would have to
repeat the same verbose bindings multiple times (specifically, buttonContainer's
hidden re-uses the computed bindings for all of its children). That was not
possible if the <if expr> statements stayed in JS.

I hope that makes sense, but if it does not, I would be happy to explain this in
person.

[Dan Beam, 2016-12-06 00:29:44]

https://codereview.chromium.org/2538283002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2538283002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:52: computed:
(function() {
On 2016/12/06 00:23:41, dpapad wrote:
> On 2016/12/06 at 00:17:26, Dan Beam wrote:
> > On 2016/12/06 00:16:51, Dan Beam wrote:
> > > On 2016/12/05 23:59:47, dpapad wrote:
> > > > This is unfortunately necessary to make the Closure compiler happy.
During
> > > > compilation we simply remove the "if expr" lines, so the compiler starts
> > > > returning bogus errors, that do not apply in the code that actually gets
> > > > executed at runtime.
> > > 
> > > why can't you just make different member names, i.e.
> 
> If I understand the suggestion, that would defeat the purpose. Explaining
below.
> 
> Before those <if expr> statements where in HTML, and that worked fine, since
> Closer Compiler does not care about HTML files. In this CL I had to move them
in
> JS, to make it more convenient to use them from HTML, otherwise I would have
to
> repeat the same verbose bindings multiple times (specifically,
buttonContainer's
> hidden re-uses the computed bindings for all of its children). That was not
> possible if the <if expr> statements stayed in JS.
> 
> I hope that makes sense, but if it does not, I would be happy to explain this
in
> person.

then make them observers instead of computed properties

[dpapad, 2016-12-06 01:56:59]

> then make them observers instead of computed properties

Done.

[Dan Beam, 2016-12-06 02:03:18]

https://codereview.chromium.org/2538283002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2538283002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:98: </if>
can put each observer into a single if here?  i.e.

<if expr="chromeos">
  'updateShowUpdateStatus_(' +
      'obsoleteSystemInfo_, currentUpdateStatusEvent_,' +
      'hasCheckedForUpdates_)',
  'updateShowRelaunch_(currentUpdateStatusEvent_, targetChannel_)',
  'updateShowButtonContainer_(' +
      'showRelaunch_, showRelaunchAndPowerwash_,showCheckUpdates_)',
</if>

<if expr="not chromeos">
  'updateShowUpdateStatus_(' +
      'obsoleteSystemInfo_, currentUpdateStatusEvent_)',
  'updateShowRelaunch_(currentUpdateStatusEvent_)',
  'updateShowButtonContainer_(showRelaunch_)',
</if>

https://codereview.chromium.org/2538283002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:194:
this.showRelaunchAndPowerwash_ || this.showCheckUpdates_;
can you avoid setting this twice for the sake of observers?

var showButtonContainer = this.showRelaunch_;
<if expr="chromeos">
if (this.showRelaunchAndPowerwash_ || this.showCheckUpdates_)
  showButtonContainer = true;
</if>
this.showButtonContainer_ = showButtonContainer;

https://codereview.chromium.org/2538283002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:206: </if>
var showRelaunch = this.checkStatus_(UpdateStatus.NEARLY_UPDATED);
<if expr="chromeos">
if (this.isTargetChannelMoreStable_()) showRelaunch = false;
</if>
this.showRelaunch_ = showRelaunch;

[dpapad, 2016-12-06 16:59:58]

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

[commit-bot: I haz the power, 2016-12-06 17:00:31]

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

[dpapad, 2016-12-06 17:00:40]

https://codereview.chromium.org/2538283002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2538283002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:98: </if>
On 2016/12/06 at 02:03:18, Dan Beam wrote:
> can put each observer into a single if here?  i.e.
> 
> <if expr="chromeos">
>   'updateShowUpdateStatus_(' +
>       'obsoleteSystemInfo_, currentUpdateStatusEvent_,' +
>       'hasCheckedForUpdates_)',
>   'updateShowRelaunch_(currentUpdateStatusEvent_, targetChannel_)',
>   'updateShowButtonContainer_(' +
>       'showRelaunch_, showRelaunchAndPowerwash_,showCheckUpdates_)',
> </if>
> 
> <if expr="not chromeos">
>   'updateShowUpdateStatus_(' +
>       'obsoleteSystemInfo_, currentUpdateStatusEvent_)',
>   'updateShowRelaunch_(currentUpdateStatusEvent_)',
>   'updateShowButtonContainer_(showRelaunch_)',
> </if>

Done.

https://codereview.chromium.org/2538283002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:194:
this.showRelaunchAndPowerwash_ || this.showCheckUpdates_;
On 2016/12/06 at 02:03:18, Dan Beam wrote:
> can you avoid setting this twice for the sake of observers?
> 
> var showButtonContainer = this.showRelaunch_;
> <if expr="chromeos">
> if (this.showRelaunchAndPowerwash_ || this.showCheckUpdates_)
>   showButtonContainer = true;
> </if>
> this.showButtonContainer_ = showButtonContainer;

Done (avoided setting it twice) , but in a simpler way I think.

https://codereview.chromium.org/2538283002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:206: </if>
On 2016/12/06 at 02:03:18, Dan Beam wrote:
> var showRelaunch = this.checkStatus_(UpdateStatus.NEARLY_UPDATED);
> <if expr="chromeos">
> if (this.isTargetChannelMoreStable_()) showRelaunch = false;
> </if>
> this.showRelaunch_ = showRelaunch;

Acknowledged. I think that completely separating the code for the two platforms
in this case leads to shorter and more readable code.

[commit-bot: I haz the power, 2016-12-06 18:09:11]

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

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

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-12-06 19:10:20]

lgtm w/nits

https://codereview.chromium.org/2538283002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.html (right):

https://codereview.chromium.org/2538283002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.html:119:
hidden="[[!showRelaunchAndPowerwash_]]"
it's kind of confusing because there's something named showRelaunch_, so this
sounds like both relaunch AND a powerwash thing should be showing.

showRelaunchThenPowerwash_ or just showPowerwash_?

https://codereview.chromium.org/2538283002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2538283002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:199: </if>
i would argue that this is still more duplicative than my examples, but whatevs

[dschuyler, 2016-12-06 19:41:21]

LGTM

https://codereview.chromium.org/2538283002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2538283002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:74: observers: [
Maybe a comment about why this is done this way?

https://codereview.chromium.org/2538283002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:88: 'showRelaunch_,
showRelaunchAndPowerwash_,showCheckUpdates_)',
nit: space after comma

[dpapad, 2016-12-06 21:54:00]

https://codereview.chromium.org/2538283002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.html (right):

https://codereview.chromium.org/2538283002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.html:119:
hidden="[[!showRelaunchAndPowerwash_]]"
On 2016/12/06 at 19:10:20, Dan Beam wrote:
> it's kind of confusing because there's something named showRelaunch_, so this
sounds like both relaunch AND a powerwash thing should be showing.
> 
> showRelaunchThenPowerwash_ or just showPowerwash_?

There is a "Relaunch" button and a separate "Relaunch and powerwash" button. The
convention used throughout the code to name things related to those buttons is
to use the English text appearing in the button, for example

HTML id: relaunchAndPowerwash
on-tap handler: onRelaunchAndPowerwashTap_
i18n name: aboutRelaunchAndPowerwash

similarly to

HTML id: relaunch
on-tap handler: onRelaunchTap_
i18n name: aboutRelaunch

My understanding is that following such a convention makes the code more
readable. Are you suggesting to change all three usages above to use "powerwash"
instead of "relaunchAndPowerwash"? FWIW, I am just following the convention
already in this file, so this CL did not invent any new conventions, just
attempts to stay consistent with surrounding code.

https://codereview.chromium.org/2538283002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2538283002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:74: observers: [
On 2016/12/06 at 19:41:21, dschuyler wrote:
> Maybe a comment about why this is done this way?

I tried to think of a helpful comment here, but everything I came up with is
pretty much already documented at updateShowButtonContainer_'s docs a few lines
below.

https://codereview.chromium.org/2538283002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:88: 'showRelaunch_,
showRelaunchAndPowerwash_,showCheckUpdates_)',
On 2016/12/06 at 19:41:21, dschuyler wrote:
> nit: space after comma

Done.

[Dan Beam, 2016-12-07 00:42:42]

still lgtm

[dpapad, 2016-12-07 01:15:41]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-12-07 01:15:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2538283002/#ps180001
(title: "Nit.")

[commit-bot: I haz the power, 2016-12-07 01:16:17]

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-07 03:39:00]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1481073341163550,
"parent_rev": "a8e8fc5451453cdc6076acf0775554f8ca200fb9", "commit_rev":
"d04b7e542b3abb22b7a4d1db1e4d25c60ba33c58"}

[commit-bot: I haz the power, 2016-12-07 03:39:36]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-12-07 03:41:41]

Description was changed from

==========
MD Settings: Fix "Check for updates" button regression.

Had to move "if expr" conditional logic to JS from HTML, such that
the visibility of the three buttons can be easily checked by the parent
container (which needs to be hidden when all of its children are hidden).

This CL also fixes the issue where the update status message was
erroneously visible before the user had cliced on the "Check for
updates" button.

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

to

==========
MD Settings: Fix "Check for updates" button regression.

Had to move "if expr" conditional logic to JS from HTML, such that
the visibility of the three buttons can be easily checked by the parent
container (which needs to be hidden when all of its children are hidden).

This CL also fixes the issue where the update status message was
erroneously visible before the user had cliced on the "Check for
updates" button.

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

Committed: https://crrev.com/d47dc0a551b0fedc4cd58db1498426405dd2d044
Cr-Commit-Position: refs/heads/master@{#436851}
==========

[commit-bot: I haz the power, 2016-12-07 03:41:42]

Patchset 8 (id:??) landed as
https://crrev.com/d47dc0a551b0fedc4cd58db1498426405dd2d044
Cr-Commit-Position: refs/heads/master@{#436851}