Site Settings Desktop: Implement the cookies list and details dialog.

Site Settings Desktop: Implement the cookies list and details dialog.

Might require some UI polish follow-up, but the functionally should work.

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

Committed: https://crrev.com/86813d9354a393874f5af0cf79a99f67b73ca348
Cr-Commit-Position: refs/heads/master@{#404358}

[Finnur, 2016-07-01 10:59:00]

Description was changed from

==========
Site Settings Desktop: Implement the cookies list and details dialog.

Might require some UI polish follow-up, but the functionally should work.

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

to

==========
Site Settings Desktop: Implement the cookies list and details dialog.

Might require some UI polish follow-up, but the functionally should work.

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

[Finnur, 2016-07-01 11:30:48]

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

[Finnur, 2016-07-01 11:30:49]

PTAL.

[Finnur, 2016-07-04 11:48:50]

Friendly ping...

[michaelpg, 2016-07-04 19:44:22]

screenshots would be helpful

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/cookie_tree_node.js
(right):

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:32:
'cookie': [['name', 'label_cookie_name'],
Yikes. Can we generate this in C++ instead of duplicating it all?

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:80:
this.data = data;
throughout: trailing underscores for private members

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:91: *
Converts a list of cookies and add them as CookieTreenode children to
Node

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:93: * @param
{settings.CookieTreeNode} parentNode The parent node to add
{!...}

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:100: return
new settings.CookieTreeNode(x);
2-space indent for function

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:138: while
(count--)
node.children.splice(firstChild, count) ?

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:144: *
@return Array< {CookieDataItem} > The Cookie List.
nit: no spaces <{...}>

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:150: for
(var j = 0; j < group.children.length; ++j) {
opt nit: for-of for nested loops for readability:
    for (var group of this.children) {
      for (var cookie of group.children) {

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:251: var key
= item.data[cookieInfo[type][i][0]];
now my brain hurts >_<

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data.js (right):

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data.js:7: * 'media-picker'
handles showing the dropdown allowing users to select the
update

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data.js:19: * @type
Array<CookieDataSummaryItem>.
@type {Array...}

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data_details_dialog.js
(right):

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data_details_dialog.js:54:
this.listener_ = cr.addWebUIListener(
cr => this?

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc (right):

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc:1: //
Copyright (c) 2016 The Chromium Authors. All rights reserved.
remove (c)

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/settings_cookies_view_handler.h (right):

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/settings_cookies_view_handler.h:1: // Copyright
(c) 2016 The Chromium Authors. All rights reserved.
remove (c)

[Finnur, 2016-07-05 11:04:49]

Thank you for taking a look during your holiday. It was quite unexpected but
greatly appreciated, especially since my upcoming vacation is starting this
weekend and this helps me whittle down the task list a bit.

PTAL.

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/cookie_tree_node.js
(right):

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:32:
'cookie': [['name', 'label_cookie_name'],
So... couple of comments. But first some background...

This list serves three purposes:

1) It lists what subset of the cookie data we want to show in the UI. For
example, we show 'content', 'domain' and 'path',  etc (as seen right below) but
we don't show: 'hasChildren', 'icon', 'id', 'name', 'title', or 'type' (because
they are omitted from this list).
2) It determines the order in which to show the data ('domain' before 'path',
for example, and 'created' before 'expired').
3) It determines which user friendly label to use to prefix the data with.

The way I see it, the list is always going to be manually curated (because it is
a subset in each case and order is important) and I see a few downsides to
moving it to C++:
a) Extra work and extra code.
b) This is primarily to look up what label to show, meaning it is purely UI code
that belongs with the remaining UI code.
c) It is copied from cookie_list.js and it would be slightly harder to track
changes to it if converted to C++.

Have I convinced you to keep it here? :)

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:80:
this.data = data;
On 2016/07/04 19:44:20, michaelpg wrote:
> throughout: trailing underscores for private members

Done.

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:91: *
Converts a list of cookies and add them as CookieTreenode children to
On 2016/07/04 19:44:21, michaelpg wrote:
> Node

Done.

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:93: * @param
{settings.CookieTreeNode} parentNode The parent node to add
On 2016/07/04 19:44:21, michaelpg wrote:
> {!...}

