ChromeOS: Powerwash UI update

ChromeOS: Powerwash UI update

This CL does two things:
1) Replace old pictures with new ones for powerwash page.
2) Rebuild everything using Polymer for material design.

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

Review-Url: https://codereview.chromium.org/2756863002
Cr-Commit-Position: refs/heads/master@{#464326}
Committed: https://chromium.googlesource.com/chromium/src/+/2a8fcff1676460930aab885766445035aa451625

[Wenzhao (Colin) Zang, 2017-03-17 04:41:10]

Description was changed from

==========
Powerwash UI update

BUG=653269
==========

to

==========
Powerwash UI update

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

[Wenzhao (Colin) Zang, 2017-03-17 04:47:35]

wzang@chromium.org changed reviewers:
+ alemate@chromium.org, grt@chromium.org

[Wenzhao (Colin) Zang, 2017-03-17 04:47:35]

alemate@chromium.org: Please review changes in files under chrome/browser

grt@chromium.org: Please review changes in files under chrome/app

Thanks.

[Alexander Alekseev, 2017-03-17 06:20:57]

I think we need to rebuild this using polymer.

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

https://codereview.chromium.org/2756863002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.css:11: text-align:
left;
text-align: start;

We need this for RTL languages.

https://codereview.chromium.org/2756863002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.css:38: color: #000
100%;
nit: rgb(0, 0, 0)

We tend to specify colors in rgb() or rgba() notation according to code style.

https://codereview.chromium.org/2756863002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.css:41: text-align:
left;
text-align: start;

[Wenzhao (Colin) Zang, 2017-03-21 02:01:06]

alemate@chromium.org: This is an incomplete CL. Just want to see if the basic
idea is correct.

[grt (UTC plus 2), 2017-03-23 09:27:40]

chrome/app rubberstamp lgtm

[Wenzhao (Colin) Zang, 2017-03-28 03:29:19]

Description was changed from

==========
Powerwash UI update

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

to

==========
Powerwash UI update

This CL changes two things:
1) Replace old pictures with new ones for powerwash page.
2) Rebuild everything using Polymer for material design.

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

[Wenzhao (Colin) Zang, 2017-03-28 03:36:57]

Both oobe_screen_reset.html and oobe_screen_reset_confirmation_overlay.html need
Polymer rebuild. 

The combination of the five variables in 
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/screens/re...

control the UI state of the powerwash page. We can use methods such as
GetContextEditor().SetBoolen() to manually simulate the signal and see the
corresponding changes of the page.

[Wenzhao (Colin) Zang, 2017-03-29 03:28:05]

Description was changed from

==========
Powerwash UI update

This CL changes two things:
1) Replace old pictures with new ones for powerwash page.
2) Rebuild everything using Polymer for material design.

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

to

==========
ChromeOS: Powerwash UI update

This CL does two things:
1) Replace old pictures with new ones for powerwash page.
2) Rebuild everything using Polymer for material design.

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

[Wenzhao (Colin) Zang, 2017-03-29 03:29:46]

Minor change: add md-mode to #reset and #overlay-reset so that their style
conforms to material design.

[Wenzhao (Colin) Zang, 2017-03-29 19:25:19]

Minor change on reset container width

[Wenzhao (Colin) Zang, 2017-03-29 23:57:15]

Minor css change for compatibility with non-md page

[Wenzhao (Colin) Zang, 2017-03-30 23:56:39]

Fix help link visibility

[Wenzhao (Colin) Zang, 2017-04-06 00:23:55]

wzang@chromium.org changed reviewers:
+ jdufault@chromium.org

[Wenzhao (Colin) Zang, 2017-04-06 00:40:21]

@jdufault, please take a look at the files under chrome/browser when you get a
chance. Thanks a lot.

