[ChromeOS] Implement collected cookies in webui.

[ChromeOS] Implement collected cookies in webui.

- Allow ConstrainedWindowGtkDelegate to specify whether it needs padding. And webui based one use no padding so that the padding could be controled from html side;
- Resurrected cookies_tree.js for the collected cookies UI and patched a bit so that we could have multiple instances in one html;
- Added CookiesTreeModelAdapter to work with the JS CookiesTree;
- Implemented the collected cookies UI in CollectedCookiesUIDelegate;

BUG=chromium-os:8134
TEST=Verify webui based cookie prompt work as expected.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=77853

[Mike Mammarella, 2011-03-09 03:21:53]

On 2011/03/08 20:14:34, xiyuan wrote:

I'm probably not the right reviewer for this. While I did just rewrite the
cookies page, I'm still pretty new at WebUI. Maybe try arv?

[xiyuan, 2011-03-09 18:23:38]

arv is more than welcome to do a drive-by review.

Oshima, do you think you could help to review this? Thanks.

[oshima_google, 2011-03-09 18:40:58]

I'm working R11 issue now and phone interview today, so
I may or may not have time today. I probably can do it tonight,
if it's okay with you.

- oshima

On 2011/03/09 18:23:38, xiyuan wrote:
> arv is more than welcome to do a drive-by review.
> 
> Oshima, do you think you could help to review this? Thanks.

[arv (Not doing code reviews), 2011-03-09 18:42:47]

