Mac bookmark mgr: Restore previously selected group when search field is cleared.

Mac bookmark mgr: Restore previously selected group when search field is cleared.
BUG=32734
TEST=none

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

[Nico, 2010-01-27 00:59:17]

LG if you change the preSearchGroup from a scoped_nsobject to a pointer.

Ping Pink to give you ForMerge for your bookmark manager CLs btw, since stuff
that isn't merged into the next beta will probably never be seen by users.
That'd be a pity.

http://codereview.chromium.org/555112/diff/1/2
File chrome/browser/cocoa/bookmark_manager_controller.h (right):

http://codereview.chromium.org/555112/diff/1/2#newcode32
chrome/browser/cocoa/bookmark_manager_controller.h:32:
scoped_nsobject<BookmarkItem> preSearchGroup_;  // Selected before search
Shouldn't this just be a weak pointer?

http://codereview.chromium.org/555112/diff/1/5
File chrome/browser/cocoa/bookmark_tree_controller.mm (right):

http://codereview.chromium.org/555112/diff/1/5#newcode466
chrome/browser/cocoa/bookmark_tree_controller.mm:466: [self
setSelectedItems:sel];
This seems unrelated. (Just pointing it out, it doesn't necessarily seem wrong.)

[Jens Alfke, 2010-01-27 17:08:47]

On 2010/01/27 00:59:17, Nico wrote:
> LG if you change the preSearchGroup from a scoped_nsobject to a pointer.

See inline for why I don't want to do that.
I'll await your approval before committing, since technically I haven't
satisfied your "if" condition :)

[Jens Alfke, 2010-01-27 17:12:59]

http://codereview.chromium.org/555112/diff/1/2
File chrome/browser/cocoa/bookmark_manager_controller.h (right):

http://codereview.chromium.org/555112/diff/1/2#newcode32
chrome/browser/cocoa/bookmark_manager_controller.h:32:
scoped_nsobject<BookmarkItem> preSearchGroup_;  // Selected before search
On 2010/01/27 00:59:18, Nico wrote:
> Shouldn't this just be a weak pointer?

That would make assumptions about the refcounting of BookmarkItems. As currently
implemented that would probably be safe because nodeMap_ keep strong references
to all of them; but there's no reason why closing a parent folder in the outline
couldn't release the last ref to a BookmarkItem. And there's no downside to
keeping a reference here.

http://codereview.chromium.org/555112/diff/1/5
File chrome/browser/cocoa/bookmark_tree_controller.mm (right):

http://codereview.chromium.org/555112/diff/1/5#newcode466
chrome/browser/cocoa/bookmark_tree_controller.mm:466: [self
setSelectedItems:sel];
On 2010/01/27 00:59:18, Nico wrote:
> This seems unrelated. (Just pointing it out, it doesn't necessarily seem
wrong.)

I actually had to fix this because for some reason NSOutlineView was restoring
the selection incorrectly after the Search group was added, which screwed up
returning to the original selection afterwards.

[Jens Alfke, 2010-01-27 19:09:57]

I found a stupid bug that I introduced in one of the last changes I made before
sending out the initial patch. :-( New patch is uploaded now, including unit
test for that bug.

[Nico, 2010-01-28 16:52:23]

LG.

The scoped_nsobject still feels weird to me, but that might just be me.

Unrelated: Have you tried the following? The user is in a folder, then starts a
search. Then he switches to the main browser window and deletes the folder he
was in. Then he goes back and cancels the search. What happens? (everything !=
crash is fine)

[Jens Alfke, 2010-01-28 21:33:14]

On 2010/01/28 16:52:23, Nico wrote:
> Have you tried the following? The user is in a folder, then starts a
> search. Then he switches to the main browser window and deletes the folder he
> was in. Then he goes back and cancels the search. What happens? 

That's what the lines added to -forgetNode: take care of: they forget the
remembered folder if it's deleted. When you clear the search field it won't
change the selection.