Converting the Cookies options page from a TableView to a TreeView...

Converting the Cookies options page from a TableView to a TreeView
so that we can add in Database, LocalStorage, and Appcache next
to the cookies for an origin.

BUG=26713
TEST=cookies_tree_model_unittest.cc


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

[eroman, 2009-11-05 08:07:26]

Drive-by with some style nits.

(Didn't review thoroughly, but tried to give some advice on better matching the
style guide)

http://codereview.chromium.org/365005/diff/2011/2017
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/365005/diff/2011/2017#newcode13
Line 13: #include "app/resource_bundle.h"
order lexicographically.

http://codereview.chromium.org/365005/diff/2011/2017#newcode78
Line 78: // strip the trailing dot if it exists
nit: can the comment start with capital letter and end with period?

http://codereview.chromium.org/365005/diff/2011/2017#newcode83
Line 83: // First see if there is an existing match
end with period.

http://codereview.chromium.org/365005/diff/2011/2017#newcode106
Line 106: 
extra newlines.

http://codereview.chromium.org/365005/diff/2011/2018
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/365005/diff/2011/2018#newcode16
Line 16: class CookieTreeCookieNode;
order these alphabetically.

http://codereview.chromium.org/365005/diff/2011/2018#newcode31
Line 31: public:
you can omit as it is implied or a struct.

http://codereview.chromium.org/365005/diff/2011/2018#newcode32
Line 32: std::wstring origin_;
don't put trailing underscore on public members.

also, can't this be an ascii string?

http://codereview.chromium.org/365005/diff/2011/2018#newcode33
Line 33: enum NodeKind {
nit: I would suggest calling this "NodeType".

http://codereview.chromium.org/365005/diff/2011/2018#newcode34
Line 34: KIND_ROOT = 0,
unless you care about the numeric value, please leave the start value
unspecified.

http://codereview.chromium.org/365005/diff/2011/2018#newcode39
Line 39: const net::CookieMonster::CookieListPair* cookie_;
Within a struct, you should first put the type declarations, then the
constructors/destructors, then the method declarations, and finally the member
variables.

please re-order this.

http://codereview.chromium.org/365005/diff/2011/2018#newcode52
Line 52: // Get a pointer back to the associated model for the tree we are in.
nit: please use passive tense in method descriptions. "Get" --> "Gets".

http://codereview.chromium.org/365005/diff/2011/2018#newcode61
Line 61: };
can you add a DISALLOW_COPY_AND_ASSIGN ?

http://codereview.chromium.org/365005/diff/2011/2018#newcode71
Line 71: virtual void SetModel(CookiesTreeModel* model) { model_ = model; }
We generally try to label methods which are the implemention of an interface,
like:

// CookieTreeNode methods:

http://codereview.chromium.org/365005/diff/2011/2018#newcode105
Line 105: virtual ~CookieTreeCookiesNode() {}
ordering: dtor goes after ctor.

http://codereview.chromium.org/365005/diff/2011/2018#newcode139
Line 139: typedef net::CookieMonster::CookieList CookieList;
ordering: types should be listed at the top of this private section.

[ian fette, 2009-11-05 08:48:24]

Eric, thanks for the comments. Addressed all except those we discussed in
person, and the NodeKind->NodeType as described below.

http://codereview.chromium.org/365005/diff/2011/2017
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/365005/diff/2011/2017#newcode13
Line 13: #include "app/resource_bundle.h"
On 2009/11/05 08:07:26, eroman wrote:
> order lexicographically.

Done.

http://codereview.chromium.org/365005/diff/2011/2017#newcode78
Line 78: // strip the trailing dot if it exists
On 2009/11/05 08:07:26, eroman wrote:
> nit: can the comment start with capital letter and end with period?

Done.

http://codereview.chromium.org/365005/diff/2011/2017#newcode83
Line 83: // First see if there is an existing match
On 2009/11/05 08:07:26, eroman wrote:
> end with period.

Done.

http://codereview.chromium.org/365005/diff/2011/2017#newcode106
Line 106: 
On 2009/11/05 08:07:26, eroman wrote:
> extra newlines.

Done.

http://codereview.chromium.org/365005/diff/2011/2018
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/365005/diff/2011/2018#newcode16
Line 16: class CookieTreeCookieNode;
On 2009/11/05 08:07:26, eroman wrote:
> order these alphabetically.

Done.