I'm curious why we have this as well as the cookies list in the preferences?

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
File chrome/browser/resources/collected_cookies.css (right):

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.css:6: .hidden {
Can you use the hidden property instead?

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.css:20: padding: 0;
padding 0 should be default already?

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
File chrome/browser/resources/collected_cookies.html (right):

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:22: <div id="info-banner"
class="hidden">
<hx> maybe?

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:23: <span id="info-text"></span>
Do you need the span here?

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:26: <div
i18n-content="allowedCookies"></div>
p, hx?

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:27: <tree id="allowedCookies"
class="cookies-tree"></tree>
id="allowed-cookies"

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:35: <button
onclick="closeDialog();">Close Me</button>
i18n?

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:35: <button
onclick="closeDialog();">Close Me</button>
move this onclick to js as well?

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
File chrome/browser/resources/collected_cookies.js (right):

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:1: function closeDialog() {
Copyright header

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:2: chrome.send('DialogClose',
['']);
Pass empty array instead?

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:2: chrome.send('DialogClose',
['']);
dialogClose for consistency with the rest of the chrome.send messages?

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:8: if (text.length) {
$('info-banner').hidden = text.length;

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:57: if (e.keyCode == 27)
Use keyIdentifier or add a comment what 27 is for

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/coo...
File chrome/browser/resources/cookies_tree.js (right):

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:4: 
Do you mind calling out what was changed here? I can patch and manually diff but
it might be easier for you to call out what changed.

[xiyuan, 2011-03-09 21:33:31]

It makes sense to have one cookies list. But I'd like to address that in a
follow up CL and keep this one just translate existing into webui. It's already
kind of big.

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
File chrome/browser/resources/collected_cookies.css (right):

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.css:6: .hidden {
On 2011/03/09 18:42:47, arv wrote:
> Can you use the hidden property instead?

Done.

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.css:20: padding: 0;
On 2011/03/09 18:42:47, arv wrote:
> padding 0 should be default already?

Done.

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
File chrome/browser/resources/collected_cookies.html (right):

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:22: <div id="info-banner"
class="hidden">
On 2011/03/09 18:42:47, arv wrote:
> <hx> maybe?

<hx>'s before and after space seems to be too much.

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:23: <span id="info-text"></span>
On 2011/03/09 18:42:47, arv wrote:
> Do you need the span here?

Removed.

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:26: <div
i18n-content="allowedCookies"></div>
On 2011/03/09 18:42:47, arv wrote:
> p, hx?
<p> used.

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:27: <tree id="allowedCookies"
class="cookies-tree"></tree>
On 2011/03/09 18:42:47, arv wrote:
> id="allowed-cookies"

Done.

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:35: <button
onclick="closeDialog();">Close Me</button>
On 2011/03/09 18:42:47, arv wrote:
> i18n?

Oops. Forgot to update this. Fixed now.

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:35: <button
onclick="closeDialog();">Close Me</button>
On 2011/03/09 18:42:47, arv wrote:
> move this onclick to js as well?

Done.

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
File chrome/browser/resources/collected_cookies.js (right):

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:1: function closeDialog() {
On 2011/03/09 18:42:47, arv wrote:
> Copyright header

Done.

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:2: chrome.send('DialogClose',
['']);
On 2011/03/09 18:42:47, arv wrote:
> dialogClose for consistency with the rest of the chrome.send messages?

'DialogClose' is a HTML dialog close message that used in underlying
ConstrainedHtmlUI. I have changed all other messages defined by this webui for
follow this convention.

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:2: chrome.send('DialogClose',
['']);
On 2011/03/09 18:42:47, arv wrote:
> Pass empty array instead?

Could not unless we change the underlying ConstrainedHtmlUI and HtmlDialogUI
code to also expect an empty list.

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:8: if (text.length) {
On 2011/03/09 18:42:47, arv wrote:
> $('info-banner').hidden = text.length;

Done.

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:57: if (e.keyCode == 27)
On 2011/03/09 18:42:47, arv wrote:
> Use keyIdentifier or add a comment what 27 is for

Comment added. keyIdentifier for ESC is 'U+001B'.

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/coo...
File chrome/browser/resources/cookies_tree.js (right):

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:4: 
On 2011/03/09 18:42:47, arv wrote:
> Do you mind calling out what was changed here? I can patch and manually diff
but
> it might be easier for you to call out what changed.

Here's summary of what I changed:
- Change namespace from "option" -> "ui";
- Make shared treeLookup a per-tree map;
- Add a per-tree loadChildrenMessage_ so that a tree node could directly call
into its backend cookies tree model adapter;
- Add interface functions CookiesTree.setCallback, CookiesTree.onTreeItemAdded
etc so that CookiesTreeModelAdapter could talk to JS tree;
- Renaming CookieTreeItem -> CookiesTreeItem (added 's' after 'Cookie');

Full diff is here: http://www/~xiyuan/no_crawl/files/cookies_tree.diff.txt

[Evan Stade, 2011-03-09 22:57:21]

did you want me to look at anything besides the gtk files?

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/gtk/constr...
File chrome/browser/ui/gtk/constrained_window_gtk.h (right):

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/gtk/constr...
chrome/browser/ui/gtk/constrained_window_gtk.h:43: virtual gfx::Insets
GetInsets() const;
can we make this instead return a boolean, i.e. ShouldHaveBorderPadding? I think
there are really only 2 cases, the standard border width and no border width.

[oshima, 2011-03-10 15:41:22]

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/coll...
File chrome/browser/ui/webui/collected_cookies_ui_delegate.cc (right):

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.cc:242: DCHECK(type ==
NotificationType::COLLECTED_COOKIES_SHOWN);
why not DCHECK_EQ?

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/coll...
File chrome/browser/ui/webui/collected_cookies_ui_delegate.h (right):

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.h:1: // Copyright (c) 2010
The Chromium Authors. All rights reserved.
2011 (and the rest)

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.h:5: #ifndef
CHROME_BROWSER_UI_WEBUI_COLLECTED_COOKIES_UI_H_
... COOKIES_UI_DELEGATE_H

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.h:21: }
move this after toplevel namespace

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/cook...
File chrome/browser/ui/webui/cookies_tree_model_adapter.cc (right):

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/cook...
chrome/browser/ui/webui/cookies_tree_model_adapter.cc:44: CookiesTreeModel*
model) {
indent

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/cook...
chrome/browser/ui/webui/cookies_tree_model_adapter.cc:121: }
nuke {}

[xiyuan, 2011-03-10 17:27:04]

CL updated to address Evan and Oshima's comments.

Evan, you are welcome to review all parts. If you are busy, it's okay to just do
the gtk part. And arv could concentrate on html and related code.

Oshima please help with the rest of the change.

Thanks a lot, guys.

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/gtk/constr...
File chrome/browser/ui/gtk/constrained_window_gtk.h (right):

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/gtk/constr...
chrome/browser/ui/gtk/constrained_window_gtk.h:43: virtual gfx::Insets
GetInsets() const;
On 2011/03/09 22:57:21, Evan Stade wrote:
> can we make this instead return a boolean, i.e. ShouldHaveBorderPadding? I
think
> there are really only 2 cases, the standard border width and no border width.

Done.

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/coll...
File chrome/browser/ui/webui/collected_cookies_ui_delegate.cc (right):

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.cc:242: DCHECK(type ==
NotificationType::COLLECTED_COOKIES_SHOWN);
On 2011/03/10 15:41:22, oshima wrote:
> why not DCHECK_EQ?

Done.

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/coll...
File chrome/browser/ui/webui/collected_cookies_ui_delegate.h (right):

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.h:1: // Copyright (c) 2010
The Chromium Authors. All rights reserved.
On 2011/03/10 15:41:22, oshima wrote:
> 2011 (and the rest)

Done.

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.h:5: #ifndef
CHROME_BROWSER_UI_WEBUI_COLLECTED_COOKIES_UI_H_
On 2011/03/10 15:41:22, oshima wrote:
> ... COOKIES_UI_DELEGATE_H

Done.

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.h:21: }
On 2011/03/10 15:41:22, oshima wrote:
> move this after toplevel namespace

Done.

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/cook...
File chrome/browser/ui/webui/cookies_tree_model_adapter.cc (right):

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/cook...
chrome/browser/ui/webui/cookies_tree_model_adapter.cc:44: CookiesTreeModel*
model) {
On 2011/03/10 15:41:22, oshima wrote:
> indent

Done.

http://codereview.chromium.org/6644002/diff/5002/chrome/browser/ui/webui/cook...
chrome/browser/ui/webui/cookies_tree_model_adapter.cc:121: }
On 2011/03/10 15:41:22, oshima wrote:
> nuke {}

Done.

[oshima, 2011-03-10 17:46:11]

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/ui/gtk/constr...
File chrome/browser/ui/gtk/constrained_window_gtk.cc (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/ui/gtk/constr...
chrome/browser/ui/gtk/constrained_window_gtk.cc:13: #include "ui/gfx/insets.h"
this is no longer necessary, right?

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/ui/gtk/constr...
File chrome/browser/ui/gtk/constrained_window_gtk.h (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/ui/gtk/constr...
chrome/browser/ui/gtk/constrained_window_gtk.h:39: virtual bool
ShouldHaveBorderPadding() const;
Comment?

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/ui/webui/coll...
File chrome/browser/ui/webui/collected_cookies_ui_delegate.h (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.h:14: #include
"chrome/browser/ui/webui/cookies_tree_model_adapter.h"
sort include

[xiyuan, 2011-03-10 17:54:50]

CL updated to address Oshima's comments #2.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/ui/gtk/constr...
File chrome/browser/ui/gtk/constrained_window_gtk.cc (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/ui/gtk/constr...
chrome/browser/ui/gtk/constrained_window_gtk.cc:13: #include "ui/gfx/insets.h"
On 2011/03/10 17:46:11, oshima wrote:
> this is no longer necessary, right?

That's right. Removed.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/ui/gtk/constr...
File chrome/browser/ui/gtk/constrained_window_gtk.h (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/ui/gtk/constr...
chrome/browser/ui/gtk/constrained_window_gtk.h:39: virtual bool
ShouldHaveBorderPadding() const;
On 2011/03/10 17:46:11, oshima wrote:
> Comment?

Done.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/ui/webui/coll...
File chrome/browser/ui/webui/collected_cookies_ui_delegate.h (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.h:14: #include
"chrome/browser/ui/webui/cookies_tree_model_adapter.h"
On 2011/03/10 17:46:11, oshima wrote:
> sort include

Done.

[arv (Not doing code reviews), 2011-03-10 18:03:00]

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
File chrome/browser/resources/collected_cookies.html (right):

http://codereview.chromium.org/6644002/diff/2001/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:22: <div id="info-banner"
class="hidden">
On 2011/03/09 21:33:31, xiyuan wrote:
> On 2011/03/09 18:42:47, arv wrote:
> > <hx> maybe?
> 
> <hx>'s before and after space seems to be too much.

That should be fixed with css margins.

In general it is better to use the more semantically correct elements

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/browser_resou...
File chrome/browser/browser_resources.grd (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/browser_resou...
chrome/browser/browser_resources.grd:31: <include
name="IDR_COLLECTED_COOKIES_HTML" file="resources\collected_cookies.html"
flattenhtml="true" type="BINDATA" />
Do we want this on all platforms? I guess I still don't get it?

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
File chrome/browser/resources/collected_cookies.css (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.css:6: [hidden] {
This rule should not be needed (it might but in general it is not needed). If it
really is needed I would like a comment explaining why.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
File chrome/browser/resources/collected_cookies.html (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:20: <body oncontextmenu="return
false;">
move to script

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:20: <body oncontextmenu="return
false;">
Why do you want to disable the context menu?

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
File chrome/browser/resources/collected_cookies.js (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:24:
blockedCookies.selectedItem.data.type != 'origin';;
too many semicolons

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:33: if (!selected)
negate and remove return?

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:55: window.onkeydown = function(e)
{
document.onkeydown

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:74:
$('close-button').addEventListener('click', function(e) {
$('close-button').addEventListener('click', closeDialog);

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:78:
cr.defineProperty($('info-banner'), 'hidden', cr.PropertyKind.BOOL_ATTR);
You don't need this line. hidden is already available on all elements

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/coo...
File chrome/browser/resources/cookies_tree.js (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:98: /*
/**

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:101: treeLookup_: 0,
init to null instead

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:125: * @param {int} start Start index
of where to insert nodes.
{number}

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:185: * @type {String}
{string}

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:217: CookiesTree.setCallback =
function(args) {
can this be

CookiesTree.setCallback = function(id, message) {

instead?

[Evan Stade, 2011-03-10 18:27:59]

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/resources/coo...
File chrome/browser/resources/cookies_tree.js (right):

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:22: if (data.icon) {
no curlies

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:68: if (parent && parent instanceof
CookiesTreeItem) {
no curlies

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:129: if (!parent) {
no curlies

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:159: if (!parent) {
no curlies

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:207: if (!parent) {
no curlies

is there a legitimate case where parent would be null here? If not, I would say
it's best to avoid the early return. Noisy failure (which won't do anything
anyway, except stop execution) is better than silent failure. Same is true of
all the places in this file you check for parent.

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:229: CookiesTree.loadChildren =
function(args) {
this name is a little over-used, can you think of different names for clarity?
it's a C++ function, a static CookiesTree function, and a member function on
CookiesTree. Maybe the C++ function should be called requestChildren and the
static function here should be setChildren

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/ui/webui/cook...
File chrome/browser/ui/webui/cookies_tree_model_adapter.cc (right):

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/ui/webui/cook...
chrome/browser/ui/webui/cookies_tree_model_adapter.cc:77:
Value::CreateNullValue() :
indentation here is kinda weird, might be easier to read if you pulled the
parameters out of the Append() call into a local var

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/ui/webui/cook...
chrome/browser/ui/webui/cookies_tree_model_adapter.cc:119: if
(!args->GetString(0, &node_path))
this can never fail, right? So it should be a CHECK (no return), instead of a
silent failure

[oshima, 2011-03-10 18:39:09]

one more round.

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/ui/webui/coll...
File chrome/browser/ui/webui/collected_cookies_ui_delegate.cc (right):

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.cc:35: while (node) {
node && node->Get....
would be simpler.

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.cc:63: NOTREACHED();
you may wan to log setting in NOTREACHED

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.cc:184:
handlers->push_back(const_cast<WebUIMessageHandler*>(handler));
just curious. is handler necessary?

[xiyuan, 2011-03-10 19:23:59]

CL updated to address arv's comments in patch set 4 and even, oshima's comments
in patch set 5.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/browser_resou...
File chrome/browser/browser_resources.grd (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/browser_resou...
chrome/browser/browser_resources.grd:31: <include
name="IDR_COLLECTED_COOKIES_HTML" file="resources\collected_cookies.html"
flattenhtml="true" type="BINDATA" />
On 2011/03/10 18:03:00, arv wrote:
> Do we want this on all platforms? I guess I still don't get it?

Moved to chromeos only section.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
File chrome/browser/resources/collected_cookies.css (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.css:6: [hidden] {
On 2011/03/10 18:03:00, arv wrote:
> This rule should not be needed (it might but in general it is not needed). If
it
> really is needed I would like a comment explaining why.

It is indeed not needed. Removed.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
File chrome/browser/resources/collected_cookies.html (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:20: <body oncontextmenu="return
false;">
On 2011/03/10 18:03:00, arv wrote:
> move to script

Done.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.html:20: <body oncontextmenu="return
false;">
On 2011/03/10 18:03:00, arv wrote:
> Why do you want to disable the context menu?

This webui is in a constrained dialog. We don't want the context menu for webui
dialogs, right?

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
File chrome/browser/resources/collected_cookies.js (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:24:
blockedCookies.selectedItem.data.type != 'origin';;
On 2011/03/10 18:03:00, arv wrote:
> too many semicolons

Done.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:33: if (!selected)
On 2011/03/10 18:03:00, arv wrote:
> negate and remove return?

Done.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:55: window.onkeydown = function(e)
{
On 2011/03/10 18:03:00, arv wrote:
> document.onkeydown

Done. And moved it into load().

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:74:
$('close-button').addEventListener('click', function(e) {
On 2011/03/10 18:03:00, arv wrote:
> $('close-button').addEventListener('click', closeDialog);

Done.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/col...
chrome/browser/resources/collected_cookies.js:78:
cr.defineProperty($('info-banner'), 'hidden', cr.PropertyKind.BOOL_ATTR);
On 2011/03/10 18:03:00, arv wrote:
> You don't need this line. hidden is already available on all elements

Removed.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/coo...
File chrome/browser/resources/cookies_tree.js (right):

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:98: /*
On 2011/03/10 18:03:00, arv wrote:
> /**

Done.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:101: treeLookup_: 0,
On 2011/03/10 18:03:00, arv wrote:
> init to null instead

Done.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:125: * @param {int} start Start index
of where to insert nodes.
On 2011/03/10 18:03:00, arv wrote:
> {number}

Done.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:185: * @type {String}
On 2011/03/10 18:03:00, arv wrote:
> {string}

Done.

http://codereview.chromium.org/6644002/diff/5007/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:217: CookiesTree.setCallback =
function(args) {
On 2011/03/10 18:03:00, arv wrote:
> can this be
> 
> CookiesTree.setCallback = function(id, message) {
> 
> instead?

Done.

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:22: if (data.icon) {
On 2011/03/10 18:27:59, Evan Stade wrote:
> no curlies

Done.

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:68: if (parent && parent instanceof
CookiesTreeItem) {
On 2011/03/10 18:27:59, Evan Stade wrote:
> no curlies

Done.

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:129: if (!parent) {
On 2011/03/10 18:27:59, Evan Stade wrote:
> no curlies

Done.

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:159: if (!parent) {
On 2011/03/10 18:27:59, Evan Stade wrote:
> no curlies

Done.

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:207: if (!parent) {
On 2011/03/10 18:27:59, Evan Stade wrote:
> no curlies
> 
> is there a legitimate case where parent would be null here? If not, I would
say
> it's best to avoid the early return. Noisy failure (which won't do anything
> anyway, except stop execution) is better than silent failure. Same is true of
> all the places in this file you check for parent.

Think you are right. If all code runs as expected, parent should never be null.
Removed all parent check.

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/resources/coo...
chrome/browser/resources/cookies_tree.js:229: CookiesTree.loadChildren =
function(args) {
On 2011/03/10 18:27:59, Evan Stade wrote:
> this name is a little over-used, can you think of different names for clarity?
> it's a C++ function, a static CookiesTree function, and a member function on
> CookiesTree. Maybe the C++ function should be called requestChildren and the
> static function here should be setChildren

Good suggestions. Done.

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/ui/webui/coll...
File chrome/browser/ui/webui/collected_cookies_ui_delegate.cc (right):

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.cc:35: while (node) {
On 2011/03/10 18:39:09, oshima wrote:
> node && node->Get....
> would be simpler.

Done.

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.cc:63: NOTREACHED();
On 2011/03/10 18:39:09, oshima wrote:
> you may wan to log setting in NOTREACHED

Done.

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/ui/webui/coll...
chrome/browser/ui/webui/collected_cookies_ui_delegate.cc:184:
handlers->push_back(const_cast<WebUIMessageHandler*>(handler));
On 2011/03/10 18:39:09, oshima wrote:
> just curious. is handler necessary?

yep, you need two casts to convert this here to WebUIMessageHandler*.

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/ui/webui/cook...
File chrome/browser/ui/webui/cookies_tree_model_adapter.cc (right):

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/ui/webui/cook...
chrome/browser/ui/webui/cookies_tree_model_adapter.cc:77:
Value::CreateNullValue() :
On 2011/03/10 18:27:59, Evan Stade wrote:
> indentation here is kinda weird, might be easier to read if you pulled the
> parameters out of the Append() call into a local var

Wrapped the logic into a helper function to make it simpler.

http://codereview.chromium.org/6644002/diff/7004/chrome/browser/ui/webui/cook...
chrome/browser/ui/webui/cookies_tree_model_adapter.cc:119: if
(!args->GetString(0, &node_path))
On 2011/03/10 18:27:59, Evan Stade wrote:
> this can never fail, right? So it should be a CHECK (no return), instead of a
> silent failure

Done. Changed to CHECK.

[oshima, 2011-03-10 20:08:14]

LGTM on my bits

[arv (Not doing code reviews), 2011-03-11 00:30:20]

LGTM

http://codereview.chromium.org/6644002/diff/13001/chrome/browser/resources/co...
File chrome/browser/resources/cookies_tree.js (right):

http://codereview.chromium.org/6644002/diff/13001/chrome/browser/resources/co...
chrome/browser/resources/cookies_tree.js:209: CookiesTree.onTreeItemAdded =
function(id, args) {
I still fell like passing an args array is ugly.

[oshima, 2011-03-11 01:10:22]

still LGTM

http://codereview.chromium.org/6644002/diff/13001/chrome/browser/ui/webui/col...
File chrome/browser/ui/webui/collected_cookies_ui_delegate.cc (right):

http://codereview.chromium.org/6644002/diff/13001/chrome/browser/ui/webui/col...
chrome/browser/ui/webui/collected_cookies_ui_delegate.cc:170: return L"";
std::wstring()

http://codereview.chromium.org/6644002/diff/13001/chrome/browser/ui/webui/col...
chrome/browser/ui/webui/collected_cookies_ui_delegate.cc:189: return "";
std::string()

[Evan Stade, 2011-03-11 01:42:07]

http://codereview.chromium.org/6644002/diff/13001/chrome/browser/resources/co...
File chrome/browser/resources/cookies_tree.js (right):

http://codereview.chromium.org/6644002/diff/13001/chrome/browser/resources/co...
chrome/browser/resources/cookies_tree.js:210: $(id).addByParentId(args[0],
args[1], args[2]);
can't you do

$(id).addByParentId.apply($(id), args)

http://codereview.chromium.org/6644002/diff/13001/chrome/browser/resources/co...
chrome/browser/resources/cookies_tree.js:217: CookiesTree.setChildren =
function(id, args) {
you still have 2 js functions with the same name, which is confusing -- maybe
this can be setChildren and the one above can be doSetChildren

[xiyuan, 2011-03-11 16:46:03]

CL updated to address comments in patch set 6.

arv should be happy now. :)

http://codereview.chromium.org/6644002/diff/13001/chrome/browser/resources/co...
File chrome/browser/resources/cookies_tree.js (right):

http://codereview.chromium.org/6644002/diff/13001/chrome/browser/resources/co...
chrome/browser/resources/cookies_tree.js:209: CookiesTree.onTreeItemAdded =
function(id, args) {
On 2011/03/11 00:30:20, arv wrote:
> I still fell like passing an args array is ugly.

Done.

http://codereview.chromium.org/6644002/diff/13001/chrome/browser/resources/co...
chrome/browser/resources/cookies_tree.js:210: $(id).addByParentId(args[0],
args[1], args[2]);
On 2011/03/11 01:42:07, Evan Stade wrote:
> can't you do
> 
> $(id).addByParentId.apply($(id), args)

Followed arv's comments to replace args array with named params.

http://codereview.chromium.org/6644002/diff/13001/chrome/browser/resources/co...
chrome/browser/resources/cookies_tree.js:217: CookiesTree.setChildren =
function(id, args) {
On 2011/03/11 01:42:07, Evan Stade wrote:
> you still have 2 js functions with the same name, which is confusing -- maybe
> this can be setChildren and the one above can be doSetChildren

Done.

http://codereview.chromium.org/6644002/diff/13001/chrome/browser/ui/webui/col...
File chrome/browser/ui/webui/collected_cookies_ui_delegate.cc (right):

http://codereview.chromium.org/6644002/diff/13001/chrome/browser/ui/webui/col...
chrome/browser/ui/webui/collected_cookies_ui_delegate.cc:170: return L"";
On 2011/03/11 01:10:22, oshima wrote:
> std::wstring()

Done.

http://codereview.chromium.org/6644002/diff/13001/chrome/browser/ui/webui/col...
chrome/browser/ui/webui/collected_cookies_ui_delegate.cc:189: return "";
On 2011/03/11 01:10:22, oshima wrote:
> std::string()

Done.

[Evan Stade, 2011-03-11 18:09:44]

lgtm

http://codereview.chromium.org/6644002/diff/29001/chrome/browser/resources/co...
File chrome/browser/resources/collected_cookies.js (right):

http://codereview.chromium.org/6644002/diff/29001/chrome/browser/resources/co...
chrome/browser/resources/collected_cookies.js:67: document.body.oncontextmenu=
function() {
nit: space before =

http://codereview.chromium.org/6644002/diff/29001/chrome/browser/resources/co...
File chrome/browser/resources/cookies_tree.js (right):

http://codereview.chromium.org/6644002/diff/29001/chrome/browser/resources/co...
chrome/browser/resources/cookies_tree.js:206: $(treeId).setCallback(message);
same applies here, please use doSetCallback

[xiyuan, 2011-03-11 18:17:57]

All done. will submit after trybots greenness. Thanks, everyone.

http://codereview.chromium.org/6644002/diff/29001/chrome/browser/resources/co...
File chrome/browser/resources/collected_cookies.js (right):

http://codereview.chromium.org/6644002/diff/29001/chrome/browser/resources/co...
chrome/browser/resources/collected_cookies.js:67: document.body.oncontextmenu=
function() {
On 2011/03/11 18:09:44, Evan Stade wrote:
> nit: space before =

Done.

http://codereview.chromium.org/6644002/diff/29001/chrome/browser/resources/co...
File chrome/browser/resources/cookies_tree.js (right):

http://codereview.chromium.org/6644002/diff/29001/chrome/browser/resources/co...
chrome/browser/resources/cookies_tree.js:206: $(treeId).setCallback(message);
On 2011/03/11 18:09:44, Evan Stade wrote:
> same applies here, please use doSetCallback

Done.

[arv (Not doing code reviews), 2011-03-11 18:31:51]

LGTM

Thanks for getting rid of the args ;)

http://codereview.chromium.org/6644002/diff/29002/chrome/browser/resources/co...
File chrome/browser/resources/collected_cookies.js (right):

http://codereview.chromium.org/6644002/diff/29002/chrome/browser/resources/co...
chrome/browser/resources/collected_cookies.js:67: document.body.oncontextmenu =
function() {
document.oncontextmenu would also be correct. document is the root of the DOM
tree.

http://codereview.chromium.org/6644002/diff/29002/chrome/browser/resources/co...
chrome/browser/resources/collected_cookies.js:68: return false;
I prefer event.preventDefault() since it is more explicit.

[xiyuan, 2011-03-11 18:39:48]

http://codereview.chromium.org/6644002/diff/29002/chrome/browser/resources/co...
File chrome/browser/resources/collected_cookies.js (right):

http://codereview.chromium.org/6644002/diff/29002/chrome/browser/resources/co...
chrome/browser/resources/collected_cookies.js:67: document.body.oncontextmenu =
function() {
On 2011/03/11 18:31:51, arv wrote:
> document.oncontextmenu would also be correct. document is the root of the DOM
> tree.

Done.

http://codereview.chromium.org/6644002/diff/29002/chrome/browser/resources/co...
chrome/browser/resources/collected_cookies.js:68: return false;
On 2011/03/11 18:31:51, arv wrote:
> I prefer event.preventDefault() since it is more explicit.

Done.