Done.

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:100: return
new settings.CookieTreeNode(x);
On 2016/07/04 19:44:20, michaelpg wrote:
> 2-space indent for function

Done.

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:138: while
(count--)
Bah. Of course. Done. :)

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:144: *
@return Array< {CookieDataItem} > The Cookie List.
The presubmit check barfs on that...

Found JavaScript style violations in
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:
  line 143: E0002: Missing space before "> The Cookie List."
     * @return Array<{!CookieDataItem}> The Cookie List.
                                      ^^^^^^^^^^^^^^^^^^
  line 143: E0002: Missing space before "{"
     * @return Array<{!CookieDataItem}> The Cookie List.
                     ^

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:150: for
(var j = 0; j < group.children.length; ++j) {
On 2016/07/04 19:44:21, michaelpg wrote:
> opt nit: for-of for nested loops for readability:
>     for (var group of this.children) {
>       for (var cookie of group.children) {

Done.

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:251: var key
= item.data[cookieInfo[type][i][0]];
I've made an attempt at commenting what is going on here. See if it helps.

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data.js (right):

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data.js:7: * 'media-picker'
handles showing the dropdown allowing users to select the
On 2016/07/04 19:44:21, michaelpg wrote:
> update

Done.

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data.js:19: * @type
Array<CookieDataSummaryItem>.
On 2016/07/04 19:44:21, michaelpg wrote:
> @type {Array...}

Done.

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data_details_dialog.js
(right):

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data_details_dialog.js:54:
this.listener_ = cr.addWebUIListener(
I'm not sure what you mean here. A straight s/cr/this/ gives a Closure error due
to assigning |undefined| to listener_.

Also, the reason I'm using cr.addWebUIListener is so I can manually unregister
(line 161) when the dialog is going away. The documentation for
WebUIListenerBehavior says that for such cases I should use the class directly.

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc (right):

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc:1: //
Copyright (c) 2016 The Chromium Authors. All rights reserved.
On 2016/07/04 19:44:21, michaelpg wrote:
> remove (c) 

Done.

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/settings_cookies_view_handler.h (right):

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/settings_cookies_view_handler.h:1: // Copyright
(c) 2016 The Chromium Authors. All rights reserved.
On 2016/07/04 19:44:21, michaelpg wrote:
> remove (c) 

Done.

[Finnur, 2016-07-05 11:05:11]

Also, check bug for screenshot.

[Finnur, 2016-07-05 14:00:19]

Addendum:

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/settings/site_settings/site_data_details_dialog.html
(right):

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data_details_dialog.html:37:
class="cancel-button remove-button">
The latest mocks have this button in the same style as the cancel button (not
blue) and I took the opportunity and also aligned it better vertically. (post
screenshot)

[michaelpg, 2016-07-05 18:01:11]

On 2016/07/05 11:04:49, Finnur wrote:
> Thank you for taking a look during your holiday. It was quite unexpected but
> greatly appreciated, especially since my upcoming vacation is starting this
> weekend and this helps me whittle down the task list a bit.

NP, I have time and I had intended to get to this on Friday so you wouldn't have
to wait basically a week.

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/cookie_tree_node.js
(right):

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:32:
'cookie': [['name', 'label_cookie_name'],
On 2016/07/05 11:04:49, Finnur wrote:
> So... couple of comments. But first some background...
> 
> This list serves three purposes:
> 
> 1) It lists what subset of the cookie data we want to show in the UI. For
> example, we show 'content', 'domain' and 'path',  etc (as seen right below)
but
> we don't show: 'hasChildren', 'icon', 'id', 'name', 'title', or 'type'
(because
> they are omitted from this list).

The comment doesn't make that clear (that this was a curated subset rather than
a pure mapping from an existing structure in C++). Having that info would help
explain why this is, and help me decide whether I need to add something here
when I go to add a cookie type or metadata or whatever to Chrome.
-
> 2) It determines the order in which to show the data ('domain' before 'path',
> for example, and 'created' before 'expired').
> 3) It determines which user friendly label to use to prefix the data with.

If we replace $i18n{} template strings in JavaScript, we could do that for the
labels so the lookup is performed once by the (probably faster) C++ template
code, rather than every time the list updates. Perhaps ask dschuyler if that's
feasible, because:
https://cs.chromium.org/chromium/src/content/browser/webui/web_ui_data_source...

> 
> The way I see it, the list is always going to be manually curated (because it
is
> a subset in each case and order is important) and I see a few downsides to
> moving it to C++:
> a) Extra work and extra code.
> b) This is primarily to look up what label to show, meaning it is purely UI
code
> that belongs with the remaining UI code.
> c) It is copied from cookie_list.js and it would be slightly harder to track
> changes to it if converted to C++.
> 
> Have I convinced you to keep it here? :)