http://codereview.chromium.org/365005/diff/2011/2018#newcode31
Line 31: public:
On 2009/11/05 08:07:26, eroman wrote:
> you can omit as it is implied or a struct.

Done.

http://codereview.chromium.org/365005/diff/2011/2018#newcode32
Line 32: std::wstring origin_;
On 2009/11/05 08:07:26, eroman wrote:
> don't put trailing underscore on public members.
> 
done
> also, can't this be an ascii string?
where it would be displayed and used requires a wstring, talked to ericroman and
am leaving this as-is

http://codereview.chromium.org/365005/diff/2011/2018#newcode33
Line 33: enum NodeKind {
On 2009/11/05 08:07:26, eroman wrote:
> nit: I would suggest calling this "NodeType".

I would too, except that NodeType is used already in tree_node_model.cc (it's
the template parameter), and so having two different NodeTypes would seem more
confusing than NodeKind. So, leaving this one as, while it's perhaps
sub-optimal, having a collision with the existing NodeType in code this uses
seems worse.

http://codereview.chromium.org/365005/diff/2011/2018#newcode34
Line 34: KIND_ROOT = 0,
On 2009/11/05 08:07:26, eroman wrote:
> unless you care about the numeric value, please leave the start value
> unspecified.

Done.

http://codereview.chromium.org/365005/diff/2011/2018#newcode39
Line 39: const net::CookieMonster::CookieListPair* cookie_;
On 2009/11/05 08:07:26, eroman wrote:
> Within a struct, you should first put the type declarations, then the
> constructors/destructors, then the method declarations, and finally the member
> variables.
> 
> please re-order this.

Done.

http://codereview.chromium.org/365005/diff/2011/2018#newcode52
Line 52: // Get a pointer back to the associated model for the tree we are in.
On 2009/11/05 08:07:26, eroman wrote:
> nit: please use passive tense in method descriptions. "Get" --> "Gets".

Done.

http://codereview.chromium.org/365005/diff/2011/2018#newcode61
Line 61: };
On 2009/11/05 08:07:26, eroman wrote:
> can you add a DISALLOW_COPY_AND_ASSIGN ?

Done.

http://codereview.chromium.org/365005/diff/2011/2018#newcode71
Line 71: virtual void SetModel(CookiesTreeModel* model) { model_ = model; }
On 2009/11/05 08:07:26, eroman wrote:
> We generally try to label methods which are the implemention of an interface,
> like:
> 
> // CookieTreeNode methods:

Done.

http://codereview.chromium.org/365005/diff/2011/2018#newcode105
Line 105: virtual ~CookieTreeCookiesNode() {}
On 2009/11/05 08:07:26, eroman wrote:
> ordering: dtor goes after ctor.

Done.

http://codereview.chromium.org/365005/diff/2011/2018#newcode139
Line 139: typedef net::CookieMonster::CookieList CookieList;
On 2009/11/05 08:07:26, eroman wrote:
> ordering: types should be listed at the top of this private section.

Done.

[Ben Goodger (Google), 2009-11-05 18:27:31]

+sky

Scott is more familiar with tree usage he can comment on your tree node model.

http://codereview.chromium.org/365005/diff/4018/4025
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/365005/diff/4018/4025#newcode32
Line 32: KIND_ROOT,
Some documentation as to what type of node these refer to would be helpful.

Also, we typically call these things Type rather than Kind.

http://codereview.chromium.org/365005/diff/4018/4025#newcode58
Line 58: virtual void SetModel(CookiesTreeModel* model);
Who calls this? Why does it need to be available on every node?

[sky, 2009-11-05 19:51:22]

I only looked at the tree_model classes. I think one node class would improve
readability, but Ian tells me he plans on adding different types.

