Site Settings Desktop: Implement tri-state 'Delete local data on exit'.

Site Settings Desktop: Implement tri-state 'Delete local data on exit'.

BUG=635847, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[Finnur, 2016-09-06 11:53:11]

Description was changed from

==========
Site Settings Desktop: Implement tri-state 'Delete local data on exit'.

BUG=635847, 614277
==========

to

==========
Site Settings Desktop: Implement tri-state 'Delete local data on exit'.

BUG=635847, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Finnur, 2016-09-06 12:09:28]

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

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

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

[Finnur, 2016-09-06 13:10:01]

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

[Finnur, 2016-09-06 13:10:02]

https://codereview.chromium.org/2316633002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/2316633002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_category.js:159:
if (this.category == settings.ContentSettingsTypes.PLUGINS) {
The Flash section is changing because I noticed a bug. If you go to the old
content settings UI and select the equivalent of IMPORTANT_CONTENT there, then
the new UI is not updated (checkbox not checked). You need a refresh to see it
change.

[commit-bot: I haz the power, 2016-09-06 13:21:02]

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

[commit-bot: I haz the power, 2016-09-06 13:21:04]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-09-06 18:36:43]

LGTM since my comments are just about comments or nits.

https://codereview.chromium.org/2316633002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/2316633002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:266:
//     vs "Keep local data only until you quit your browser".
It looks like there is four states. Blocked and Allowed 
seem to be treated as one state. I think the code is
fine, but the comment is misleading.

https://codereview.chromium.org/2316633002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_settings_category.html
(right):

https://codereview.chromium.org/2316633002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_category.html:54:
disabled="[[!categoryEnabled]]">
nit: consider unwrapping this line to fewer lines.

https://codereview.chromium.org/2316633002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/2316633002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_category.js:48: *
Used only for Cookies to persist the Session Only checkbox state.
nit: Not sure, but something seems off in this sentence.
Maybe add 'in' (or 'during')?

Used only for Cookies to persist in the Session Only checkbox state.

https://codereview.chromium.org/2316633002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/site_settings_category_tests.js (right):

https://codereview.chromium.org/2316633002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_settings_category_tests.js:60: * An example
pref where the Cookies category is set on delete when
optional: Maybe change 'on' --> 'to'

An example pref where the Cookies category is set to delete when

https://codereview.chromium.org/2316633002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_settings_category_tests.js:171: false));
nit: unwrap a bit.

[Finnur, 2016-09-08 09:58:18]

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

[commit-bot: I haz the power, 2016-09-08 09:58:33]

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

[Finnur, 2016-09-08 09:59:39]

https://codereview.chromium.org/2316633002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/2316633002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:266:
//     vs "Keep local data only until you quit your browser".
I don't quite understand... There are three states for Cookies: Allow, Block and
SessionOnly.

https://codereview.chromium.org/2316633002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_settings_category.html
(right):

https://codereview.chromium.org/2316633002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_category.html:54:
disabled="[[!categoryEnabled]]">
On 2016/09/06 18:36:43, dschuyler wrote:
> nit: consider unwrapping this line to fewer lines.

Done.

https://codereview.chromium.org/2316633002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/2316633002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_category.js:48: *
Used only for Cookies to persist the Session Only checkbox state.
On 2016/09/06 18:36:43, dschuyler wrote:
> nit: Not sure, but something seems off in this sentence.
> Maybe add 'in' (or 'during')?
> 
> Used only for Cookies to persist in the Session Only checkbox state.

Done.

https://codereview.chromium.org/2316633002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/site_settings_category_tests.js (right):

https://codereview.chromium.org/2316633002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_settings_category_tests.js:60: * An example
pref where the Cookies category is set on delete when
On 2016/09/06 18:36:43, dschuyler wrote:
> optional: Maybe change 'on' --> 'to'
> 
> An example pref where the Cookies category is set to delete when

Done.

https://codereview.chromium.org/2316633002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_settings_category_tests.js:171: false));
On 2016/09/06 18:36:43, dschuyler wrote:
> nit: unwrap a bit.

Done.

[commit-bot: I haz the power, 2016-09-08 11:23:38]

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

[commit-bot: I haz the power, 2016-09-08 11:23:39]

Dry run: This issue passed the CQ dry run.

[Finnur, 2016-09-08 11:44:52]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-09-08 11:44:52]

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/2316633002/#ps20001
(title: "Address feedback")

[commit-bot: I haz the power, 2016-09-08 11:45:06]

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-09-08 11:49:00]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-08 11:50:30]

Description was changed from

==========
Site Settings Desktop: Implement tri-state 'Delete local data on exit'.

BUG=635847, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Site Settings Desktop: Implement tri-state 'Delete local data on exit'.

BUG=635847, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-09-08 11:50:31]

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