Yep, sounds fine. I would slightly prefer having it in its own JS file though,
since it's just a big chunk of (almost) JSON.

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:144: *
@return Array< {CookieDataItem} > The Cookie List.
On 2016/07/05 11:04:49, Finnur wrote:
> The presubmit check barfs on that...
> 
> Found JavaScript style violations in
> chrome/browser/resources/settings/site_settings/cookie_tree_node.js:
>   line 143: E0002: Missing space before "> The Cookie List."
>      * @return Array<{!CookieDataItem}> The Cookie List.
>                                       ^^^^^^^^^^^^^^^^^^
>   line 143: E0002: Missing space before "{"
>      * @return Array<{!CookieDataItem}> The Cookie List.
>                      ^

try wrapping Array in {}

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:251: var key
= item.data[cookieInfo[type][i][0]];
On 2016/07/05 11:04:49, Finnur wrote:
> I've made an attempt at commenting what is going on here. See if it helps.

Yes, very much!

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data_details_dialog.js
(right):

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data_details_dialog.js:54:
this.listener_ = cr.addWebUIListener(
On 2016/07/05 11:04:49, Finnur wrote:
> I'm not sure what you mean here. A straight s/cr/this/ gives a Closure error
due
> to assigning |undefined| to listener_.
> 
> Also, the reason I'm using cr.addWebUIListener is so I can manually unregister
> (line 161) when the dialog is going away. The documentation for
> WebUIListenerBehavior says that for such cases I should use the class
directly.

Noted. Then can we remove WebUIListenerBehavior?

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/cookie_tree_node.js
(right):

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:247: var
type = item.data_.type;
I still suggest some cleanup to avoid the [[][][]]:

  var fields = cookieInfo[item.data_.type];
  for (var field of fields) {
    // Iterate...
    var key = field[0];
    if (item.data_[key].length > 0) {
      ...

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:252: var key
= item.data_[cookieInfo[type][i][0]];
this is the value, not the key (as evidenced below)

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data.html (right):

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data.html:26: <div
class="favicon-image" style$="[[computeSiteIcon(item.site)]]"
the screenshot looks like the favicon is not aligned with the text - could you
check?

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data.js (right):

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data.js:25: * @type
{!settings.CookieTreeNode}.
nope :)

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/settings/site_settings/site_data_details_dialog.html
(right):

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data_details_dialog.html:23:
<paper-dropdown-menu>
should this be disabled or something when there's only one item?

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data_details_dialog.html:49:
<paper-button class="action-button" dialog-dismiss
dialog-confirm? (though I don't think it really matters)

[Finnur, 2016-07-06 09:57:35]

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/cookie_tree_node.js
(right):

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:32:
'cookie': [['name', 'label_cookie_name'],
I've updated the comment and moved it to a separate js file. I'll ask dschuyler
and do a follow-up CL if feasible.

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:144: *
@return Array< {CookieDataItem} > The Cookie List.
On 2016/07/05 18:01:11, michaelpg wrote:
> On 2016/07/05 11:04:49, Finnur wrote:
> > The presubmit check barfs on that...
> > 
> > Found JavaScript style violations in
> > chrome/browser/resources/settings/site_settings/cookie_tree_node.js:
> >   line 143: E0002: Missing space before "> The Cookie List."
> >      * @return Array<{!CookieDataItem}> The Cookie List.
> >                                       ^^^^^^^^^^^^^^^^^^
> >   line 143: E0002: Missing space before "{"
> >      * @return Array<{!CookieDataItem}> The Cookie List.
> >                      ^
> 
> try wrapping Array in {}

Done.

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data_details_dialog.js
(right):

https://codereview.chromium.org/2115833003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data_details_dialog.js:54:
this.listener_ = cr.addWebUIListener(
Ah, yes! Done.

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/cookie_tree_node.js
(right):

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:247: var
type = item.data_.type;
On 2016/07/05 18:01:11, michaelpg wrote:
> I still suggest some cleanup to avoid the [[][][]]:
> 
>   var fields = cookieInfo[item.data_.type];
>   for (var field of fields) {
>     // Iterate...
>     var key = field[0];
>     if (item.data_[key].length > 0) {
>       ...

Done.

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_tree_node.js:252: var key
= item.data_[cookieInfo[type][i][0]];
Fair enough.

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data.html (right):

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data.html:26: <div
class="favicon-image" style$="[[computeSiteIcon(item.site)]]"
Done.

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data.js (right):

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data.js:25: * @type
{!settings.CookieTreeNode}.
yup. 

:)

... which is another way of saying: I'm not sure I follow. :)

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/settings/site_settings/site_data_details_dialog.html
(right):

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data_details_dialog.html:23:
<paper-dropdown-menu>
It's hidden when there's only one item (see populateDialog_).

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data_details_dialog.html:49:
<paper-button class="action-button" dialog-dismiss
On 2016/07/05 18:01:11, michaelpg wrote:
> dialog-confirm? (though I don't think it really matters)

Done.

[dschuyler, 2016-07-06 17:19:38]

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/cookie_info.js (right):

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_info.js:5: // This
structure maps the various cookie type names from C++ (hence the
Instead of having any mapping like this, could these
be inlined into HTML and use $i18n{}? Maybe a 
dom-module for the cookies - that may be more
Polymerific, imo. This looks like it's doing it the old
options way of doing things before we had Polymer
and $i18n{} replacements. I may be missing something, 
please let me know if what I'm suggesting doesn't
work.

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_info.js:6: //
underscores) to arrays of the different types of data each has, along with
Are the underscore labels passed from c++ to JS? It *looks* like
these are only used to map the i18n text. If that's the case they
can be camelCase instead(?).

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_info.js:13: 'cookie':
[['name', 'label_cookie_name'],
Having 'label_' on each is not informative imo. :)
Maybe 'site settings' instead of 'label' or omitting
the prefix altogether?

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_info.js:46: ['modified',
'label_cache_storage_last_modified']],
The indent looks off here.

[michaelpg, 2016-07-06 17:31:00]

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data.js (right):

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data.js:18: * @type
{Array<CookieDataSummaryItem>}.
here too, no period after the {}

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data.js:25: * @type
{!settings.CookieTreeNode}.
On 2016/07/06 09:57:35, Finnur wrote:
> yup. 
> 
> :)
> 
> ... which is another way of saying: I'm not sure I follow. :)

ah, i thought closure would barf on this too. No period after the {}

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data_details_dialog.js
(right):

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data_details_dialog.js:71:
this.$.clear.textContent =
when do you "undo" these changes to hidden and textContent? eg, if I open the
site in another tab and it adds cookies, i'd expect the container to be shown

[Finnur, 2016-07-07 10:54:01]

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data.js (right):

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data.js:18: * @type
{Array<CookieDataSummaryItem>}.
On 2016/07/06 17:31:00, michaelpg wrote:
> here too, no period after the {}

Done.

https://codereview.chromium.org/2115833003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data.js:25: * @type
{!settings.CookieTreeNode}.
On 2016/07/06 17:31:00, michaelpg wrote:
> On 2016/07/06 09:57:35, Finnur wrote:
> > yup. 
> > 
> > :)
> > 
> > ... which is another way of saying: I'm not sure I follow. :)
> 
> ah, i thought closure would barf on this too. No period after the {}

Done.

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/cookie_info.js (right):

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_info.js:5: // This
structure maps the various cookie type names from C++ (hence the
I'm sure it would be an improvement, even though I'm not entirely sure how to
approach it. But it is getting to be a moot point because I'm quickly running
out of time (my vacation starts at EOD tomorrow) and I'd like to get this (and
hopefully another CL) out of the way. I guess I'm asking if we can punt on this
for now? :)

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_info.js:6: //
underscores) to arrays of the different types of data each has, along with
This comment refers to the keys in this structure, e.g. 'flash_lso' at the
bottom. That's not part of the i18n.

I have, though, changed the i18n *labels* to use camelCase. I don't think you
were asking for more (e.g. for me to change e.g. the 'flash_lso' *key* to
'flashLso'). So: If you are happy with the change I made here, you can stop
reading. Otherwise:

The C++ code creates a list of entries, each represented as a dictionary with
keys like 'flash_lso'. See for example:
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/cookies_tree_mod...

To change the keys here to camelCase, I'd have to change the constants in C++ to
match and I have no idea what breaks if I do that.

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_info.js:13: 'cookie':
[['name', 'label_cookie_name'],
On 2016/07/06 17:19:38, dschuyler wrote:
> Having 'label_' on each is not informative imo. :)
> Maybe 'site settings' instead of 'label' or omitting
> the prefix altogether?

Done.

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/cookie_info.js:46: ['modified',
'label_cache_storage_last_modified']],
On 2016/07/06 17:19:38, dschuyler wrote:
> The indent looks off here.

Done.

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data_details_dialog.js
(right):

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data_details_dialog.js:71:
this.$.clear.textContent =
So... The list of data stored by the website is mostly static, meaning: you'd
have to refresh the page to see changes. The exception is when cookies are
deleted -- that's reflected right away (it would be really weird if it didn't).