[jdufault, 2017-04-06 01:04:52]

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.css (right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.css:1: /* Copyright 2016 The
Chromium Authors. All rights reserved.
nit: 2017

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.html (right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:1: <!-- Copyright 2016
The Chromium Authors. All rights reserved.
nit: 2017

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.js (right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.js:19: * 4 -
'rollback-proposal-view',
Use separate variables instead of trying to encode all state via flags.

Encoding via flags like this is generally harder to understand because
'uiState_' does not provide any semantic value to the reader. For example,
getHidden_(*, 2) has no immediate meaning - compare to
isPowerwashProposalHidden().

Using a constant for 2 helps, getHidden(*, POWERWASH_PROPOSAL), but the
underlying implementation is still hiding additional complexity that isn't
needed.

There's probably an argument for code deduplication, but there's already a
method isHelpLinkHidden_, which makes the reader question the difference between
getHidden_ and isHelpLinkHidden_.

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.css
(right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.css:1:
/* Copyright 2016 The Chromium Authors. All rights reserved.
nit: 2017

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.css:11:
border: 1px solid rgb(188, 193, 208);
Please use the --google colors where possible, ex,
https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/login/....

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.html
(right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.html:1:
<!-- Copyright 2016 The Chromium Authors. All rights reserved.
nit: 2017

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.html:21:
<oobe-text-button border on-tap="onCancelClicked_">
nit: since this is an on-tap handler the function should be named onCancelTap_

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.html:25:
<oobe-text-button inverse on-tap="onContinueClicked_">
ditto

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.js
(right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.js:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
nit: 2017

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:168: if
(loadTimeData.getString('newOobeUI') == 'on')
create helper function for loadTimeData.getString('newOobeUI') == 'on' since
you've repeated it and it is using constants.

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:195: setMDMode_:
function() {
nit: showMdMode_

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:196: var useMDOobe
= (loadTimeData.getString('newOobeUI') == 'on');
nit: useMdOobe

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_text_button.css (right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_text_button.css:35: border-color:
rgba(0, 0, 0, 0.2);
use --google-* if possible

[Wenzhao (Colin) Zang, 2017-04-07 02:55:37]

@jdufault I addressed all the comments. Thanks so much!

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.css (right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.css:1: /* Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/04/06 01:04:52, jdufault wrote:
> nit: 2017

Done.

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.html (right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:1: <!-- Copyright 2016
The Chromium Authors. All rights reserved.
On 2017/04/06 01:04:52, jdufault wrote:
> nit: 2017

Done.

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.js (right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.js:19: * 4 -
'rollback-proposal-view',
On 2017/04/06 01:04:52, jdufault wrote:
> Use separate variables instead of trying to encode all state via flags.
> 
> Encoding via flags like this is generally harder to understand because
> 'uiState_' does not provide any semantic value to the reader. For example,
> getHidden_(*, 2) has no immediate meaning - compare to
> isPowerwashProposalHidden().
> 
> Using a constant for 2 helps, getHidden(*, POWERWASH_PROPOSAL), but the
> underlying implementation is still hiding additional complexity that isn't
> needed.
> 
> There's probably an argument for code deduplication, but there's already a
> method isHelpLinkHidden_, which makes the reader question the difference
between
> getHidden_ and isHelpLinkHidden_.

Done.

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.css
(right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.css:1:
/* Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/04/06 01:04:52, jdufault wrote:
> nit: 2017

Done.

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.css:11:
border: 1px solid rgb(188, 193, 208);
On 2017/04/06 01:04:52, jdufault wrote:
> Please use the --google colors where possible, ex,
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/login/....

Done.

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.html
(right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.html:1:
<!-- Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/04/06 01:04:52, jdufault wrote:
> nit: 2017

Done.

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.html:21:
<oobe-text-button border on-tap="onCancelClicked_">
On 2017/04/06 01:04:52, jdufault wrote:
> nit: since this is an on-tap handler the function should be named onCancelTap_

Done.

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.html:25:
<oobe-text-button inverse on-tap="onContinueClicked_">
On 2017/04/06 01:04:52, jdufault wrote:
> ditto

Done.

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.js
(right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset_confirmation_overlay.js:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/04/06 01:04:52, jdufault wrote:
> nit: 2017

Done.

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:168: if
(loadTimeData.getString('newOobeUI') == 'on')
On 2017/04/06 01:04:52, jdufault wrote:
> create helper function for loadTimeData.getString('newOobeUI') == 'on' since
> you've repeated it and it is using constants.

Done.

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:195: setMDMode_:
function() {
On 2017/04/06 01:04:52, jdufault wrote:
> nit: showMdMode_

Since setMDMode_ is used in all other files, I want it to be consistent. e.g.
https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/login/...

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:196: var useMDOobe
= (loadTimeData.getString('newOobeUI') == 'on');
On 2017/04/06 01:04:52, jdufault wrote:
> nit: useMdOobe

Acknowledged.

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_text_button.css (right):

https://codereview.chromium.org/2756863002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_text_button.css:35: border-color:
rgba(0, 0, 0, 0.2);
On 2017/04/06 01:04:52, jdufault wrote:
> use --google-* if possible

Just want it to be consistent with another material design button: 

https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/login/...

[Alexander Alekseev, 2017-04-08 10:36:19]

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.css (right):

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.css:6: height: 248px;
We don't need hardcoded value of pixel image size.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.css:7: padding: 50px 40px;
Why?
Could you just center it vertically?

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.css:17: vertical-align:
middle;
Could you do this with polymer class styling?

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.html (right):

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:10: <oobe-dialog
id="resetDialog" role="dialog"
The dialog size should be 768x640 px.
This one is so large that it does not fit the screen.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:13: <img
src="chrome://theme/IDR_RESET_ALERT"
OOBE/signin code is traditionally using embedded images.
I don't think we really need embedded images for this screen (probably your
approach is better because this screen is rarely used), but just FYI.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:13: <img
src="chrome://theme/IDR_RESET_ALERT"
Please replace this icon with polymer icon icons:info .

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:13: <img
src="chrome://theme/IDR_RESET_ALERT"
If you place the icon here, the header will occupy too much space.
The icon should be passed to oobe-dialog element as oobe-icon.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:15: <h1 class="title"
i18n-content="resetWarningTitle"></h1>
We do not have precise specks for reset screen, but we usually use  13px Roboto
(see specks in go/cros-oobe).

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:26: <a
id="powerwash-help-link-md" href="#"
Are you sure that this link must be always displayed? (i.e. it is not a part of
some div about)

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:46: <div
class="vertical-margin"
Is it just a spacer? Could margin CSS attribute be used (for button)?

[Wenzhao (Colin) Zang, 2017-04-10 20:37:35]

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.css (right):

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.css:6: height: 248px;
On 2017/04/08 10:36:18, Alexander Alekseev wrote:
> We don't need hardcoded value of pixel image size.

It's removed.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.css:7: padding: 50px 40px;
On 2017/04/08 10:36:18, Alexander Alekseev wrote:
> Why?
> Could you just center it vertically?

Because the illustration is not symmetric, with more white space on the right.
In order to make it look 'centered', I think it's better to have more padding on
the left. Please see:

https://drive.google.com/a/google.com/file/d/0B_2Uyb2Rhx2OSmlRUEE4U18wemc/vie...

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.css:17: vertical-align:
middle;
On 2017/04/08 10:36:18, Alexander Alekseev wrote:
> Could you do this with polymer class styling?

Sorry I don't get it, could you please explain on this? Is it the same with CSS
class binding, like: 'class$="[[]]"', then do we need it if the class name
doesn't change?

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.html (right):

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:10: <oobe-dialog
id="resetDialog" role="dialog"
On 2017/04/08 10:36:18, Alexander Alekseev wrote:
> The dialog size should be 768x640 px.
> This one is so large that it does not fit the screen.

It's changed to 768x640 px now.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:13: <img
src="chrome://theme/IDR_RESET_ALERT"
On 2017/04/08 10:36:18, Alexander Alekseev wrote:
> If you place the icon here, the header will occupy too much space.
> The icon should be passed to oobe-dialog element as oobe-icon.

Originally I used iron-icon, but by using <iron-icon src="..."> I couldn't make
it automatically show two resolutions, like <img src="..."> does.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:13: <img
src="chrome://theme/IDR_RESET_ALERT"
On 2017/04/08 10:36:18, Alexander Alekseev wrote:
> OOBE/signin code is traditionally using embedded images.
> I don't think we really need embedded images for this screen (probably your
> approach is better because this screen is rarely used), but just FYI.

Done.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:13: <img
src="chrome://theme/IDR_RESET_ALERT"
On 2017/04/08 10:36:18, Alexander Alekseev wrote:
> Please replace this icon with polymer icon icons:info .

Done. The info icon is black so it's different from the mock. Is it okay? If so
I'll delete the image in the assets directory.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:15: <h1 class="title"
i18n-content="resetWarningTitle"></h1>
On 2017/04/08 10:36:18, Alexander Alekseev wrote:
> We do not have precise specks for reset screen, but we usually use  13px
Roboto
> (see specks in go/cros-oobe).

Done.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:26: <a
id="powerwash-help-link-md" href="#"
On 2017/04/08 10:36:18, Alexander Alekseev wrote:
> Are you sure that this link must be always displayed? (i.e. it is not a part
of
> some div about)

I mimicked the original oobe_screen_reset.html structure and the link is not
part of a div. The hidden state of the link is controlled by the
'is-official-build' flag sent from reset_screen.cc. The flag value is actually
never changed so the link is always hidden.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:46: <div
class="vertical-margin"
On 2017/04/08 10:36:18, Alexander Alekseev wrote:
> Is it just a spacer? Could margin CSS attribute be used (for button)?

Done. Sorry I ignored this much simpler method.

[Alexander Alekseev, 2017-04-11 04:19:26]

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.html (right):

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:18: <div
hidden="[[isPowerwashHidden_(uiState_)]]"
We can (probably) make this more readable if we embed states to HTML.
You can actually put constants to polymer expressions:

hidden="[[!isState(uiState_,'powerwash-proposal-view')]]"

Then, you can define isState() as:

isState: function(actualState, state) {
  return state == actualState;
}

It looks like the only specific method left here would be isHelpLinkHidden_().

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:49:
on-tap="onRestartTap_">
I believe you can also replace all the on.*Tap_() functions with
userActed('action');

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:54: <oobe-text-button
hidden="[[isDataWarningHidden_(uiState_)]]" inverse
Could you split this into two buttons?
I see that they share the same action, but they look different to user, so let's
make them separate.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.js (right):

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.js:22: isPowerwashHidden_:
function(uiState_) {
@private

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.css (left):

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_reset.css:25: #reset
#reset-warning-icon {
reset-warning-icon is still used in old UI.
We do not want to break it.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_reset.css:31: #reset
#powerwash-and-rollback-warning-msg {
Please do not update old styles.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_reset.css:37: .reset-text {
Ditto.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.css (right):

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_reset.css:18: width: 768px;
Could you specify text-align here, and not update style above?

[Wenzhao (Colin) Zang, 2017-04-11 20:42:54]

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.html (right):

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:13: <img
src="chrome://theme/IDR_RESET_ALERT"
On 2017/04/10 20:37:35, Wenzhao (Colin) Zang wrote:
> On 2017/04/08 10:36:18, Alexander Alekseev wrote:
> > Please replace this icon with polymer icon icons:info .
> 
> Done. The info icon is black so it's different from the mock. Is it okay? If
so
> I'll delete the image in the assets directory.

Now the icon is blue. And the illustration is embedded image now.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:18: <div
hidden="[[isPowerwashHidden_(uiState_)]]"
On 2017/04/11 04:19:26, Alexander Alekseev wrote:
> We can (probably) make this more readable if we embed states to HTML.
> You can actually put constants to polymer expressions:
> 
> hidden="[[!isState(uiState_,'powerwash-proposal-view')]]"
> 
> Then, you can define isState() as:
> 
> isState: function(actualState, state) {
>   return state == actualState;
> }
> 
> It looks like the only specific method left here would be isHelpLinkHidden_().

Done.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:49:
on-tap="onRestartTap_">
On 2017/04/11 04:19:26, Alexander Alekseev wrote:
> I believe you can also replace all the on.*Tap_() functions with
> userActed('action');

I tried something like 
<oobe-text-button on-tap="userActed" action="cancel-reset">

but it doesn't work, it seems that oobe-text-button does not have 'action' field
as paper-button does.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:54: <oobe-text-button
hidden="[[isDataWarningHidden_(uiState_)]]" inverse
On 2017/04/11 04:19:26, Alexander Alekseev wrote:
> Could you split this into two buttons?
> I see that they share the same action, but they look different to user, so
let's
> make them separate.

Done.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.js (right):

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.js:22: isPowerwashHidden_:
function(uiState_) {
On 2017/04/11 04:19:26, Alexander Alekseev wrote:
> @private

Done.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.css (left):

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_reset.css:25: #reset
#reset-warning-icon {
On 2017/04/11 04:19:26, Alexander Alekseev wrote:
> reset-warning-icon is still used in old UI.
> We do not want to break it.

Acknowledged.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_reset.css:31: #reset
#powerwash-and-rollback-warning-msg {
On 2017/04/11 04:19:26, Alexander Alekseev wrote:
> Please do not update old styles.

Acknowledged.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_reset.css:37: .reset-text {
On 2017/04/11 04:19:26, Alexander Alekseev wrote:
> Ditto.

Acknowledged.

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.css (right):

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_reset.css:18: width: 768px;
On 2017/04/11 04:19:26, Alexander Alekseev wrote:
> Could you specify text-align here, and not update style above?

Acknowledged.

[Alexander Alekseev, 2017-04-12 23:48:19]

On 2017/04/11 20:42:54, Wenzhao (Colin) Zang wrote:
>
https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
> File chrome/browser/resources/chromeos/login/oobe_reset.html (right):
> 
>
https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/login/oobe_reset.html:13: <img
> src="chrome://theme/IDR_RESET_ALERT"
> On 2017/04/10 20:37:35, Wenzhao (Colin) Zang wrote:
> > On 2017/04/08 10:36:18, Alexander Alekseev wrote:
> > > Please replace this icon with polymer icon icons:info .
> > 
> > Done. The info icon is black so it's different from the mock. Is it okay? If
> so
> > I'll delete the image in the assets directory.
> 
> Now the icon is blue. And the illustration is embedded image now.

Do you still need to add chrome/app/theme/* files ?

[Alexander Alekseev, 2017-04-13 00:01:53]

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.html (right):

https://codereview.chromium.org/2756863002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:49:
on-tap="onRestartTap_">
On 2017/04/11 20:42:54, Wenzhao (Colin) Zang wrote:
> On 2017/04/11 04:19:26, Alexander Alekseev wrote:
> > I believe you can also replace all the on.*Tap_() functions with
> > userActed('action');
> 
> I tried something like 
> <oobe-text-button on-tap="userActed" action="cancel-reset">
> 
> but it doesn't work, it seems that oobe-text-button does not have 'action'
field
> as paper-button does.

You're right, my comment was incorrect. Just ignore it.

https://codereview.chromium.org/2756863002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.html (right):

https://codereview.chromium.org/2756863002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:46:
hidden="[[isCancelHidden_(uiState_)]]" border
nit: move this to the previous line.

https://codereview.chromium.org/2756863002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:51:
hidden="[[!isState_(uiState_, 'restart-required-view')]]" inverse
nit: you can move shorter attributes like 'inverse' and on-tap to the previous
line.

https://codereview.chromium.org/2756863002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:57:
on-tap="onPowerwashTap_">
Ditto.

https://codereview.chromium.org/2756863002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:61:
hidden="[[!isState_(uiState_, 'powerwash-proposal-view')]]" inverse
Ditto.

https://codereview.chromium.org/2756863002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.js (right):

https://codereview.chromium.org/2756863002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.js:19: isOfficial_: Boolean,
Please, add comment to this property.

[Wenzhao (Colin) Zang, 2017-04-13 02:58:21]

Changed to embedded images and deletes images under chrome/app/theme

https://codereview.chromium.org/2756863002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.html (right):

https://codereview.chromium.org/2756863002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:46:
hidden="[[isCancelHidden_(uiState_)]]" border
On 2017/04/13 00:01:53, Alexander Alekseev wrote:
> nit: move this to the previous line.

Done.

https://codereview.chromium.org/2756863002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:51:
hidden="[[!isState_(uiState_, 'restart-required-view')]]" inverse
On 2017/04/13 00:01:53, Alexander Alekseev wrote:
> nit: you can move shorter attributes like 'inverse' and on-tap to the previous
> line.

Done.

https://codereview.chromium.org/2756863002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:57:
on-tap="onPowerwashTap_">
On 2017/04/13 00:01:53, Alexander Alekseev wrote:
> Ditto.

Done.

https://codereview.chromium.org/2756863002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.html:61:
hidden="[[!isState_(uiState_, 'powerwash-proposal-view')]]" inverse
On 2017/04/13 00:01:53, Alexander Alekseev wrote:
> Ditto.

Done.

https://codereview.chromium.org/2756863002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_reset.js (right):

https://codereview.chromium.org/2756863002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_reset.js:19: isOfficial_: Boolean,
On 2017/04/13 00:01:53, Alexander Alekseev wrote:
> Please, add comment to this property.

Done.

[Alexander Alekseev, 2017-04-13 03:13:28]

lgtm

[Wenzhao (Colin) Zang, 2017-04-13 03:13:52]

The CQ bit was checked by wzang@chromium.org

[Wenzhao (Colin) Zang, 2017-04-13 03:13:52]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2756863002/#ps200001
(title: "Change to embedded images")

[commit-bot: I haz the power, 2017-04-13 03:14:20]

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-04-13 03:19:14]

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

[commit-bot: I haz the power, 2017-04-13 03:19:16]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Wenzhao (Colin) Zang, 2017-04-13 03:24:59]

The CQ bit was checked by wzang@chromium.org

[commit-bot: I haz the power, 2017-04-13 03:25:50]

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-04-13 03:29:37]

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

[commit-bot: I haz the power, 2017-04-13 03:29:38]

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

[Wenzhao (Colin) Zang, 2017-04-13 03:30:55]

The CQ bit was checked by wzang@chromium.org

[commit-bot: I haz the power, 2017-04-13 03:31:27]

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-04-13 03:36:46]

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

[commit-bot: I haz the power, 2017-04-13 03:36:47]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Wenzhao (Colin) Zang, 2017-04-13 04:21:57]

The CQ bit was checked by wzang@chromium.org

[Wenzhao (Colin) Zang, 2017-04-13 04:21:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
alemate@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2756863002/#ps220001
(title: "merge")

[commit-bot: I haz the power, 2017-04-13 04:22:20]

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-04-13 05:50:23]

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

[commit-bot: I haz the power, 2017-04-13 05:50:25]

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

[Wenzhao (Colin) Zang, 2017-04-13 06:03:03]

The CQ bit was checked by wzang@chromium.org

[commit-bot: I haz the power, 2017-04-13 06:03:34]

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-04-13 06:53:15]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1492063383405870,
"parent_rev": "5d84a680108877c6a9c526b3e0711913a85245e5", "commit_rev":
"2a8fcff1676460930aab885766445035aa451625"}

[commit-bot: I haz the power, 2017-04-13 06:54:01]

Description was changed from

==========
ChromeOS: Powerwash UI update

This CL does two things:
1) Replace old pictures with new ones for powerwash page.
2) Rebuild everything using Polymer for material design.

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

to

==========
ChromeOS: Powerwash UI update

This CL does two things:
1) Replace old pictures with new ones for powerwash page.
2) Rebuild everything using Polymer for material design.

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

Review-Url: https://codereview.chromium.org/2756863002
Cr-Commit-Position: refs/heads/master@{#464326}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a8fcff1676460930aab88576644...
==========

[commit-bot: I haz the power, 2017-04-13 06:54:03]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/2a8fcff1676460930aab88576644...