ChromeOS: Add more controls to OOBE EULA screen.

ChromeOS: Add more controls to OOBE EULA screen.

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

Committed: https://crrev.com/32f6f1d45280cc6895cad70fbc691c3719c4fc7a
Cr-Commit-Position: refs/heads/master@{#414578}

[Alexander Alekseev, 2016-08-21 10:40:01]

Description was changed from

==========
ChromeOS: Add more controls to OOBE EULA screen.

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

to

==========
ChromeOS: Add more controls to OOBE EULA screen.

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

[Alexander Alekseev, 2016-08-21 10:45:49]

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

[Alexander Alekseev, 2016-08-21 10:45:49]

Please review.

[Alexander Alekseev, 2016-08-21 10:45:54]

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

[commit-bot: I haz the power, 2016-08-21 10:46:00]

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-08-21 12:05:26]

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

[commit-bot: I haz the power, 2016-08-21 12:05:27]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2016-08-22 09:56:28]

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

[commit-bot: I haz the power, 2016-08-22 09:56:38]

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-08-22 10:54:48]

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

[commit-bot: I haz the power, 2016-08-22 10:54:49]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2016-08-22 18:46:11]

Would you mind sending this to stevenjb? Just trying to share the load a little
:-)

[Alexander Alekseev, 2016-08-22 21:31:50]

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

[Alexander Alekseev, 2016-08-22 21:31:51]

-michaelpg
+stevenjb

Please review.

