MD Settings: About page, updating buttons based on current update status.

MD Settings: About page, updating buttons based on current update status.

 - For ChromeOS there are three buttons, "Check for updates", "Restart"
   and "Restart and Powerwash".
 - For non-Chrome OS there is only one "Relaunch" button.

BUG=546841, 603625
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/2c82e33281c61d660697a56bb337c598e487ef0a
Cr-Commit-Position: refs/heads/master@{#395398}

[dpapad, 2016-05-19 22:13:42]

Description was changed from

==========
MD Settings: About page, updating buttons based on current state, WIP.

BUG=546841,603625
==========

to

==========
MD Settings: About page, updating buttons based on current state, WIP.

BUG=546841,603625
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-05-20 22:20:02]

Patchset #4 (id:60001) has been deleted

[dpapad, 2016-05-20 22:20:06]

Patchset #3 (id:40001) has been deleted

[dpapad, 2016-05-20 22:20:10]

Patchset #2 (id:20001) has been deleted

[dpapad, 2016-05-20 22:20:13]

Patchset #1 (id:1) has been deleted

[dpapad, 2016-05-20 22:26:47]

Description was changed from

==========
MD Settings: About page, updating buttons based on current state, WIP.

BUG=546841,603625
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: About page, updating buttons based on current state.

 - For ChromeOS there are three buttons, "Check for updates", "Restart"
   and "Restart and Powerwash".
 - For non-Chrome OS there is only one "Relaunch" button.

BUG=546841,603625
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-05-20 22:26:47]

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

[dpapad, 2016-05-20 22:28:36]

Description was changed from

==========
MD Settings: About page, updating buttons based on current state.

 - For ChromeOS there are three buttons, "Check for updates", "Restart"
   and "Restart and Powerwash".
 - For non-Chrome OS there is only one "Relaunch" button.

BUG=546841,603625
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: About page, updating buttons based on current update status.

 - For ChromeOS there are three buttons, "Check for updates", "Restart"
   and "Restart and Powerwash".
 - For non-Chrome OS there is only one "Relaunch" button.

BUG=546841,603625
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2016-05-20 22:53:19]

dpapad: here you go. No structural issues. pretty much just nits.

https://codereview.chromium.org/1997453006/diff/80001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1997453006/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:36: <message
name="IDS_SETTINGS_ABOUT_PAGE_RELAUNCH_AND_POWERWASH" desc="The label for the 
button that relaunches and powerwashes the browser once update is complete">
nit: I assume you are trying to keep these alphabetical by IDR? If so, these
might have to go up a few entries.

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:27: /** private */
nit: @private {boolean} ?

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:117: var shouldShow =
false;
nit: eliminate var in favor of direct returns?

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:154: return
this.currentUpdateStatusEvent_.message || '';
Looking at the C++ handler, is it possible for the event to be defined but the
message to be undefined?

I have two questions:

What happens when you access a property of an undefined object? Does it also
give undefined? or is it an exception?

Also - do computed bindings get called while the object is undefined? I thought
I heard it didn't, but i'm not sure.

