ChromeOS: Implement Update screen of material design OOBE.

ChromeOS: Implement Update screen of material design OOBE.

This CL adds Update screen to material design OOBE.

BUG=604119
TEST=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/582bb7855b9daa681427476934f13b0cfae8f5be
Cr-Commit-Position: refs/heads/master@{#423415}

[Alexander Alekseev, 2016-10-05 06:18:30]

Description was changed from

==========
ChromeOS: Implement Update screen of material design OOBE.

This CL adds Update screen to material design OOBE.

BUG=604119
TEST=none
==========

to

==========
ChromeOS: Implement Update screen of material design OOBE.

This CL adds Update screen to material design OOBE.

BUG=604119
TEST=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Alexander Alekseev, 2016-10-05 06:19:26]

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

[Alexander Alekseev, 2016-10-05 06:19:26]

Please review.

[Alexander Alekseev, 2016-10-05 06:21:10]

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

[commit-bot: I haz the power, 2016-10-05 06:21:28]

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-10-05 08:24:31]

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

[commit-bot: I haz the power, 2016-10-05 08:24:32]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2016-10-05 21:51:42]

Overall looks good after some small things

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_update.js (right):

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_update.js:125:
loadTimeData.getStringF('downloading', message);
nit: 4-space indent

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_update.js:181: * @param
{object} data Screen init payload.
no param

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_update.css (right):

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_update.css:5: #estimated-time-left
missing comma

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_update.html (right):

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_update.html:35: <div
class="subtitle" id="cancelHint1" i18n-content="cancelUpdateHint"
nit: use clearer names, e.g. cancelCheckForUpdatesHint, cancelUpdateHint

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_update.html:40: <paper-progress
id="checking-progress" indeterminate></paper-progress>
indent

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_update.js (right):

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_update.js:77:
this.$.cancelHint1.textContent = message;
optional: Would be more "Polymeric" to have a "cancelUpdateHint" property.
Instead of setting textContent (and i18n-content), you could then use
[[cancelUpdateHint]] in the two hint <div>s.

To initialize a property to the "cancelUpdateHint" string:

    cancelUpdateHint: {
      type: String,
      value: function() {
        return loadTimeData.getString('cancelUpdateHint');
      },
    },

[Alexander Alekseev, 2016-10-05 22:06:23]

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_update.js (right):

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_update.js:125:
loadTimeData.getStringF('downloading', message);
On 2016/10/05 21:51:41, michaelpg wrote:
> nit: 4-space indent

Done.

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_update.js:181: * @param
{object} data Screen init payload.
On 2016/10/05 21:51:41, michaelpg wrote:
> no param

Done.

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_update.css (right):

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_update.css:5: #estimated-time-left
On 2016/10/05 21:51:41, michaelpg wrote:
> missing comma

Done.

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_update.html (right):

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_update.html:35: <div
class="subtitle" id="cancelHint1" i18n-content="cancelUpdateHint"
On 2016/10/05 21:51:42, michaelpg wrote:
> nit: use clearer names, e.g. cancelCheckForUpdatesHint, cancelUpdateHint

Done.

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_update.html:40: <paper-progress
id="checking-progress" indeterminate></paper-progress>
On 2016/10/05 21:51:41, michaelpg wrote:
> indent

Done.

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_update.js (right):

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_update.js:77:
this.$.cancelHint1.textContent = message;
On 2016/10/05 21:51:42, michaelpg wrote:
> optional: Would be more "Polymeric" to have a "cancelUpdateHint" property.
> Instead of setting textContent (and i18n-content), you could then use
> [[cancelUpdateHint]] in the two hint <div>s.
> 
> To initialize a property to the "cancelUpdateHint" string:
> 
>     cancelUpdateHint: {
>       type: String,
>       value: function() {
>         return loadTimeData.getString('cancelUpdateHint');
>       },
>     },

I decided not to go this way because it would require onBeforeShow and
updateLocalizedContent hooks. This "dev-only" method seems to be simpler and
less error-prone.

[Alexander Alekseev, 2016-10-05 22:06:24]

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_update.js (right):

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_update.js:125:
loadTimeData.getStringF('downloading', message);
On 2016/10/05 21:51:41, michaelpg wrote:
> nit: 4-space indent

