Site Settings Desktop: Implement the 'Clear on exit' menu for Cookies.

Site Settings Desktop: Implement the 'Clear on exit' menu for Cookies.

BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/67798eab1c7003952ba4176f2e2809df6425395f
Cr-Commit-Position: refs/heads/master@{#400154}

[Finnur, 2016-06-09 15:12:51]

Description was changed from

==========
Site Settings Desktop: Implement the 'Clear on exit' menu for Cookies.

BUG=614277, 543635
==========

to

==========
Site Settings Desktop: Implement the 'Clear on exit' menu for Cookies.

BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Finnur, 2016-06-09 15:35:49]

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

[michaelpg, 2016-06-09 18:43:43]

https://codereview.chromium.org/2055823003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/constants.js (right):

https://codereview.chromium.org/2055823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/constants.js:41: SESSION_ONLY:
'session_only',
nit: use consistent ordering
https://cs.chromium.org/chromium/src/components/content_settings/core/common/...

https://codereview.chromium.org/2055823003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/2055823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:101: i18n_: {
do you still need this in the JS? if not, remove and use $i18n{} in HTML

https://codereview.chromium.org/2055823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:121:
this.PermissionValues = settings.PermissionValues;
consider making this a readOnly property of SiteSettingsBehavior to avoid
duplication

https://codereview.chromium.org/2055823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:417: if
(this.categorySubtype == settings.PermissionValues.ALLOW) {
suggested cleanup:

var title = '';
if (this.categorySubtype == settings.PermissionValues.ALLOW)
  title = toggleState ? 'siteSettingsAllow' : 'siteSettingsExceptions';
else if (...)
  title = 'siteSettingsBlock';
else if (...)
  title = 'siteSettingsSessionOnly';
else
  return title;
return loadTimeData.getStringF(
    'titleAndCount', loadTimeData.getString(title), siteList.length);

https://codereview.chromium.org/2055823003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/2055823003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:279:
listItem.querySelectorAll('paper-menu-button paper-item');
Use the platform, Luke!

var menuItems = listItem.querySelectorAll(
    'paper-menu-button paper-item:not([hidden])');
assertEquals(items.length, menuItems.length);
for (var i = 0; i < items.length; i++)
  assertEquals(items[i], menuItems[i].textContent.trim());

https://codereview.chromium.org/2055823003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:435: function(contentType) {
nit: 4-space indent

[Finnur, 2016-06-10 11:35:31]

Patchset #3 (id:40001) has been deleted

[Finnur, 2016-06-10 14:20:47]

Rietveld seems to have freaked out with the whitespace changes in the test. You
might be better served running a diff locally...

I also hid the Session Only category when it is empty. It felt too weird to have
it visible.

https://codereview.chromium.org/2055823003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/constants.js (right):

https://codereview.chromium.org/2055823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/constants.js:41: SESSION_ONLY:
'session_only',
On 2016/06/09 18:43:42, michaelpg wrote:
> nit: use consistent ordering
>
https://cs.chromium.org/chromium/src/components/content_settings/core/common/...

Done.

https://codereview.chromium.org/2055823003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/2055823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:101: i18n_: {
Good catch. Done.

https://codereview.chromium.org/2055823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:121:
this.PermissionValues = settings.PermissionValues;
On 2016/06/09 18:43:42, michaelpg wrote:
> consider making this a readOnly property of SiteSettingsBehavior to avoid
> duplication

Done.

https://codereview.chromium.org/2055823003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:417: if
(this.categorySubtype == settings.PermissionValues.ALLOW) {
On 2016/06/09 18:43:42, michaelpg wrote:
> suggested cleanup:
> 
> var title = '';
> if (this.categorySubtype == settings.PermissionValues.ALLOW)
>   title = toggleState ? 'siteSettingsAllow' : 'siteSettingsExceptions';
> else if (...)
>   title = 'siteSettingsBlock';
> else if (...)
>   title = 'siteSettingsSessionOnly';
> else
>   return title;
> return loadTimeData.getStringF(
>     'titleAndCount', loadTimeData.getString(title), siteList.length);

Done.

https://codereview.chromium.org/2055823003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/2055823003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:279:
listItem.querySelectorAll('paper-menu-button paper-item');
Hey, neat. :)

https://codereview.chromium.org/2055823003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:435: function(contentType) {
On 2016/06/09 18:43:43, michaelpg wrote:
> nit: 4-space indent

Done.

[michaelpg, 2016-06-13 15:54:38]

lgtm

https://codereview.chromium.org/2055823003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/2055823003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:32:
this.PermissionValues = settings.PermissionValues;
Why attached instead of ready? (I would be worried we would use this somewhere
after ready but before attached, but I guess that's not happening?)

[Finnur, 2016-06-16 13:53:10]

https://codereview.chromium.org/2055823003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/2055823003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:32:
this.PermissionValues = settings.PermissionValues;
When I implemented this initially, something didn't work when this was set in
ready(), but I can't seem to find anything broken now -- so I changed it to
ready.

[Finnur, 2016-06-16 13:53:20]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-06-16 13:53:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2055823003/#ps140001
(title: "Ready")

[commit-bot: I haz the power, 2016-06-16 13:53:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2055823003/140001

[commit-bot: I haz the power, 2016-06-16 14:55:37]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-06-16 14:57:28]

Description was changed from

==========
Site Settings Desktop: Implement the 'Clear on exit' menu for Cookies.

BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Site Settings Desktop: Implement the 'Clear on exit' menu for Cookies.

BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/67798eab1c7003952ba4176f2e2809df6425395f
Cr-Commit-Position: refs/heads/master@{#400154}
==========

[commit-bot: I haz the power, 2016-06-16 14:57:29]

Patchset 7 (id:??) landed as
https://crrev.com/67798eab1c7003952ba4176f2e2809df6425395f
Cr-Commit-Position: refs/heads/master@{#400154}