This is also kind of a nitty comment, i'm sure this works just fine.

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:195:
isTargetChannelMoreStable_: function() {
should this have some asserts at the top that the current and target channels
are non-empty defined strings?

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/about_page_browser_proxy.js
(right):

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:57: *  
progress: (number|undefined),
Looking at the C++ handler, these are always defined right?

Is this only to accommodate the test code? Maybe not worth.

https://codereview.chromium.org/1997453006/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/about_page_tests.js (right):

https://codereview.chromium.org/1997453006/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:123: function initNewPage()
{
I assume you have to re-init the page because the about page would never expect
the channel to change while it's open right?

https://codereview.chromium.org/1997453006/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:259: } else {
Nit: since this is so far down, would it be more readable to make this a
separate if (!cr.isChromeOS)?

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

https://codereview.chromium.org/1997453006/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:75:
window.setUpdateStatus = this.onUpdateStatusChanged_.bind(this);
Was this a leftover also?

[dpapad, 2016-05-20 23:24:13]

Patchset #3 (id:120001) has been deleted

[dpapad, 2016-05-20 23:25:36]

Thanks for the speedy review!

https://codereview.chromium.org/1997453006/diff/80001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1997453006/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:36: <message
name="IDS_SETTINGS_ABOUT_PAGE_RELAUNCH_AND_POWERWASH" desc="The label for the 
button that relaunches and powerwashes the browser once update is complete">
On 2016/05/20 at 22:53:18, tommycli wrote:
> nit: I assume you are trying to keep these alphabetical by IDR? If so, these
might have to go up a few entries.

Done. Honestly I've given up of trying to make grdp files sorted. It gets
complicated when you deal with multiple platforms and chromium/chrome string
variations.

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:27: /** private */
On 2016/05/20 at 22:53:18, tommycli wrote:
> nit: @private {boolean} ?

I've been told in other code reviews that the compiler's Polymer pass
understands the type from the following line, so only @private is needed here.

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:117: var shouldShow =
false;
On 2016/05/20 at 22:53:18, tommycli wrote:
> nit: eliminate var in favor of direct returns?

Can't avoid the variable here, that was my first version. The compiler does not
understand <if> statements, all it does is to ignore them, so if I do direct
returns, I get the "unreachadle code" error because all the compiler sees is

return 'foo';
return 'bar'; // unreachable code

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:154: return
this.currentUpdateStatusEvent_.message || '';
On 2016/05/20 at 22:53:18, tommycli wrote:
> Looking at the C++ handler, is it possible for the event to be defined but the
message to be undefined?
> 
> I have two questions:
> 
> What happens when you access a property of an undefined object? Does it also
give undefined? or is it an exception?
> 
> Also - do computed bindings get called while the object is undefined? I
thought I heard it didn't, but i'm not sure.
> 
> This is also kind of a nitty comment, i'm sure this works just fine.

The C++ is actually passing an empty string whenever the "message" does not
apply to the current state. AFAIU UpdateStatus.FAILED state is the only state
where "message" is not empty. But, I changed the typedef in the proxy to declare
the message as optional, such that I don't have to repeat this meaningless field
when I create an UpdateStatusEvent payload myself (either in tests, or at line
230 here.

Accessing an undefined member of an object simply returns undefined. You can try
in the console
var a = {};
console.log(a.foo);

Regarding computed bindings, nothing depends on
this.currentUpdateStatusEvent_.message. All bindings depend on
this.currentUpdateStatusEvent_ itself, and the code never modifies a subproperty
of that member variable, it is always re-assigned as a whole.

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:195:
isTargetChannelMoreStable_: function() {
On 2016/05/20 at 22:53:18, tommycli wrote:
> should this have some asserts at the top that the current and target channels
are non-empty defined strings?

Done.

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/about_page_browser_proxy.js
(right):

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:57: *  
progress: (number|undefined),
On 2016/05/20 at 22:53:18, tommycli wrote:
> Looking at the C++ handler, these are always defined right?
> 
> Is this only to accommodate the test code? Maybe not worth.

Test code but also prod code (see about_page.js where onUpdateStatusChanged_ is
called manually once). But also I think this is better reflecting that "message"
is only meaningful sometimes. Otherwise this fact is very implicit and the
developer is surprised by seeing empty strings in most cases. Ideally I would
change the C++ to only pass it when it is non-empty, but I consider the priority
of this little cleanup lower than getting the about page to a functional state.

https://codereview.chromium.org/1997453006/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/about_page_tests.js (right):

https://codereview.chromium.org/1997453006/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:123: function initNewPage()
{
On 2016/05/20 at 22:53:18, tommycli wrote:
> I assume you have to re-init the page because the about page would never
expect the channel to change while it's open right?

Right, so far the channel is only queried when page loads and does not change.
I'll look more into this once I implement the "channel switcher".

https://codereview.chromium.org/1997453006/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:259: } else {
On 2016/05/20 at 22:53:19, tommycli wrote:
> Nit: since this is so far down, would it be more readable to make this a
separate if (!cr.isChromeOS)?

Done.

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

https://codereview.chromium.org/1997453006/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:75:
window.setUpdateStatus = this.onUpdateStatusChanged_.bind(this);
On 2016/05/20 at 22:53:19, tommycli wrote:
> Was this a leftover also?

Ah yes. Removed.

[tommycli, 2016-05-20 23:32:29]

lgtm thanks for the explanations.

One more nit below.

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:27: /** private */
On 2016/05/20 23:25:35, dpapad wrote:
> On 2016/05/20 at 22:53:18, tommycli wrote:
> > nit: @private {boolean} ?
> 
> I've been told in other code reviews that the compiler's Polymer pass
> understands the type from the following line, so only @private is needed here.

Sounds good. I think you still need to add an "@" character then.

https://codereview.chromium.org/1997453006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:117: var shouldShow =
false;
On 2016/05/20 23:25:35, dpapad wrote:
> On 2016/05/20 at 22:53:18, tommycli wrote:
> > nit: eliminate var in favor of direct returns?
> 
> Can't avoid the variable here, that was my first version. The compiler does
not
> understand <if> statements, all it does is to ignore them, so if I do direct
> returns, I get the "unreachadle code" error because all the compiler sees is
> 
> return 'foo';
> return 'bar'; // unreachable code

Ahh... that makes sense. Thanks for the clarification.

[dpapad, 2016-05-20 23:44:20]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-05-20 23:44:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/1997453006/#ps160001
(title: "@private")

[commit-bot: I haz the power, 2016-05-20 23:44:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1997453006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1997453006/160001

[commit-bot: I haz the power, 2016-05-21 02:26:40]

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

[commit-bot: I haz the power, 2016-05-21 02:26:42]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dpapad, 2016-05-23 17:02:13]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-05-23 17:02:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1997453006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1997453006/160001

[commit-bot: I haz the power, 2016-05-23 20:10:17]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2016-05-23 20:12:42]

Description was changed from

==========
MD Settings: About page, updating buttons based on current update status.

 - For ChromeOS there are three buttons, "Check for updates", "Restart"
   and "Restart and Powerwash".
 - For non-Chrome OS there is only one "Relaunch" button.

BUG=546841,603625
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: About page, updating buttons based on current update status.

 - For ChromeOS there are three buttons, "Check for updates", "Restart"
   and "Restart and Powerwash".
 - For non-Chrome OS there is only one "Relaunch" button.

BUG=546841,603625
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/2c82e33281c61d660697a56bb337c598e487ef0a
Cr-Commit-Position: refs/heads/master@{#395398}
==========

[commit-bot: I haz the power, 2016-05-23 20:12:43]

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