[Mac] Implement the cookie manager

[Mac] Implement the cookie manager

This CL creates the basic cookie management interface, allowing users to view
and delete cookies from Chromium. TODO: (1) Add the filtering by domain
capability. (2) Localize the NIB.

BUG=15360
TEST=Chromium-->Preferences-->Under the Hood-->Show Cookies. Also covered by unit tests.

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

[Robert Sesek, 2009-12-30 16:58:52]

John, could you please review this? Pinkerton is still out and I'd rather not
let this sit. Thanks!

Screen shot: http://www.bluestatic.org/temp/chromium-cookies.png

[John Grabowski, 2009-12-30 20:05:49]

Big picture I like what you've done.

I've asked for some structural changes (e.g. use of classes instead of dicts). 
If you strongly disagree we can talk more.

http://codereview.chromium.org/523025/diff/1/3
File chrome/app/nibs/Cookies.xib (right):

http://codereview.chromium.org/523025/diff/1/3#newcode1
chrome/app/nibs/Cookies.xib:1: <?xml version="1.0" encoding="UTF-8"?>
Needs to be localized.
See
http://www.chromium.org/developers/design-documents/ui-localization/mac-notes.
Look at some simple xibs (e.g. About.xib) for an example.

Also, see the latest dev channel on Windows.  The cookie manager should probably
also have a search box (or a clear reason why not).

Would be nice if 'Remove''s Enabled was Cocoa-bound to the presence/absence of a
cookie selection.

http://codereview.chromium.org/523025/diff/1/5
File chrome/browser/cocoa/cookies_window_controller.h (right):

http://codereview.chromium.org/523025/diff/1/5#newcode33
chrome/browser/cocoa/cookies_window_controller.h:33: typedef
net::CookieMonster::CookieListPair CookieListPair;
We try and use namespaces to prevent collision; this typedef goes in the other
direction.

But that probably doesn't matter since you never use this new type anywhere;
minus well delete it.

http://codereview.chromium.org/523025/diff/1/5#newcode63
chrome/browser/cocoa/cookies_window_controller.h:63: friend class
CookiesWindowControllerTest;
You make CookiesWindowControllerTest a friend giving it access to
CookiesTreeModelObserverCocoa's protected area.  But
CookiesTreeModelObserverCocoa has nothing protected.  So this 'friend'
declaration does nothing.  Remove?

http://codereview.chromium.org/523025/diff/1/5#newcode67
chrome/browser/cocoa/cookies_window_controller.h:67: NSMutableDictionary*
CreateCocoaTreeNode(TreeModelNode* node, bool recurse);
Cocoa convention:
[blah alloc] initWithBlah:]: retained
[Blah blahWithFoo]: autoreleased

Following the covention you might want to name CreateCocoaTreeNode() something
like CocoaTreeNodeWithBlah() or so it's clear(er) on 1st glance the returned
object is autoreleased.

