Don't crash when reusing a bookmark context menu.

Rebuild the bookmark context menu from scratch when selection changes.

Since the "remove" item always causes selection to change, we don't have to worry about trying to call remove twice with the same context menu.

TEST=see bug. Also, no new crashes on linux.
BUG=14443
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=18951

[Evan Stade, 2009-06-18 23:57:02]

what was the original purpose of the RemoveModelObserver() call?

[sky, 2009-06-19 00:12:25]

http://codereview.chromium.org/132048/diff/1/2
File chrome/browser/bookmarks/bookmark_context_menu.cc (left):

http://codereview.chromium.org/132048/diff/1/2#oldcode352
Line 352: BookmarkModel* model = RemoveModelObserver();
The reason I did this is that as soon as you remove an item BookmarkNodeRemoved
is invoked, which then invokes menu_->Cancel(). I was worried about the effects
cancelling the menu has on this object and wanted to avoid that.

BookmarkContextMenu was designed to be used once, then chucked. Looks like that
changed recently though. If you're going to reuse it, that's fine, but I think
you should have to reset the model each time through. To reinforce that
Run/Popup should take all the arguments that are passed in the constructor.

To avoid these sort of problems I don't think we should attempt to reuse the
menu.

[Evan Stade, 2009-06-19 00:26:20]

I am reusing it because I think it's really hard and unnatural in gtk to
generate the menu when it is popped up (it's fine when it's being used as a
popup menu, but not so much when it's the bookmark manager's organize menu). I
don't know what kind of problems you are worried about with menu_->Cancel().
Doesn't that just stop showing the menu? How come during deletion of a node is
the only time we are worried?

[sky, 2009-06-19 03:43:37]

On 2009/06/19 00:26:20, Evan Stade wrote:
> I am reusing it because I think it's really hard and unnatural in gtk to
> generate the menu when it is popped up (it's fine when it's being used as a
> popup menu, but not so much when it's the bookmark manager's organize menu). I
> don't know what kind of problems you are worried about with menu_->Cancel().
> Doesn't that just stop showing the menu? How come during deletion of a node is
> the only time we are worried?

I was worried about the possibility of cancel deleting this, but I think was too
hasty in that thinking. So, I'm ok with this approach as long as you do the
following:

1. Remove all args from the constructor and put them on the RunMenuAt/Popup
method. This makes the lifetime clearer.
2. I don't like the idea of the menu being an observer for longer than it needs
to be. So, make sure you uninstall the listener from the model after the menu is
hidden, as well the destructor.
3. reset all args after running.

I'll be out tomorrow, feel free to get Ben to finish the review tomorrow.

[Evan Stade, 2009-06-22 16:54:10]

On 2009/06/19 03:43:37, sky wrote:
> On 2009/06/19 00:26:20, Evan Stade wrote:
> > I am reusing it because I think it's really hard and unnatural in gtk to
> > generate the menu when it is popped up (it's fine when it's being used as a
> > popup menu, but not so much when it's the bookmark manager's organize menu).
I
> > don't know what kind of problems you are worried about with menu_->Cancel().
> > Doesn't that just stop showing the menu? How come during deletion of a node
is
> > the only time we are worried?
> 
> I was worried about the possibility of cancel deleting this, but I think was
too
> hasty in that thinking. So, I'm ok with this approach as long as you do the
> following:
> 
> 1. Remove all args from the constructor and put them on the RunMenuAt/Popup
> method. This makes the lifetime clearer.

For the bookmark manager organize menu, we never call PopupAsContext(), or any
other Popup() function (MenuGtk has several). We want to avoid this and instead
let GTK handle when and where to show the menu (for best native feel).

> 2. I don't like the idea of the menu being an observer for longer than it
needs
> to be. So, make sure you uninstall the listener from the model after the menu
is
> hidden, as well the destructor.

can do.

> 3. reset all args after running.

see 1.

> I'll be out tomorrow, feel free to get Ben to finish the review tomorrow.

[sky, 2009-06-22 20:30:07]

LGTM