http://codereview.chromium.org/365005/diff/4018/4024
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/365005/diff/4018/4024#newcode61
Line 61: class OriginNodeToStringEquality {
this should be in an anonymous namespace.

http://codereview.chromium.org/365005/diff/4018/4024#newcode65
Line 65: if (origin_.length() >= 1 && origin[0] == '.')
You also have this code where you create the class, do you really need it in
both places?

http://codereview.chromium.org/365005/diff/4018/4024#newcode85
Line 85: std::vector<CookieTreeNode*>::iterator OriginNodeIterator =
origin_node_iterator.

http://codereview.chromium.org/365005/diff/4018/4024#newcode86
Line 86: find_if(children().begin(),
As the children are ordered you should use lower_bound.

http://codereview.chromium.org/365005/diff/4018/4024#newcode119
Line 119: 
nit: nuke this line.

http://codereview.chromium.org/365005/diff/4018/4024#newcode130
Line 130: for (index = 0; index < GetChildCount(); ++index) {
Use lower_bound here too.

http://codereview.chromium.org/365005/diff/4018/4024#newcode136
Line 136: GetModel()->Add(this, index, new_child);
If you only do adds from the constructor, then you don't need to go through the
model.

http://codereview.chromium.org/365005/diff/4018/4024#newcode160
Line 160: }
nit: spacing is off.

http://codereview.chromium.org/365005/diff/4018/4024#newcode162
Line 162: // Returns the index of the icon to use for |node|. Return -1 to use
the
spacing is off.

http://codereview.chromium.org/365005/diff/4018/4024#newcode181
Line 181: // CookiesTreeModel, public:
You've got this header a couple of times.

http://codereview.chromium.org/365005/diff/4018/4025
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/365005/diff/4018/4025#newcode40
Line 40: : origin(origin), node_kind(node_kind), cookie(cookie) {}
You need to put each initializer on it's own line.

http://codereview.chromium.org/365005/diff/4018/4025#newcode42
Line 42: std::wstring origin;
Can there be methods for these on CookieTreeNode rather than a getter for the
DetailedInfo? If not, you should make the getter return a const&.

http://codereview.chromium.org/365005/diff/4018/4025#newcode50
Line 50: // Delete backend storage for this node, and any children nodes. (E.g.
delete
nit:newline between 49 and 50.

http://codereview.chromium.org/365005/diff/4018/4025#newcode64
Line 64: DISALLOW_COPY_AND_ASSIGN(CookieTreeNode);
DISALLOW_COPY_AND_ASSIGN should be in the private section.

http://codereview.chromium.org/365005/diff/4018/4025#newcode67
Line 67: 
nit: nuke this.

http://codereview.chromium.org/365005/diff/4018/4025#newcode102
Line 102: private:
nit: newline between 101 and 102.

http://codereview.chromium.org/365005/diff/4018/4025#newcode171
Line 171: private:
nit: newline between 170 and 171.

[sky, 2009-11-05 20:53:36]

http://codereview.chromium.org/365005/diff/3016/2051
File views/controls/tree/tree_view.cc (right):

http://codereview.chromium.org/365005/diff/3016/2051#newcode658
Line 658: // Need to re-size the provided icons to be the same size as
nit: resize

http://codereview.chromium.org/365005/diff/3016/2051#newcode659
Line 659: // IDR_FOLDER_CLOSED if they aren't already
nit: end with a period.

http://codereview.chromium.org/365005/diff/3016/2051#newcode666
Line 666: // draw our icons into this canvas
nit: draw -> Draw, end with a '.'.

http://codereview.chromium.org/365005/diff/3016/2051#newcode667
Line 667: canvas.DrawBitmapInt(model_images[i], 0, 0);
Don't you want to center?

http://codereview.chromium.org/365005/diff/3016/2051#newcode668
Line 668: HICON model_icon = IconUtil::CreateHICONFromSkBitmap(
Put model_icon outside the if statement so that you don't need to duplicate
AddIcon/DestroyIcon.

[mattm, 2009-11-05 21:10:58]

http://codereview.chromium.org/365005/diff/3016/2045
File chrome/browser/views/options/cookies_view.cc (left):

http://codereview.chromium.org/365005/diff/3016/2045#oldcode564
Line 564: UpdateForEmptyState();
This still needs to be called (in the new version the bottom part comes up blank
if you remove all and reopen the cookies window)

http://codereview.chromium.org/365005/diff/3016/2045
File chrome/browser/views/options/cookies_view.cc (right):

http://codereview.chromium.org/365005/diff/3016/2045#newcode33
Line 33: static const int kSearchFilterDelayMs = 500;
No longer used

http://codereview.chromium.org/365005/diff/3016/2045#newcode58
Line 58: SetRootShown(false);
Should do SetEditable(false) too.

[mattm, 2009-11-05 21:25:01]

http://codereview.chromium.org/365005/diff/3016/2045
File chrome/browser/views/options/cookies_view.cc (right):

http://codereview.chromium.org/365005/diff/3016/2045#newcode71
Line 71: delete cookies_model_->Remove(parent_node, ct_node_index);
It would be nice if these details were encapsulated in the CookiesTreeModel so
you could just do "if (selected_node)
cookies_model_->DeleteCookieNode(selected_node)" or something along those lines,
and each platform didn't need to copy this.

[ian fette, 2009-11-05 22:10:49]

I believe I've addressed all the issues.

http://codereview.chromium.org/365005/diff/4018/4024
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/365005/diff/4018/4024#newcode61
Line 61: class OriginNodeToStringEquality {
On 2009/11/05 19:51:22, sky wrote:
> this should be in an anonymous namespace.

Done.

http://codereview.chromium.org/365005/diff/4018/4024#newcode65
Line 65: if (origin_.length() >= 1 && origin[0] == '.')
On 2009/11/05 19:51:22, sky wrote:
> You also have this code where you create the class, do you really need it in
> both places?

Done.

http://codereview.chromium.org/365005/diff/4018/4024#newcode85
Line 85: std::vector<CookieTreeNode*>::iterator OriginNodeIterator =
On 2009/11/05 19:51:22, sky wrote:
> origin_node_iterator.

Done.

http://codereview.chromium.org/365005/diff/4018/4024#newcode86
Line 86: find_if(children().begin(),
On 2009/11/05 19:51:22, sky wrote:
> As the children are ordered you should use lower_bound.

Done.

http://codereview.chromium.org/365005/diff/4018/4024#newcode119
Line 119: 
On 2009/11/05 19:51:22, sky wrote:
> nit: nuke this line.

Done.

http://codereview.chromium.org/365005/diff/4018/4024#newcode130
Line 130: for (index = 0; index < GetChildCount(); ++index) {
On 2009/11/05 19:51:22, sky wrote:
> Use lower_bound here too.

Done.

http://codereview.chromium.org/365005/diff/4018/4024#newcode136
Line 136: GetModel()->Add(this, index, new_child);
On 2009/11/05 19:51:22, sky wrote:
> If you only do adds from the constructor, then you don't need to go through
the
> model.

I don't think that this is always the case.

http://codereview.chromium.org/365005/diff/4018/4024#newcode160
Line 160: }
On 2009/11/05 19:51:22, sky wrote:
> nit: spacing is off.

Done.

http://codereview.chromium.org/365005/diff/4018/4024#newcode162
Line 162: // Returns the index of the icon to use for |node|. Return -1 to use
the
On 2009/11/05 19:51:22, sky wrote:
> spacing is off.

Done.

http://codereview.chromium.org/365005/diff/4018/4024#newcode181
Line 181: // CookiesTreeModel, public:
On 2009/11/05 19:51:22, sky wrote:
> You've got this header a couple of times.

Done.

http://codereview.chromium.org/365005/diff/4018/4025
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/365005/diff/4018/4025#newcode32
Line 32: KIND_ROOT,
On 2009/11/05 18:27:31, Ben Goodger wrote:
> Some documentation as to what type of node these refer to would be helpful.
> 
> Also, we typically call these things Type rather than Kind.

Done.

http://codereview.chromium.org/365005/diff/4018/4025#newcode40
Line 40: : origin(origin), node_kind(node_kind), cookie(cookie) {}
On 2009/11/05 19:51:22, sky wrote:
> You need to put each initializer on it's own line.

Done.

http://codereview.chromium.org/365005/diff/4018/4025#newcode42
Line 42: std::wstring origin;
On 2009/11/05 19:51:22, sky wrote:
> Can there be methods for these on CookieTreeNode rather than a getter for the
> DetailedInfo? If not, you should make the getter return a const&.

Talked to Scott about this on IM. The query for detailed info should happen much
less frequently. Returning a const reference would mean I would have to hold
this struct around, increasing memory size, as opposed to just creating it on
demand, handing it off, and then letting it go out of scope. So, leaving for
now.

http://codereview.chromium.org/365005/diff/4018/4025#newcode50
Line 50: // Delete backend storage for this node, and any children nodes. (E.g.
delete
On 2009/11/05 19:51:22, sky wrote:
> nit:newline between 49 and 50.

Done.

http://codereview.chromium.org/365005/diff/4018/4025#newcode58
Line 58: virtual void SetModel(CookiesTreeModel* model);
On 2009/11/05 18:27:31, Ben Goodger wrote:
> Who calls this? Why does it need to be available on every node?

Basically, TreeNodeModel's constructor takes in the root node in the initializer
list. The root node wants a pointer back to the model. So, there's a catch-22,
one has to come first. I can't say TreeNodeModel(new CookieTreeRootNode(this))
because then I would be passing the this pointer in the initializer list, hence
adding the SetModel method. But, this method can be scoped down to the root
node, and not exposed on all.

http://codereview.chromium.org/365005/diff/4018/4025#newcode64
Line 64: DISALLOW_COPY_AND_ASSIGN(CookieTreeNode);
On 2009/11/05 19:51:22, sky wrote:
> DISALLOW_COPY_AND_ASSIGN should be in the private section.

Done.

http://codereview.chromium.org/365005/diff/4018/4025#newcode67
Line 67: 
On 2009/11/05 19:51:22, sky wrote:
> nit: nuke this.

Done.

http://codereview.chromium.org/365005/diff/4018/4025#newcode102
Line 102: private:
On 2009/11/05 19:51:22, sky wrote:
> nit: newline between 101 and 102.

Done.

http://codereview.chromium.org/365005/diff/4018/4025#newcode171
Line 171: private:
On 2009/11/05 19:51:22, sky wrote:
> nit: newline between 170 and 171.

Done.

http://codereview.chromium.org/365005/diff/3016/2045
File chrome/browser/views/options/cookies_view.cc (left):

http://codereview.chromium.org/365005/diff/3016/2045#oldcode564
Line 564: UpdateForEmptyState();
On 2009/11/05 21:10:58, mattm wrote:
> This still needs to be called (in the new version the bottom part comes up
blank
> if you remove all and reopen the cookies window)

Done.

http://codereview.chromium.org/365005/diff/3016/2045
File chrome/browser/views/options/cookies_view.cc (right):

http://codereview.chromium.org/365005/diff/3016/2045#newcode33
Line 33: static const int kSearchFilterDelayMs = 500;
On 2009/11/05 21:10:58, mattm wrote:
> No longer used

Done.

http://codereview.chromium.org/365005/diff/3016/2045#newcode58
Line 58: SetRootShown(false);
On 2009/11/05 21:10:58, mattm wrote:
> Should do SetEditable(false) too.

Done.

http://codereview.chromium.org/365005/diff/3016/2045#newcode71
Line 71: delete cookies_model_->Remove(parent_node, ct_node_index);
On 2009/11/05 21:25:01, mattm wrote:
> It would be nice if these details were encapsulated in the CookiesTreeModel so
> you could just do "if (selected_node)
> cookies_model_->DeleteCookieNode(selected_node)" or something along those
lines,
> and each platform didn't need to copy this.

Done.

http://codereview.chromium.org/365005/diff/3016/2051
File views/controls/tree/tree_view.cc (right):

http://codereview.chromium.org/365005/diff/3016/2051#newcode658
Line 658: // Need to re-size the provided icons to be the same size as
On 2009/11/05 20:53:37, sky wrote:
> nit: resize

Done.

http://codereview.chromium.org/365005/diff/3016/2051#newcode659
Line 659: // IDR_FOLDER_CLOSED if they aren't already
On 2009/11/05 20:53:37, sky wrote:
> nit: end with a period.

Done.

http://codereview.chromium.org/365005/diff/3016/2051#newcode666
Line 666: // draw our icons into this canvas
On 2009/11/05 20:53:37, sky wrote:
> nit: draw -> Draw, end with a '.'.

Done.

http://codereview.chromium.org/365005/diff/3016/2051#newcode667
Line 667: canvas.DrawBitmapInt(model_images[i], 0, 0);
On 2009/11/05 20:53:37, sky wrote:
> Don't you want to center?

Done.

http://codereview.chromium.org/365005/diff/3016/2051#newcode668
Line 668: HICON model_icon = IconUtil::CreateHICONFromSkBitmap(
On 2009/11/05 20:53:37, sky wrote:
> Put model_icon outside the if statement so that you don't need to duplicate
> AddIcon/DestroyIcon.

Done.

[sky, 2009-11-05 22:28:41]

http://codereview.chromium.org/365005/diff/12006/12012
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/365005/diff/12006/12012#newcode48
Line 48: : CookieTreeNode(UTF8ToWide(cookie->second.Name())), cookie_(cookie) {
cookie_ should be on the following line.

http://codereview.chromium.org/365005/diff/12006/12012#newcode135
Line 135: (*cookie_iterator)->GetParent()->IndexOfChild(*cookie_iterator),
Isn't (*cookie_iterator)->GetParent() always this?

http://codereview.chromium.org/365005/diff/12006/12012#newcode143
Line 143: : TreeNodeModel<CookieTreeNode>(new CookieTreeRootNode(NULL)),
It's safe to pass in 'this' as long you don't use it right away, which is the
case. Use the ALLOW_THIS_IN_INITIALIAZER_LIST.

http://codereview.chromium.org/365005/diff/12006/12012#newcode157
Line 157: IDR_DEFAULT_FAVICON));
nit: indent 2 more spaces.

http://codereview.chromium.org/365005/diff/12006/12012#newcode159
Line 159: IDR_COOKIE_ICON));
same here.

http://codereview.chromium.org/365005/diff/12006/12012#newcode227
Line 227: cookie_node->DeleteStoredObjects();
nit: these should be 2 space indented, not 4.

http://codereview.chromium.org/365005/diff/12006/12013
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/365005/diff/12006/12013#newcode42
Line 42: node_type(node_type),
nit: indent node_type and cookie by 2 more spaces.

http://codereview.chromium.org/365005/diff/12006/12013#newcode82
Line 82: return DetailedInfo(L"", DetailedInfo::TYPE_ROOT, NULL);
L"" -> std::wstring()

http://codereview.chromium.org/365005/diff/12006/12013#newcode84
Line 84: private:
nit: newline between 83 and 84.

http://codereview.chromium.org/365005/diff/12006/12013#newcode104
Line 104: private:
nit: newline between 103 and 104.

http://codereview.chromium.org/365005/diff/12006/12013#newcode139
Line 139: class CookieNodeComparitor {
comparitor -> comparator.

http://codereview.chromium.org/365005/diff/12006/12013#newcode139
Line 139: class CookieNodeComparitor {
Can this be private? Can it be in the .cc?

http://codereview.chromium.org/365005/diff/12006/12013#newcode162
Line 162: net::CookieMonster::CookieListPair* cookie_;
please document who owns this.

http://codereview.chromium.org/365005/diff/12006/12016
File views/controls/tree/tree_view.cc (right):

http://codereview.chromium.org/365005/diff/12006/12016#newcode671
Line 671: if (height > model_images[i].height())
Shouldn't you center all the time? Even if the height/width is bigger?

http://codereview.chromium.org/365005/diff/12006/12016#newcode672
Line 672: height_offset = (height - model_images[i].height()) / 2;
nit: spacing goes off here.

[ian fette, 2009-11-05 23:02:58]

Thanks. I believe I've addressed all of these.

http://codereview.chromium.org/365005/diff/12006/12012
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/365005/diff/12006/12012#newcode48
Line 48: : CookieTreeNode(UTF8ToWide(cookie->second.Name())), cookie_(cookie) {
On 2009/11/05 22:28:41, sky wrote:
> cookie_ should be on the following line.

Done.

http://codereview.chromium.org/365005/diff/12006/12012#newcode135
Line 135: (*cookie_iterator)->GetParent()->IndexOfChild(*cookie_iterator),
On 2009/11/05 22:28:41, sky wrote:
> Isn't (*cookie_iterator)->GetParent() always this?

Done.

http://codereview.chromium.org/365005/diff/12006/12012#newcode143
Line 143: : TreeNodeModel<CookieTreeNode>(new CookieTreeRootNode(NULL)),
On 2009/11/05 22:28:41, sky wrote:
> It's safe to pass in 'this' as long you don't use it right away, which is the
> case. Use the ALLOW_THIS_IN_INITIALIAZER_LIST.

Done.

http://codereview.chromium.org/365005/diff/12006/12012#newcode157
Line 157: IDR_DEFAULT_FAVICON));
On 2009/11/05 22:28:41, sky wrote:
> nit: indent 2 more spaces.

Done.

http://codereview.chromium.org/365005/diff/12006/12012#newcode159
Line 159: IDR_COOKIE_ICON));
On 2009/11/05 22:28:41, sky wrote:
> same here.

Done.

http://codereview.chromium.org/365005/diff/12006/12012#newcode227
Line 227: cookie_node->DeleteStoredObjects();
On 2009/11/05 22:28:41, sky wrote:
> nit: these should be 2 space indented, not 4.

Done.

http://codereview.chromium.org/365005/diff/12006/12013
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/365005/diff/12006/12013#newcode42
Line 42: node_type(node_type),
On 2009/11/05 22:28:41, sky wrote:
> nit: indent node_type and cookie by 2 more spaces.

Done.

http://codereview.chromium.org/365005/diff/12006/12013#newcode82
Line 82: return DetailedInfo(L"", DetailedInfo::TYPE_ROOT, NULL);
On 2009/11/05 22:28:41, sky wrote:
> L"" -> std::wstring()

Done.

http://codereview.chromium.org/365005/diff/12006/12013#newcode84
Line 84: private:
On 2009/11/05 22:28:41, sky wrote:
> nit: newline between 83 and 84.

Done.

http://codereview.chromium.org/365005/diff/12006/12013#newcode104
Line 104: private:
On 2009/11/05 22:28:41, sky wrote:
> nit: newline between 103 and 104.

Done.

http://codereview.chromium.org/365005/diff/12006/12013#newcode139
Line 139: class CookieNodeComparitor {
On 2009/11/05 22:28:41, sky wrote:
> comparitor -> comparator.

Done.

http://codereview.chromium.org/365005/diff/12006/12013#newcode139
Line 139: class CookieNodeComparitor {
On 2009/11/05 22:28:41, sky wrote:
> comparitor -> comparator.

Done.

http://codereview.chromium.org/365005/diff/12006/12013#newcode162
Line 162: net::CookieMonster::CookieListPair* cookie_;
On 2009/11/05 22:28:41, sky wrote:
> please document who owns this.

Done.

http://codereview.chromium.org/365005/diff/12006/12016
File views/controls/tree/tree_view.cc (right):

http://codereview.chromium.org/365005/diff/12006/12016#newcode671
Line 671: if (height > model_images[i].height())
On 2009/11/05 22:28:41, sky wrote:
> Shouldn't you center all the time? Even if the height/width is bigger?

Done.

http://codereview.chromium.org/365005/diff/12006/12016#newcode672
Line 672: height_offset = (height - model_images[i].height()) / 2;
On 2009/11/05 22:28:41, sky wrote:
> nit: spacing goes off here.

Done.

[sky, 2009-11-06 00:14:01]

http://codereview.chromium.org/365005/diff/11022/11028
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/365005/diff/11022/11028#newcode86
Line 86: if (origin_node_iterator != children().end())
lower_bound returns either the position the object should be at, or if not
present where it should be inserted.

http://codereview.chromium.org/365005/diff/11022/11028#newcode88
Line 88: // doesn't exist, create a new one and insert it into the (ordered)
children
nit: doesn't -> Doesn't, end with period.

http://codereview.chromium.org/365005/diff/11022/11032
File views/controls/tree/tree_view.cc (right):

http://codereview.chromium.org/365005/diff/11022/11032#newcode671
Line 671: height_offset = (height - model_images[i].height()) / 2;
nit: assign and declare at the same time, eg:

int height_offset = xxx

[ian fette, 2009-11-06 01:06:33]

Hopefully we're nearing the end? :)

http://codereview.chromium.org/365005/diff/11022/11028
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/365005/diff/11022/11028#newcode86
Line 86: if (origin_node_iterator != children().end())
On 2009/11/06 00:14:02, sky wrote:
> lower_bound returns either the position the object should be at, or if not
> present where it should be inserted.

Sorry, botched that. Fixed.

http://codereview.chromium.org/365005/diff/11022/11028#newcode88
Line 88: // doesn't exist, create a new one and insert it into the (ordered)
children
On 2009/11/06 00:14:02, sky wrote:
> nit: doesn't -> Doesn't, end with period.

Done.

http://codereview.chromium.org/365005/diff/11022/11032
File views/controls/tree/tree_view.cc (right):

http://codereview.chromium.org/365005/diff/11022/11032#newcode671
Line 671: height_offset = (height - model_images[i].height()) / 2;
On 2009/11/06 00:14:02, sky wrote:
> nit: assign and declare at the same time, eg:
> 
> int height_offset = xxx

Sorry, a left-over from when I was only going to do it if it was smaller :(.
Done.

[sky, 2009-11-06 04:16:26]

LGTM

http://codereview.chromium.org/365005/diff/17012/17018
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/365005/diff/17012/17018#newcode119
Line 119: // If there are no children, that's simple.
nit: it's not worth special casing empty.

[ian fette, 2009-11-06 04:26:14]

Scott, thanks for the review.

Ben, anything from your side, or good to check in?

http://codereview.chromium.org/365005/diff/17012/17018
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/365005/diff/17012/17018#newcode119
Line 119: // If there are no children, that's simple.
On 2009/11/06 04:16:26, sky wrote:
> nit: it's not worth special casing empty.

Done.

[Ben Goodger (Google), 2009-11-06 04:39:37]

http://codereview.chromium.org/365005/diff/17023/18007
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/365005/diff/17023/18007#newcode154
Line 154: 
delete NL

http://codereview.chromium.org/365005/diff/17023/18004
File chrome/browser/views/options/cookies_view.cc (right):

http://codereview.chromium.org/365005/diff/17023/18004#newcode287
Line 287: // cookies_table_->RemoveSelectedCookies();
hm?

[Ben Goodger (Google), 2009-11-06 04:44:45]

LGTM with these changes.

On 2009/11/06 04:39:37, Ben Goodger wrote:
> http://codereview.chromium.org/365005/diff/17023/18007
> File chrome/browser/cookies_tree_model.h (right):
> 
> http://codereview.chromium.org/365005/diff/17023/18007#newcode154
> Line 154: 
> delete NL
> 
> http://codereview.chromium.org/365005/diff/17023/18004
> File chrome/browser/views/options/cookies_view.cc (right):
> 
> http://codereview.chromium.org/365005/diff/17023/18004#newcode287
> Line 287: // cookies_table_->RemoveSelectedCookies();
> hm?

[ian fette, 2009-11-06 04:46:00]

Done

http://codereview.chromium.org/365005/diff/17023/18007
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/365005/diff/17023/18007#newcode154
Line 154: 
On 2009/11/06 04:39:37, Ben Goodger wrote:
> delete NL

Done.

http://codereview.chromium.org/365005/diff/17023/18004
File chrome/browser/views/options/cookies_view.cc (right):

http://codereview.chromium.org/365005/diff/17023/18004#newcode287
Line 287: // cookies_table_->RemoveSelectedCookies();
On 2009/11/06 04:39:37, Ben Goodger wrote:
> hm?

Done.

[Evan Martin, 2010-01-06 17:41:34]

BTW, this change added a new string but didn't remove the old one.  In the
future it would be good to do both, because otherwise we end up with lots of
mystery strings.

[ian fette, 2010-01-06 17:51:34]

Apologies. The first change was windows only, the Linux change came after
thanks to Matt. I should have followed up with a CL to remove the old unused
strings afterwards, happy to do so now.

-Ian

2010/1/6 <evan@chromium.org>

> BTW, this change added a new string but didn't remove the old one.  In the
> future it would be good to do both, because otherwise we end up with lots
> of
> mystery strings.
>
> http://codereview.chromium.org/365005
>

[Evan Martin, 2010-01-06 17:53:30]

Pink and I are doing it.

On Wed, Jan 6, 2010 at 9:51 AM, Ian Fette <ian@chromium.org> wrote:
> Apologies. The first change was windows only, the Linux change came after
> thanks to Matt. I should have followed up with a CL to remove the old unused
> strings afterwards, happy to do so now.
> -Ian
>
> 2010/1/6 <evan@chromium.org>
>>
>> BTW, this change added a new string but didn't remove the old one.  In the
>> future it would be good to do both, because otherwise we end up with lots
>> of
>> mystery strings.
>>
>> http://codereview.chromium.org/365005
>
>

[ian fette, 2010-01-06 18:33:15]

Ok, thanks Evan. Note that a few new strings were added that are not yet
used -- please don't remove these new ones, that's still a work in progress
(for appcache, db, ...)

-Ian



2010/1/6 Evan Martin <evan@chromium.org>

> Pink and I are doing it.
>
> On Wed, Jan 6, 2010 at 9:51 AM, Ian Fette <ian@chromium.org> wrote:
> > Apologies. The first change was windows only, the Linux change came after
> > thanks to Matt. I should have followed up with a CL to remove the old
> unused
> > strings afterwards, happy to do so now.
> > -Ian
> >
> > 2010/1/6 <evan@chromium.org>
> >>
> >> BTW, this change added a new string but didn't remove the old one.  In
> the
> >> future it would be good to do both, because otherwise we end up with
> lots
> >> of
> >> mystery strings.
> >>
> >> http://codereview.chromium.org/365005
> >
> >
>