Done.

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_update.js:181: * @param
{object} data Screen init payload.
On 2016/10/05 21:51:41, michaelpg wrote:
> no param

Done.

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_update.css (right):

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_update.css:5: #estimated-time-left
On 2016/10/05 21:51:41, michaelpg wrote:
> missing comma

Done.

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_update.html (right):

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_update.html:35: <div
class="subtitle" id="cancelHint1" i18n-content="cancelUpdateHint"
On 2016/10/05 21:51:42, michaelpg wrote:
> nit: use clearer names, e.g. cancelCheckForUpdatesHint, cancelUpdateHint

Done.

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_update.html:40: <paper-progress
id="checking-progress" indeterminate></paper-progress>
On 2016/10/05 21:51:41, michaelpg wrote:
> indent

Done.

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_update.js (right):

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_update.js:77:
this.$.cancelHint1.textContent = message;
On 2016/10/05 21:51:42, michaelpg wrote:
> optional: Would be more "Polymeric" to have a "cancelUpdateHint" property.
> Instead of setting textContent (and i18n-content), you could then use
> [[cancelUpdateHint]] in the two hint <div>s.
> 
> To initialize a property to the "cancelUpdateHint" string:
> 
>     cancelUpdateHint: {
>       type: String,
>       value: function() {
>         return loadTimeData.getString('cancelUpdateHint');
>       },
>     },

I decided not to go this way because it would require onBeforeShow and
updateLocalizedContent hooks. This "dev-only" method seems to be simpler and
less error-prone.

[Alexander Alekseev, 2016-10-05 22:06:39]

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

[commit-bot: I haz the power, 2016-10-05 22:07:33]

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-10-05 23:15:00]

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

[commit-bot: I haz the power, 2016-10-05 23:15:01]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2016-10-05 23:28:15]

lgtm, but see response

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_update.html (right):

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_update.html:35: <div
class="subtitle" id="cancelHint1" i18n-content="cancelUpdateHint"
On 2016/10/05 22:06:23, Alexander Alekseev wrote:
> On 2016/10/05 21:51:42, michaelpg wrote:
> > nit: use clearer names, e.g. cancelCheckForUpdatesHint, cancelUpdateHint
> 
> Done.

You misunderstood: it's not clear what the difference is between
cancelUpdateHint1 and cancelUpdateHint2.

I was suggesting you name them cancelCheckForUpdateHint and cancelUpdateHint, so
it's easy to understand what they're for at a glance.

[Alexander Alekseev, 2016-10-06 00:07:13]

I added string for the screen title.
(Per offline discussion.)

ptal.

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_update.html (right):

https://codereview.chromium.org/2392273002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_update.html:35: <div
class="subtitle" id="cancelHint1" i18n-content="cancelUpdateHint"
On 2016/10/05 23:28:14, michaelpg wrote:
> On 2016/10/05 22:06:23, Alexander Alekseev wrote:
> > On 2016/10/05 21:51:42, michaelpg wrote:
> > > nit: use clearer names, e.g. cancelCheckForUpdatesHint, cancelUpdateHint
> > 
> > Done.
> 
> You misunderstood: it's not clear what the difference is between
> cancelUpdateHint1 and cancelUpdateHint2.
> 
> I was suggesting you name them cancelCheckForUpdateHint and cancelUpdateHint,
so
> it's easy to understand what they're for at a glance.

Done.

[michaelpg, 2016-10-06 00:41:14]

thanks, lgtm

[Alexander Alekseev, 2016-10-06 00:52:31]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2016-10-06 00:53:12]

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-10-06 03:03:57]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-06 03:07:23]

Description was changed from

==========
ChromeOS: Implement Update screen of material design OOBE.

This CL adds Update screen to material design OOBE.

BUG=604119
TEST=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
ChromeOS: Implement Update screen of material design OOBE.

This CL adds Update screen to material design OOBE.

BUG=604119
TEST=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/582bb7855b9daa681427476934f13b0cfae8f5be
Cr-Commit-Position: refs/heads/master@{#423415}
==========

[commit-bot: I haz the power, 2016-10-06 03:07:24]

Patchset 3 (id:??) landed as
https://crrev.com/582bb7855b9daa681427476934f13b0cfae8f5be
Cr-Commit-Position: refs/heads/master@{#423415}