Site Settings Desktop: Show the type of cookie in dropdown (instead of url).

Site Settings Desktop: Show the type of cookie in dropdown (instead of url).

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

Committed: https://crrev.com/938114be1ecffa98a68ededc466b85ab6b16dcd8
Cr-Commit-Position: refs/heads/master@{#414059}

[Finnur, 2016-08-23 10:10:05]

Description was changed from

==========
Site Settings Desktop: Show the type of cookie in dropdown (instead of url).

BUG=628760, 614277
==========

to

==========
Site Settings Desktop: Show the type of cookie in dropdown (instead of url).

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

[Finnur, 2016-08-23 10:10:13]

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

[commit-bot: I haz the power, 2016-08-23 10:10:37]

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

[Finnur, 2016-08-23 10:11:53]

Description was changed from

==========
Site Settings Desktop: Show the type of cookie in dropdown (instead of url).

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

to

==========
Site Settings Desktop: Show the type of cookie in dropdown (instead of url).

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

[Finnur, 2016-08-23 10:11:53]

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

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

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

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

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dschuyler, 2016-08-23 18:53:39]

All the comments are optional suggestions/opinions.

LGTM.

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

https://codereview.chromium.org/2268273002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:74: }
Suggestion: The above is ok, but it might be nicer
to read and quicker to execute if it were changed
to a dictionary/object with entries like:

  'cookie': loadTimeData.getString('cookieSingular'),
  'database': loadTimeData.getString('cookieDatabaseStorage'),
  'local_storage': loadTimeData.getString('cookieLocalStorage'),
  'indexed_db': loadTimeData.getString('cookieLocalStorage'),
  [...]

and then the if/else chain were shorter, like: 

if (dataType == 'cookie' && cookieCount > 1)
  category = loadTimeData.getStringF('cookiePlural', cookieCount);
else if (dataType == 'quota')
  category = totalUsage;
else
  category = this.categoryLabels[dataType];

Just an optional suggestion.

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

https://codereview.chromium.org/2268273002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data_details_dialog.js:167:
return getCookieDataCategoryText(item.data.type, 0, item.data.totalUsage);
Suggestion: just preference, but if 
getCookieDataCategoryText is going to have
special handling for 'cookie' in the two 
places it's used, it may make sense to move
the conditional for cookieCount > 1 out of
getCookieDataCategoryText and handle that
case outside of the function. That would 
simplify getCookieDataCategoryText and there 
would be a symmetry in handling special cases 
outside of the function in both cases.

But it's just an opinion/suggestion.

[Finnur, 2016-08-24 11:31:15]

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

https://codereview.chromium.org/2268273002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:74: }
I like that suggestion. Closure, doesn't quite like the use of |this| in a
function like that, but I moved the dictionary object outside the function and
that works fine. Will check in that way, but happy to augment if you think it is
necessary.

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

https://codereview.chromium.org/2268273002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_data_details_dialog.js:167:
return getCookieDataCategoryText(item.data.type, 0, item.data.totalUsage);
That's reasonable. Done.

[Finnur, 2016-08-24 11:31:28]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-08-24 11:31:28]

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/2268273002/#ps40001
(title: "Polish")

[commit-bot: I haz the power, 2016-08-24 11:31:58]

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-24 13:03:03]

Description was changed from

==========
Site Settings Desktop: Show the type of cookie in dropdown (instead of url).

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

to

==========
Site Settings Desktop: Show the type of cookie in dropdown (instead of url).

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

[commit-bot: I haz the power, 2016-08-24 13:03:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-24 13:06:02]

Description was changed from

==========
Site Settings Desktop: Show the type of cookie in dropdown (instead of url).

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

to

==========
Site Settings Desktop: Show the type of cookie in dropdown (instead of url).

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

Committed: https://crrev.com/938114be1ecffa98a68ededc466b85ab6b16dcd8
Cr-Commit-Position: refs/heads/master@{#414059}
==========

[commit-bot: I haz the power, 2016-08-24 13:06:03]

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