The reason the list is static is mostly down to priorities and time remaining:
a) The CL had gotten large and complex enough to review without also including
dynamic updating for when cookies are added.
b) If the CL were larger I wouldn't have been able to complete the CL in time
for my vacation.
c) Dynamically updating is probably lower priority than some of the other Site
Settings things that need to happen for this to launch (e.g. the categories for
USB, protocol handlers and Zoom are currently missing).

[michaelpg, 2016-07-07 20:21:29]

lgtm

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data_details_dialog.js
(right):

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data_details_dialog.js:71:
this.$.clear.textContent =
On 2016/07/07 10:54:01, Finnur wrote:
> So... The list of data stored by the website is mostly static, meaning: you'd
> have to refresh the page to see changes. The exception is when cookies are
> deleted -- that's reflected right away (it would be really weird if it
didn't).
> 
> The reason the list is static is mostly down to priorities and time remaining:
> a) The CL had gotten large and complex enough to review without also including
> dynamic updating for when cookies are added.
> b) If the CL were larger I wouldn't have been able to complete the CL in time
> for my vacation.
> c) Dynamically updating is probably lower priority than some of the other Site
> Settings things that need to happen for this to launch (e.g. the categories
for
> USB, protocol handlers and Zoom are currently missing).

Acknowledged. Could you please open a separate bug to track this?

[Finnur, 2016-07-08 09:40:47]

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_data_details_dialog.js
(right):

https://codereview.chromium.org/2115833003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_data_details_dialog.js:71:
this.$.clear.textContent =
The bug for the missing settings is here: http://crbug.com/614277
The new bug for dynamically updating the Cookie Data list is here:
http://crbug.com/626619

[Finnur, 2016-07-08 09:40:57]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2016-07-08 09:42:05]

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-07-08 14:12:47]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-07-08 14:13:01]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 14:15:49]

Description was changed from

==========
Site Settings Desktop: Implement the cookies list and details dialog.

Might require some UI polish follow-up, but the functionally should work.

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

to

==========
Site Settings Desktop: Implement the cookies list and details dialog.

Might require some UI polish follow-up, but the functionally should work.

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

Committed: https://crrev.com/86813d9354a393874f5af0cf79a99f67b73ca348
Cr-Commit-Position: refs/heads/master@{#404358}
==========

[commit-bot: I haz the power, 2016-07-08 14:15:51]

Patchset 11 (id:??) landed as
https://crrev.com/86813d9354a393874f5af0cf79a99f67b73ca348
Cr-Commit-Position: refs/heads/master@{#404358}