[stevenjb, 2016-08-23 16:45:17]

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_eula.css (right):

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_eula.css:15: }
We should group CSS by specificity, e.g. element types first, then ids (sorted
alphabetically). i.e. put this first.

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_eula.html (right):

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_eula.html:53:
checked="[[usageStatsChecked]]" on-change="onUsageChanged_">
This does not appear to be disabled, should this be 'checked={{...}}' ?

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_eula.js (right):

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_eula.js:45: /*
/**

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_eula.js:49: type: Object,
Please typedef this.

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_eula.js (right):

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_eula.js:44: });
The data-binding way to do this would be in the html:

on-tap="onUsageStatsClicked_"

Then in onUsageStatsClicked_:

var checked = event.currentTarget.checked;

And the addEventListener code wouldn't be needed at all.

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_eula.js:48: /*
/**

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_eula.js:50: *  @param
{boolean} value  $('usage-stats').checked value.
align

[Alexander Alekseev, 2016-08-25 01:28:22]

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_eula.css (right):

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_eula.css:15: }
On 2016/08/23 16:45:17, stevenjb wrote:
> We should group CSS by specificity, e.g. element types first, then ids (sorted
> alphabetically). i.e. put this first.
> 

Done.

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_eula.html (right):

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_eula.html:53:
checked="[[usageStatsChecked]]" on-change="onUsageChanged_">
On 2016/08/23 16:45:17, stevenjb wrote:
> This does not appear to be disabled, should this be 'checked={{...}}' ?

Done.

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_eula.js (right):

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_eula.js:45: /*
On 2016/08/23 16:45:17, stevenjb wrote:
> /**

Done.

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_eula.js:49: type: Object,
On 2016/08/23 16:45:17, stevenjb wrote:
> Please typedef this.

Done.

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_eula.js (right):

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_eula.js:44: });
On 2016/08/23 16:45:17, stevenjb wrote:
> The data-binding way to do this would be in the html:
> 
> on-tap="onUsageStatsClicked_"
> 
> Then in onUsageStatsClicked_:
> 
> var checked = event.currentTarget.checked;
> 
> And the addEventListener code wouldn't be needed at all.

This doesn't work, because usage-stats is not a polymer-enabled object.

And onClick="$('eula').onUsageStatsClicked_" violates security policy:

Refused to execute inline event handler because it violates the following
Content Security Policy directive: "script-src chrome://resources 'self'
'unsafe-eval'". Either the 'unsafe-inline' keyword, a hash ('sha256-...'), or a
nonce ('nonce-...') is required to enable inline execution.

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_eula.js:48: /*
On 2016/08/23 16:45:17, stevenjb wrote:
> /**

Done.

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_eula.js:50: *  @param
{boolean} value  $('usage-stats').checked value.
On 2016/08/23 16:45:17, stevenjb wrote:
> align

Done.

[stevenjb, 2016-08-25 16:43:46]

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_eula.js (right):

https://codereview.chromium.org/2269433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_eula.js:44: });
On 2016/08/25 01:28:22, Alexander Alekseev wrote:
> On 2016/08/23 16:45:17, stevenjb wrote:
> > The data-binding way to do this would be in the html:
> > 
> > on-tap="onUsageStatsClicked_"
> > 
> > Then in onUsageStatsClicked_:
> > 
> > var checked = event.currentTarget.checked;
> > 
> > And the addEventListener code wouldn't be needed at all.
> 
> This doesn't work, because usage-stats is not a polymer-enabled object.
> 
> And onClick="$('eula').onUsageStatsClicked_" violates security policy:
> 
> Refused to execute inline event handler because it violates the following
> Content Security Policy directive: "script-src chrome://resources 'self'
> 'unsafe-eval'". Either the 'unsafe-inline' keyword, a hash ('sha256-...'), or
a
> nonce ('nonce-...') is required to enable inline execution.

I see. I get confused by the mix of polymer/non-polymer here. This is fine.

https://codereview.chromium.org/2269433002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_types.js (right):

https://codereview.chromium.org/2269433002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_types.js:48: * @typedef {!Object}
We need to define the object properties here, otherwise we do not get any
benefit from type checking.

[Alexander Alekseev, 2016-08-25 21:20:37]

https://codereview.chromium.org/2269433002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_types.js (right):

https://codereview.chromium.org/2269433002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_types.js:48: * @typedef {!Object}
On 2016/08/25 16:43:46, stevenjb wrote:
> We need to define the object properties here, otherwise we do not get any
> benefit from type checking.

This is legacy screen object. Finding its properties would take refactoring of
the whole OOBE system. Let's leave it as Object.

[stevenjb, 2016-08-25 21:41:51]

lgtm

https://codereview.chromium.org/2269433002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_types.js (right):

https://codereview.chromium.org/2269433002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_types.js:48: * @typedef {!Object}
On 2016/08/25 21:20:37, Alexander Alekseev wrote:
> On 2016/08/25 16:43:46, stevenjb wrote:
> > We need to define the object properties here, otherwise we do not get any
> > benefit from type checking.
> 
> This is legacy screen object. Finding its properties would take refactoring of
> the whole OOBE system. Let's leave it as Object.

I see. I am still getting used to this code. Could you at least add a comment
referencing where it is defined? Thanks.

[Alexander Alekseev, 2016-08-25 21:57:01]

https://codereview.chromium.org/2269433002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_types.js (right):

https://codereview.chromium.org/2269433002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_types.js:48: * @typedef {!Object}
On 2016/08/25 21:41:51, stevenjb wrote:
> On 2016/08/25 21:20:37, Alexander Alekseev wrote:
> > On 2016/08/25 16:43:46, stevenjb wrote:
> > > We need to define the object properties here, otherwise we do not get any
> > > benefit from type checking.
> > 
> > This is legacy screen object. Finding its properties would take refactoring
of
> > the whole OOBE system. Let's leave it as Object.
> 
> I see. I am still getting used to this code. Could you at least add a comment
> referencing where it is defined? Thanks.

Done.

[Alexander Alekseev, 2016-08-25 21:57:09]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2016-08-25 21:57:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2269433002/#ps60001
(title: "Add comment.")

[commit-bot: I haz the power, 2016-08-25 21:57:55]

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-08-25 23:18:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-25 23:20:49]

Description was changed from

==========
ChromeOS: Add more controls to OOBE EULA screen.

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

to

==========
ChromeOS: Add more controls to OOBE EULA screen.

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

Committed: https://crrev.com/32f6f1d45280cc6895cad70fbc691c3719c4fc7a
Cr-Commit-Position: refs/heads/master@{#414578}
==========

[commit-bot: I haz the power, 2016-08-25 23:20:50]

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