http://codereview.chromium.org/523025/diff/1/5#newcode72
chrome/browser/cocoa/cookies_window_controller.h:72: NSMutableDictionary*
FindCocoaNode(TreeModelNode* node,
Not sure why you used an NSDictionary.    Yes, an NSDictionary can represent
most any possible structure, with strings as keys for field names, but it's
usually not ideal.  Why not a real tree (e.g. NSTreeNode)?

http://codereview.chromium.org/523025/diff/1/6
File chrome/browser/cocoa/cookies_window_controller.mm (right):

http://codereview.chromium.org/523025/diff/1/6#newcode21
chrome/browser/cocoa/cookies_window_controller.mm:21: NSString* const
kCocoaCookieTreeNodeTitle = @"title";
Would prefer actual class/structure to NSDictionary (see header comment).

http://codereview.chromium.org/523025/diff/1/6#newcode36
chrome/browser/cocoa/cookies_window_controller.mm:36: - (void)modelChanged;
Use of Cocoa bindings would eliminate the need for this call.

http://codereview.chromium.org/523025/diff/1/6#newcode53
chrome/browser/cocoa/cookies_window_controller.mm:53: [icons_
addObject:rb.GetNSImageNamed(IDR_BOOKMARK_BAR_FOLDER)];
This is probably correct but the name seems awkward.  What constant is used on
Windows or Linux?

http://codereview.chromium.org/523025/diff/1/6#newcode56
chrome/browser/cocoa/cookies_window_controller.mm:56:
CookiesTreeModelObserverCocoa::~CookiesTreeModelObserverCocoa() {
Should be no need to declare this (in src or header) if empty.

http://codereview.chromium.org/523025/diff/1/6#newcode254
chrome/browser/cocoa/cookies_window_controller.mm:254: [[self window]
setDelegate:self];
Can set the window's delegate to this controller ("file's owner") in the nib.

http://codereview.chromium.org/523025/diff/1/7
File chrome/browser/cocoa/cookies_window_controller_unittest.mm (right):

http://codereview.chromium.org/523025/diff/1/7#newcode2
chrome/browser/cocoa/cookies_window_controller_unittest.mm:2: // Use of this
source code is governed by a BSD-style license that can be
Really glad to see you went all-out here.
I will review this file in more detail once other issues addressed.

[Robert Sesek, 2009-12-31 07:55:14]

http://codereview.chromium.org/523025/diff/1/3
File chrome/app/nibs/Cookies.xib (right):

http://codereview.chromium.org/523025/diff/1/3#newcode1
chrome/app/nibs/Cookies.xib:1: <?xml version="1.0" encoding="UTF-8"?>
On 2009/12/30 20:05:49, John Grabowski wrote:
> Needs to be localized.
> See
> http://www.chromium.org/developers/design-documents/ui-localization/mac-notes.
> Look at some simple xibs (e.g. About.xib) for an example.
> 
> Also, see the latest dev channel on Windows.  The cookie manager should
probably
> also have a search box (or a clear reason why not).
> 
> Would be nice if 'Remove''s Enabled was Cocoa-bound to the presence/absence of
a
> cookie selection.

Search will come in a future CL (see description). I was going to also localize
and send a separate review to TVL, though I can do it in this CL if you deem it
necessary. The Remove button is now enabled/disabled as you suggested.

http://codereview.chromium.org/523025/diff/1/5
File chrome/browser/cocoa/cookies_window_controller.h (right):

http://codereview.chromium.org/523025/diff/1/5#newcode33
chrome/browser/cocoa/cookies_window_controller.h:33: typedef
net::CookieMonster::CookieListPair CookieListPair;
On 2009/12/30 20:05:49, John Grabowski wrote:
> We try and use namespaces to prevent collision; this typedef goes in the other
> direction.
> 
> But that probably doesn't matter since you never use this new type anywhere;
> minus well delete it.

Vestigial -> removed.

http://codereview.chromium.org/523025/diff/1/5#newcode63
chrome/browser/cocoa/cookies_window_controller.h:63: friend class
CookiesWindowControllerTest;
On 2009/12/30 20:05:49, John Grabowski wrote:
> You make CookiesWindowControllerTest a friend giving it access to
> CookiesTreeModelObserverCocoa's protected area.  But
> CookiesTreeModelObserverCocoa has nothing protected.  So this 'friend'
> declaration does nothing.  Remove?

It's friended in order to test the two private functions below.

http://codereview.chromium.org/523025/diff/1/5#newcode67
chrome/browser/cocoa/cookies_window_controller.h:67: NSMutableDictionary*
CreateCocoaTreeNode(TreeModelNode* node, bool recurse);
On 2009/12/30 20:05:49, John Grabowski wrote:
> Cocoa convention:
> [blah alloc] initWithBlah:]: retained
> [Blah blahWithFoo]: autoreleased
> 
> Following the covention you might want to name CreateCocoaTreeNode() something
> like CocoaTreeNodeWithBlah() or so it's clear(er) on 1st glance the returned
> object is autoreleased.

Good idea. Renamed to CocoaNodeFromTreeNode().

http://codereview.chromium.org/523025/diff/1/5#newcode72
chrome/browser/cocoa/cookies_window_controller.h:72: NSMutableDictionary*
FindCocoaNode(TreeModelNode* node,
On 2009/12/30 20:05:49, John Grabowski wrote:
> Not sure why you used an NSDictionary.    Yes, an NSDictionary can represent
> most any possible structure, with strings as keys for field names, but it's
> usually not ideal.  Why not a real tree (e.g. NSTreeNode)?
> 
> 

NSTreeNode is not for model objects -- you initialize it with one as the
"represented object". That said, I've created CocoaCookieTreeNode (say that 3
times fast!) to replace the dictionaries.

http://codereview.chromium.org/523025/diff/1/6
File chrome/browser/cocoa/cookies_window_controller.mm (right):

http://codereview.chromium.org/523025/diff/1/6#newcode36
chrome/browser/cocoa/cookies_window_controller.mm:36: - (void)modelChanged;
On 2009/12/30 20:05:49, John Grabowski wrote:
> Use of Cocoa bindings would eliminate the need for this call.

Switched to using KVO.

http://codereview.chromium.org/523025/diff/1/6#newcode53
chrome/browser/cocoa/cookies_window_controller.mm:53: [icons_
addObject:rb.GetNSImageNamed(IDR_BOOKMARK_BAR_FOLDER)];
On 2009/12/30 20:05:49, John Grabowski wrote:
> This is probably correct but the name seems awkward.  What constant is used on
> Windows or Linux?

On Windows and Linux, this is provided by the system. Should I switch this to
[NSImage imageNamed:NSImageNameFolder]?

http://codereview.chromium.org/523025/diff/1/6#newcode56
chrome/browser/cocoa/cookies_window_controller.mm:56:
CookiesTreeModelObserverCocoa::~CookiesTreeModelObserverCocoa() {
On 2009/12/30 20:05:49, John Grabowski wrote:
> Should be no need to declare this (in src or header) if empty.

Done.

http://codereview.chromium.org/523025/diff/1/6#newcode254
chrome/browser/cocoa/cookies_window_controller.mm:254: [[self window]
setDelegate:self];
On 2009/12/30 20:05:49, John Grabowski wrote:
> Can set the window's delegate to this controller ("file's owner") in the nib.

Done.

[John Grabowski, 2010-01-04 03:02:54]

Overall great work.

LGTM if 
- you add a simple unit test for CookieTreeNode
- you do the 'step into' test and see where we end up (re NSArray unexpected in
children thing)

http://codereview.chromium.org/523025/diff/1/3
File chrome/app/nibs/Cookies.xib (right):

http://codereview.chromium.org/523025/diff/1/3#newcode1
chrome/app/nibs/Cookies.xib:1: <?xml version="1.0" encoding="UTF-8"?>
On 2009/12/31 07:55:14, rsesek wrote:
> On 2009/12/30 20:05:49, John Grabowski wrote:
> > Needs to be localized.
> > See
> >
http://www.chromium.org/developers/design-documents/ui-localization/mac-notes.
> > Look at some simple xibs (e.g. About.xib) for an example.
> > 
> > Also, see the latest dev channel on Windows.  The cookie manager should
> probably
> > also have a search box (or a clear reason why not).
> > 
> > Would be nice if 'Remove''s Enabled was Cocoa-bound to the presence/absence
of
> a
> > cookie selection.
> 
> Search will come in a future CL (see description). I was going to also
localize
> and send a separate review to TVL, though I can do it in this CL if you deem
it
> necessary. The Remove button is now enabled/disabled as you suggested.

Happy to have search in a future CL; didn't catch that right away.

Happy to have l10n in a future CL if you add a TODO somewhere (I know you're
good for it :-)

http://codereview.chromium.org/523025/diff/1/6
File chrome/browser/cocoa/cookies_window_controller.mm (right):

http://codereview.chromium.org/523025/diff/1/6#newcode53
chrome/browser/cocoa/cookies_window_controller.mm:53: [icons_
addObject:rb.GetNSImageNamed(IDR_BOOKMARK_BAR_FOLDER)];
On 2009/12/31 07:55:14, rsesek wrote:
> On 2009/12/30 20:05:49, John Grabowski wrote:
> > This is probably correct but the name seems awkward.  What constant is used
on
> > Windows or Linux?
> 
> On Windows and Linux, this is provided by the system. Should I switch this to
> [NSImage imageNamed:NSImageNameFolder]?

GetNSImageNamed is probably fine.  The best thing is to use a proper name.  I'd
be happy with a TODO(rsesek): rename the IDR_BOOKMARK_BAR_FOLDER resource to
something more generic now that we're using it in multiple places.

http://codereview.chromium.org/523025/diff/2004/27
File chrome/browser/cocoa/cookie_tree_node.h (right):

http://codereview.chromium.org/523025/diff/2004/27#newcode15
chrome/browser/cocoa/cookie_tree_node.h:15: CookieTreeNode* treeNode_;  // weak
More comments would be nice here (e.g. "the non-Cocoa version of this node").

http://codereview.chromium.org/523025/diff/2004/28
File chrome/browser/cocoa/cookie_tree_node.mm (right):

http://codereview.chromium.org/523025/diff/2004/28#newcode14
chrome/browser/cocoa/cookie_tree_node.mm:14: -
(id)initWithNode:(CookieTreeNode*)node createChildren:(BOOL)recurse {
I don't mean to be excessive but we have a unit test for all files in
browser/cocoa/*.mm.  Would hate to break that now.  Even something simple, like
creating one node with recursion, would be fine.

http://codereview.chromium.org/523025/diff/2004/28#newcode19
chrome/browser/cocoa/cookie_tree_node.mm:19: children_.reset([[NSMutableArray
alloc] initWithCapacity:childCount]);
Perhaps can put alloc of this array inside the "if (recurse) { }"

http://codereview.chromium.org/523025/diff/2004/28#newcode44
chrome/browser/cocoa/cookie_tree_node.mm:44:
name_.reset([base::SysUTF8ToNSString(cookie.Name()) retain]);
If name and title are always the same do we need both variables?

http://codereview.chromium.org/523025/diff/2004/30
File chrome/browser/cocoa/cookies_window_controller.mm (right):

http://codereview.chromium.org/523025/diff/2004/30#newcode129
chrome/browser/cocoa/cookies_window_controller.mm:129: @synthesize
treeController = treeController_;
blank line between @impl and @synth

http://codereview.chromium.org/523025/diff/2004/31
File chrome/browser/cocoa/cookies_window_controller_unittest.mm (right):

http://codereview.chromium.org/523025/diff/2004/31#newcode80
chrome/browser/cocoa/cookies_window_controller_unittest.mm:80: // For some
utterly inexplicable reason, |-children| returns an NSArray rather
There are subtle ways to make this happen (e.g. [[NSMutableArray array] copy]
returns an NSArray).

Can you set a breakpoint on a call to -children, then "step into" in a debugger
and see where you end up?  If that doesn't work, try setting a breakpoint right
before the call to children, then type "break [NSArray init]" in the debugger
console (or something like that) before continuing so we get more info.

[pink (ping after 24hrs), 2010-01-04 14:21:46]

Do you still want me to look at this CL?

-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[John Grabowski, 2010-01-04 14:42:49]

I think you are asking rsesek and not me, but... if you have time, then
definitely provide feedback.  There is a reason you are on the cc list, yes?

jrg


On Mon, Jan 4, 2010 at 6:21 AM, Mike Pinkerton <pinkerton@chromium.org>wrote:

> Do you still want me to look at this CL?
>
> --
> Mike Pinkerton
> Mac Weenie
> pinkerton@google.com
>

[pink (ping after 24hrs), 2010-01-04 15:46:51]

lots of little things.

Also, can we use a small/narrow scrollbar in the tree, or is that something we
want to avoid? Camino uses the narrow one, Safari the regular one. *shrug*

http://codereview.chromium.org/523025/diff/2004/27
File chrome/browser/cocoa/cookie_tree_node.h (right):

http://codereview.chromium.org/523025/diff/2004/27#newcode17
chrome/browser/cocoa/cookie_tree_node.h:17: // These members are only set for
true cookie nodes.
What other things could be there? folders? anything else?

http://codereview.chromium.org/523025/diff/2004/27#newcode30
chrome/browser/cocoa/cookie_tree_node.h:30: -
(id)initWithNode:(CookieTreeNode*)node createChildren:(BOOL)recurse;
what does createChildren do? Why do you need a choice?

http://codereview.chromium.org/523025/diff/2004/27#newcode39
chrome/browser/cocoa/cookie_tree_node.h:39: - (NSMutableArray*)children;
As we don't normally allow mutable accessors, you should probably comment on why
this is this way.

http://codereview.chromium.org/523025/diff/2004/27#newcode42
chrome/browser/cocoa/cookie_tree_node.h:42: // Used only by cookies.
What does it return for other things? nil? empty string?

http://codereview.chromium.org/523025/diff/2004/28
File chrome/browser/cocoa/cookie_tree_node.mm (right):

http://codereview.chromium.org/523025/diff/2004/28#newcode16
chrome/browser/cocoa/cookie_tree_node.mm:16: treeNode_ = node;
DCHECK(node)?

http://codereview.chromium.org/523025/diff/2004/28#newcode62
chrome/browser/cocoa/cookie_tree_node.mm:62:
sendFor_.reset([l10n_util::GetNSString(
should these be GetNSStringWithFixup()?

http://codereview.chromium.org/523025/diff/2004/29
File chrome/browser/cocoa/cookies_window_controller.h (right):

http://codereview.chromium.org/523025/diff/2004/29#newcode21
chrome/browser/cocoa/cookies_window_controller.h:21: class
CookiesTreeModelObserverCocoa : public TreeModelObserver {
CookiesTreeModelObserverBridge? Does this need to be part of the public api or
can it live in the .mm file? Or is it here for testing?

http://codereview.chromium.org/523025/diff/2004/29#newcode70
chrome/browser/cocoa/cookies_window_controller.h:70: // Platform-independent
model and C++/Obj-C bridge components.
@private

http://codereview.chromium.org/523025/diff/2004/29#newcode84
chrome/browser/cocoa/cookies_window_controller.h:84: @property (readonly)
NSTreeController* treeController;
nonatomic?

http://codereview.chromium.org/523025/diff/2004/30
File chrome/browser/cocoa/cookies_window_controller.mm (right):

http://codereview.chromium.org/523025/diff/2004/30#newcode26
chrome/browser/cocoa/cookies_window_controller.mm:26: :
windowController_(controller) {
follow c++ naming in c++ classes: window_controller_

http://codereview.chromium.org/523025/diff/2004/30#newcode40
chrome/browser/cocoa/cookies_window_controller.mm:40: CookieTreeNode*
cookie_chid = cookie_parent->GetChild(start + i);
cookie_child?

http://codereview.chromium.org/523025/diff/2004/30#newcode95
chrome/browser/cocoa/cookies_window_controller.mm:95: CocoaCookieTreeNode*
CookiesTreeModelObserverCocoa::CocoaNodeFromTreeNode(
naming doesn't immediately imply that this has been retained. Either clarify the
naming or autorelease before returning.

http://codereview.chromium.org/523025/diff/2004/30#newcode112
chrome/browser/cocoa/cookies_window_controller.mm:112: NSMutableArray* children
= [start children];
this local var can be NSArray, right? No need to make it mutable if it doesn't
have to be.

http://codereview.chromium.org/523025/diff/2004/30#newcode123
chrome/browser/cocoa/cookies_window_controller.mm:123: return nil;  // We
couldn't find the node?
why is that a question?

http://codereview.chromium.org/523025/diff/2004/30#newcode187
chrome/browser/cocoa/cookies_window_controller.mm:187: NSArray* selection =
[[treeController_ selectedObjects] retain];
why do you need to retain it, and why not use a scoped_nsobject?

[Robert Sesek, 2010-01-04 21:47:28]

All comments addressed. I tried making the scroll bar small, but IB seems to
hate me and won't take it.

http://codereview.chromium.org/523025/diff/2004/27
File chrome/browser/cocoa/cookie_tree_node.h (right):

http://codereview.chromium.org/523025/diff/2004/27#newcode15
chrome/browser/cocoa/cookie_tree_node.h:15: CookieTreeNode* treeNode_;  // weak
On 2010/01/04 03:02:54, John Grabowski wrote:
> More comments would be nice here (e.g. "the non-Cocoa version of this node"). 

Done.

http://codereview.chromium.org/523025/diff/2004/27#newcode17
chrome/browser/cocoa/cookie_tree_node.h:17: // These members are only set for
true cookie nodes.
On 2010/01/04 15:46:52, pink wrote:
> What other things could be there? folders? anything else?

There's the domain-level nodes (top level) and folders (Cookies).

http://codereview.chromium.org/523025/diff/2004/27#newcode30
chrome/browser/cocoa/cookie_tree_node.h:30: -
(id)initWithNode:(CookieTreeNode*)node createChildren:(BOOL)recurse;
On 2010/01/04 15:46:52, pink wrote:
> what does createChildren do? Why do you need a choice?

It was used before I introduced |-rebuild|. Removed.

http://codereview.chromium.org/523025/diff/2004/27#newcode39
chrome/browser/cocoa/cookie_tree_node.h:39: - (NSMutableArray*)children;
On 2010/01/04 15:46:52, pink wrote:
> As we don't normally allow mutable accessors, you should probably comment on
why
> this is this way.

Done.

http://codereview.chromium.org/523025/diff/2004/27#newcode42
chrome/browser/cocoa/cookie_tree_node.h:42: // Used only by cookies.
On 2010/01/04 15:46:52, pink wrote:
> What does it return for other things? nil? empty string?

Nil.

http://codereview.chromium.org/523025/diff/2004/28
File chrome/browser/cocoa/cookie_tree_node.mm (right):

http://codereview.chromium.org/523025/diff/2004/28#newcode14
chrome/browser/cocoa/cookie_tree_node.mm:14: -
(id)initWithNode:(CookieTreeNode*)node createChildren:(BOOL)recurse {
On 2010/01/04 03:02:54, John Grabowski wrote:
> I don't mean to be excessive but we have a unit test for all files in
> browser/cocoa/*.mm.  Would hate to break that now.  Even something simple,
like
> creating one node with recursion, would be fine.

The unit tests are in cookies_window_controller_unittest.mm. There's a lot of
dummy test classes used to setup the CookieMonster in a testing profile, so
rather than duplicating that already-duplicated code, I kept the tests where
they were. I can add a test file for CocoaCookieTreeNode and add a comment
saying it's tested elsewhere, if you want.

http://codereview.chromium.org/523025/diff/2004/28#newcode16
chrome/browser/cocoa/cookie_tree_node.mm:16: treeNode_ = node;
On 2010/01/04 15:46:52, pink wrote:
> DCHECK(node)?

Done.

http://codereview.chromium.org/523025/diff/2004/28#newcode19
chrome/browser/cocoa/cookie_tree_node.mm:19: children_.reset([[NSMutableArray
alloc] initWithCapacity:childCount]);
On 2010/01/04 03:02:54, John Grabowski wrote:
> Perhaps can put alloc of this array inside the "if (recurse) { }" 

I want |-children| to always return a valid array so we can attach new children
after the node is created.

http://codereview.chromium.org/523025/diff/2004/28#newcode44
chrome/browser/cocoa/cookie_tree_node.mm:44:
name_.reset([base::SysUTF8ToNSString(cookie.Name()) retain]);
On 2010/01/04 03:02:54, John Grabowski wrote:
> If name and title are always the same do we need both variables?

They're not always the same. Only for Cookie nodes. |name| is bound in the
detail interface, and we don't want it changing when we select a folder node.
|title| is displayed in the list.

http://codereview.chromium.org/523025/diff/2004/28#newcode62
chrome/browser/cocoa/cookie_tree_node.mm:62:
sendFor_.reset([l10n_util::GetNSString(
On 2010/01/04 15:46:52, pink wrote:
> should these be GetNSStringWithFixup()?

Done.

http://codereview.chromium.org/523025/diff/2004/29
File chrome/browser/cocoa/cookies_window_controller.h (right):

http://codereview.chromium.org/523025/diff/2004/29#newcode21
chrome/browser/cocoa/cookies_window_controller.h:21: class
CookiesTreeModelObserverCocoa : public TreeModelObserver {
On 2010/01/04 15:46:52, pink wrote:
> CookiesTreeModelObserverBridge? Does this need to be part of the public api or
> can it live in the .mm file? Or is it here for testing?

Changed to Bridge. And it is exposed here for testing, yes.

http://codereview.chromium.org/523025/diff/2004/29#newcode70
chrome/browser/cocoa/cookies_window_controller.h:70: // Platform-independent
model and C++/Obj-C bridge components.
On 2010/01/04 15:46:52, pink wrote:
> @private

Done.

http://codereview.chromium.org/523025/diff/2004/29#newcode84
chrome/browser/cocoa/cookies_window_controller.h:84: @property (readonly)
NSTreeController* treeController;
On 2010/01/04 15:46:52, pink wrote:
> nonatomic?

Done.

http://codereview.chromium.org/523025/diff/2004/30
File chrome/browser/cocoa/cookies_window_controller.mm (right):

http://codereview.chromium.org/523025/diff/2004/30#newcode26
chrome/browser/cocoa/cookies_window_controller.mm:26: :
windowController_(controller) {
On 2010/01/04 15:46:52, pink wrote:
> follow c++ naming in c++ classes: window_controller_

Done.

http://codereview.chromium.org/523025/diff/2004/30#newcode40
chrome/browser/cocoa/cookies_window_controller.mm:40: CookieTreeNode*
cookie_chid = cookie_parent->GetChild(start + i);
On 2010/01/04 15:46:52, pink wrote:
> cookie_child?

Done.

http://codereview.chromium.org/523025/diff/2004/30#newcode95
chrome/browser/cocoa/cookies_window_controller.mm:95: CocoaCookieTreeNode*
CookiesTreeModelObserverCocoa::CocoaNodeFromTreeNode(
On 2010/01/04 15:46:52, pink wrote:
> naming doesn't immediately imply that this has been retained. Either clarify
the
> naming or autorelease before returning.

It's supposed to be |-autorelease|ed. I was tracking down an |-autorelease| bug
and I forgot to change this back.

http://codereview.chromium.org/523025/diff/2004/30#newcode112
chrome/browser/cocoa/cookies_window_controller.mm:112: NSMutableArray* children
= [start children];
On 2010/01/04 15:46:52, pink wrote:
> this local var can be NSArray, right? No need to make it mutable if it doesn't
> have to be.

Done.

http://codereview.chromium.org/523025/diff/2004/30#newcode123
chrome/browser/cocoa/cookies_window_controller.mm:123: return nil;  // We
couldn't find the node?
On 2010/01/04 15:46:52, pink wrote:
> why is that a question?

Because? ;)

http://codereview.chromium.org/523025/diff/2004/30#newcode129
chrome/browser/cocoa/cookies_window_controller.mm:129: @synthesize
treeController = treeController_;
On 2010/01/04 03:02:54, John Grabowski wrote:
> blank line between @impl and @synth
> 

Done.

http://codereview.chromium.org/523025/diff/2004/30#newcode187
chrome/browser/cocoa/cookies_window_controller.mm:187: NSArray* selection =
[[treeController_ selectedObjects] retain];
On 2010/01/04 15:46:52, pink wrote:
> why do you need to retain it, and why not use a scoped_nsobject?

When it wasn't retained, |selection| was being invalidated immediately. I'm not
sure why that was happening, but the fix was retaining it. Changed to a scoper,
though.

http://codereview.chromium.org/523025/diff/2004/31
File chrome/browser/cocoa/cookies_window_controller_unittest.mm (right):

http://codereview.chromium.org/523025/diff/2004/31#newcode80
chrome/browser/cocoa/cookies_window_controller_unittest.mm:80: // For some
utterly inexplicable reason, |-children| returns an NSArray rather
On 2010/01/04 03:02:54, John Grabowski wrote:
> There are subtle ways to make this happen (e.g. [[NSMutableArray array] copy]
> returns an NSArray).
> 
> Can you set a breakpoint on a call to -children, then "step into" in a
debugger
> and see where you end up?  If that doesn't work, try setting a breakpoint
right
> before the call to children, then type "break [NSArray init]" in the debugger
> console (or something like that) before continuing so we get more info.

|-children| goes to the right place. The mystery ensues.