Extension context menu refactor

Refactor ExtensionActionContextMenuModel.

- Simplify constructor
- Rename to ExtensionContextMenuModel.*
- Remove views/extension_action_context_menu.*

BUG=none
TEST=none


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

[Evan Stade, 2010-03-19 00:07:57]

This doesn't work on windows (it breaks the context menu for the browser action
overflow); I'll fix that but it's taking me forever to get a debug build on
windows.

[rafaelw1, 2010-03-19 00:42:21]

Will review tonight. To address the windows issue, you should just build
views on Linux.

On Mar 18, 2010 5:08 PM, <estade@chromium.org> wrote:

Reviewers: Finnur, rafaelw,

Message:
This doesn't work on windows (it breaks the context menu for the browser
action
overflow); I'll fix that but it's taking me forever to get a debug build on
windows.

Description:
Refactor ExtensionActionContextMenuModel.

- Create separate constructors for the ExtensionAction and Extension uses.
- Rename to ExtensionContextMenuModel.
- Remove views/extension_action_context_menu.*

BUG=none
TEST=none


Please review this at http://codereview.chromium.org/1107007

SVN Base: svn://chrome-svn/chrome/trunk/src/

Affected files:
 D     chrome/browser/extensions/extension_action_context_menu_model.h
 D     chrome/browser/extensions/extension_action_context_menu_model.cc
 A  +  chrome/browser/extensions/extension_context_menu_model.h
 A  +  chrome/browser/extensions/extension_context_menu_model.cc
 M     chrome/browser/gtk/browser_actions_toolbar_gtk.cc
 M     chrome/browser/gtk/location_bar_view_gtk.h
 M     chrome/browser/gtk/location_bar_view_gtk.cc
 M     chrome/browser/views/browser_actions_container.h
 M     chrome/browser/views/browser_actions_container.cc
 M
chrome/browser/views/extensions/browser_action_overflow_menu_controller.h
 M
chrome/browser/views/extensions/browser_action_overflow_menu_controller.cc
 D     chrome/browser/views/extensions/extension_action_context_menu.h
 D     chrome/browser/views/extensions/extension_action_context_menu.cc
 M     chrome/browser/views/extensions/extension_installed_bubble.cc
 M     chrome/browser/views/infobars/extension_infobar.h
 M     chrome/browser/views/infobars/extension_infobar.cc
 M     chrome/browser/views/location_bar_view.h
 M     chrome/browser/views/location_bar_view.cc
 M     chrome/chrome_browser.gypi

[Evan Stade, 2010-03-19 01:05:34]

fixed it. The context menu model was being deleted before it executed the
command. Delayed delete fixes that.

[rafaelw, 2010-03-19 02:57:41]

lgtm on

extension_(action_)context_menu_model.* and gtk/*.

leaving the rest for finnur.

http://codereview.chromium.org/1107007/diff/50001/41013
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/1107007/diff/50001/41013#newcode229
chrome/browser/views/browser_actions_container.cc:229: // Reconstructs the menu
every time because some menu item.
huh?

[Finnur, 2010-03-19 16:58:20]

http://codereview.chromium.org/1107007/diff/50001/41004
File chrome/browser/extensions/extension_context_menu_model.cc (left):

http://codereview.chromium.org/1107007/diff/50001/41004#oldcode70
chrome/browser/extensions/extension_context_menu_model.cc:70: return false;
Are you sure this can be removed?

http://codereview.chromium.org/1107007/diff/50001/41004
File chrome/browser/extensions/extension_context_menu_model.cc (right):

http://codereview.chromium.org/1107007/diff/50001/41004#newcode62
chrome/browser/extensions/extension_context_menu_model.cc:62: void
ExtensionContextMenuModel::InitCommonCommands() {
Why is this not called InitMenu, or something more transparent like that?

http://codereview.chromium.org/1107007/diff/50001/41003
File chrome/browser/extensions/extension_context_menu_model.h (right):

http://codereview.chromium.org/1107007/diff/50001/41003#newcode27
chrome/browser/extensions/extension_context_menu_model.h:27: virtual void
ShowPopupForDevToolsWindow(ExtensionAction* action) = 0;
Eeek... Rafael, can we just call this method InspectPopup?

http://codereview.chromium.org/1107007/diff/50001/41003#newcode54
chrome/browser/extensions/extension_context_menu_model.h:54: void
InitCommonCommands();
nit: Document.

http://codereview.chromium.org/1107007/diff/50001/41020
File chrome/browser/gtk/location_bar_view_gtk.h (right):

http://codereview.chromium.org/1107007/diff/50001/41020#newcode203
chrome/browser/gtk/location_bar_view_gtk.h:203: // Overridden from
ExtensionContextMenuModel::PopupDelegate
Missing : at the end.

http://codereview.chromium.org/1107007/diff/50001/41013
File chrome/browser/views/browser_actions_container.cc (left):

http://codereview.chromium.org/1107007/diff/50001/41013#oldcode424
chrome/browser/views/browser_actions_container.cc:424:
overflow_menu_->CancelMenu();
Why are you removing this function? It seems it would be better to rename it
CloseOverflowMenu(), than to replace one line at all the call sites with three?

http://codereview.chromium.org/1107007/diff/50001/41013
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/1107007/diff/50001/41013#newcode233
chrome/browser/views/browser_actions_container.cc:233:
context_menu_menu_->RunContextMenuAt(point);
This reminds me of another tricky issue. Make sure this sequence works:

1) right-click browser action (test inside and outside of the overflow).
2) with the context menu still open, right click a bookmark/omnibox and make
sure the right menu appears.

http://codereview.chromium.org/1107007/diff/50001/41005
File chrome/browser/views/browser_actions_container.h (left):

http://codereview.chromium.org/1107007/diff/50001/41005#oldcode261
chrome/browser/views/browser_actions_container.h:261:
ExtensionActionContextMenu* GetContextMenu();
OK, be very careful here. Try putting all browser actions into the overflow
menu, right-click on one browser action icon to get the context menu and make
sure the context menu works (try uninstalling an extension, for example).
Obviously the context menu needs to work also on icons that are not in the
overflow menu, but that's probably your test case (the other one is
non-obvious).

We also need to make sure the whole thing doesn't come crashing down if we are
showing a menu and the extension crashes/gets updated. And that we cannot show
the menu if it is already showing. (These are concerns that were raised when
this was implemented initially and we took care of -- don't want that to
regress).

My main worry is that BrowserActionButtons get created and deleted as the
container changes and since it now owns the context menu, you might get
crashes/inoperable menu in some cases if that happens while the menu is showing.

http://codereview.chromium.org/1107007/diff/50001/41005
File chrome/browser/views/browser_actions_container.h (right):

http://codereview.chromium.org/1107007/diff/50001/41005#newcode123
chrome/browser/views/browser_actions_container.h:123: scoped_ptr<views::Menu2>
context_menu_menu_;
I always felt this menu_menu name was a bit silly. Can we just call it
context_menu?

What is the motivation for moving this to the button?

http://codereview.chromium.org/1107007/diff/50001/41011
File chrome/browser/views/extensions/browser_action_overflow_menu_controller.cc
(right):

http://codereview.chromium.org/1107007/diff/50001/41011#newcode77
chrome/browser/views/extensions/browser_action_overflow_menu_controller.cc:77:
MessageLoop::current()->PostTask(FROM_HERE, this);
It is non-obvious from just looking at this code what this does (and the bots
don't like it). :)

Can you describe (in a comment) what the task is and what this accomplishes.

[Evan Stade, 2010-03-19 17:32:32]

http://codereview.chromium.org/1107007/diff/50001/41004
File chrome/browser/extensions/extension_context_menu_model.cc (left):

http://codereview.chromium.org/1107007/diff/50001/41004#oldcode70
chrome/browser/extensions/extension_context_menu_model.cc:70: return false;
On 2010/03/19 16:58:20, Finnur wrote:
> Are you sure this can be removed?

yes; INSPECT_POPUP is only added when delegate_ is non-NULL. (It is an error to
pass a NULL delegate_ to the constructor that takes a delegate.)

http://codereview.chromium.org/1107007/diff/50001/41004
File chrome/browser/extensions/extension_context_menu_model.cc (right):

http://codereview.chromium.org/1107007/diff/50001/41004#newcode62
chrome/browser/extensions/extension_context_menu_model.cc:62: void
ExtensionContextMenuModel::InitCommonCommands() {
On 2010/03/19 16:58:20, Finnur wrote:
> Why is this not called InitMenu, or something more transparent like that?

well I wanted the name to suggest that it was only initializing the common bits
of the menu, and not the whole menu. But I am open to suggestions for better
names that maintain this property.

http://codereview.chromium.org/1107007/diff/50001/41003
File chrome/browser/extensions/extension_context_menu_model.h (right):

http://codereview.chromium.org/1107007/diff/50001/41003#newcode27
chrome/browser/extensions/extension_context_menu_model.h:27: virtual void
ShowPopupForDevToolsWindow(ExtensionAction* action) = 0;
On 2010/03/19 16:58:20, Finnur wrote:
> Eeek... Rafael, can we just call this method InspectPopup?

+1

http://codereview.chromium.org/1107007/diff/50001/41020
File chrome/browser/gtk/location_bar_view_gtk.h (right):

http://codereview.chromium.org/1107007/diff/50001/41020#newcode203
chrome/browser/gtk/location_bar_view_gtk.h:203: // Overridden from
ExtensionContextMenuModel::PopupDelegate
On 2010/03/19 16:58:20, Finnur wrote:
> Missing : at the end.

Done.

http://codereview.chromium.org/1107007/diff/50001/41013
File chrome/browser/views/browser_actions_container.cc (left):

http://codereview.chromium.org/1107007/diff/50001/41013#oldcode424
chrome/browser/views/browser_actions_container.cc:424:
overflow_menu_->CancelMenu();
On 2010/03/19 16:58:20, Finnur wrote:
> Why are you removing this function? It seems it would be better to rename it
> CloseOverflowMenu(), than to replace one line at all the call sites with
three?
> 

shrug, ok

http://codereview.chromium.org/1107007/diff/50001/41013
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/1107007/diff/50001/41013#newcode229
chrome/browser/views/browser_actions_container.cc:229: // Reconstructs the menu
every time because some menu item.
On 2010/03/19 02:57:42, rafaelw wrote:
> huh?

should read "...every time because the menu's makeup is dynamic"

http://codereview.chromium.org/1107007/diff/50001/41013#newcode233
chrome/browser/views/browser_actions_container.cc:233:
context_menu_menu_->RunContextMenuAt(point);
On 2010/03/19 16:58:20, Finnur wrote:
> This reminds me of another tricky issue. Make sure this sequence works:
> 
> 1) right-click browser action (test inside and outside of the overflow).

works

> 2) with the context menu still open, right click a bookmark/omnibox and make
> sure the right menu appears.

will test

http://codereview.chromium.org/1107007/diff/50001/41005
File chrome/browser/views/browser_actions_container.h (left):

http://codereview.chromium.org/1107007/diff/50001/41005#oldcode261
chrome/browser/views/browser_actions_container.h:261:
ExtensionActionContextMenu* GetContextMenu();
On 2010/03/19 16:58:20, Finnur wrote:
> OK, be very careful here. Try putting all browser actions into the overflow
> menu, right-click on one browser action icon to get the context menu and make
> sure the context menu works (try uninstalling an extension, for example).
> Obviously the context menu needs to work also on icons that are not in the
> overflow menu, but that's probably your test case (the other one is
> non-obvious).

this works.

> 
> We also need to make sure the whole thing doesn't come crashing down if we are
> showing a menu and the extension crashes/gets updated. And that we cannot show
> the menu if it is already showing. (These are concerns that were raised when
> this was implemented initially and we took care of -- don't want that to
> regress).

suggestions on how to test this?

> 
> My main worry is that BrowserActionButtons get created and deleted as the
> container changes and since it now owns the context menu, you might get
> crashes/inoperable menu in some cases if that happens while the menu is
showing.

if the button is deleted, that will take the menu with it, since the button owns
the menu. Unless you are suggesting that deleting a views::Menu2 while it is
showing without first calling CancelMenu() is wrong; I just assumed it was ok.

http://codereview.chromium.org/1107007/diff/50001/41005
File chrome/browser/views/browser_actions_container.h (right):

http://codereview.chromium.org/1107007/diff/50001/41005#newcode123
chrome/browser/views/browser_actions_container.h:123: scoped_ptr<views::Menu2>
context_menu_menu_;
On 2010/03/19 16:58:20, Finnur wrote:
> I always felt this menu_menu name was a bit silly. Can we just call it
> context_menu?

just following convention.

> 
> What is the motivation for moving this to the button?

seemed like it made the code cleaner. For example I got to remove part of the
CloseMenus function since the button will now own the menu and the container
doesn't have to worry about it.

http://codereview.chromium.org/1107007/diff/50001/41011
File chrome/browser/views/extensions/browser_action_overflow_menu_controller.cc
(right):

http://codereview.chromium.org/1107007/diff/50001/41011#newcode77
chrome/browser/views/extensions/browser_action_overflow_menu_controller.cc:77:
MessageLoop::current()->PostTask(FROM_HERE, this);
On 2010/03/19 16:58:20, Finnur wrote:
> It is non-obvious from just looking at this code what this does (and the bots
> don't like it). :)
> 
> Can you describe (in a comment) what the task is and what this accomplishes.

this was supposed to be DeleteSoon. Scribal error.

[Finnur, 2010-03-19 19:43:18]

http://codereview.chromium.org/1107007/diff/50001/41004
File chrome/browser/extensions/extension_context_menu_model.cc (right):

http://codereview.chromium.org/1107007/diff/50001/41004#newcode62
chrome/browser/extensions/extension_context_menu_model.cc:62: void
ExtensionContextMenuModel::InitCommonCommands() {
Initialize does not imply that the object reaches a state that is never modified
afterwards. However, now that I think about this, I would prefer not to split up
the constructor like this. It gives you little benefit and introduces risk of
the initialization lists getting out of sync. It is also a bit confusing why
this class needs two constructors.

I would prefer adding an AddInspectedPopupItem function
that does the only thing that is different between these functions (add one item
to the list).

On 2010/03/19 17:32:32, Evan Stade wrote:
> But I am open to suggestions for better
> names that maintain this property.

http://codereview.chromium.org/1107007/diff/50001/41013
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/1107007/diff/50001/41013#newcode233
chrome/browser/views/browser_actions_container.cc:233:
context_menu_menu_->RunContextMenuAt(point);
> works

... on which platforms?

> will test

... what was the outcome?

On 2010/03/19 17:32:32, Evan Stade wrote:
> On 2010/03/19 16:58:20, Finnur wrote:
> > This reminds me of another tricky issue. Make sure this sequence works:
> > 
> > 1) right-click browser action (test inside and outside of the overflow).
> 
> works
> 
> > 2) with the context menu still open, right click a bookmark/omnibox and make
> > sure the right menu appears.
> 
> will test

http://codereview.chromium.org/1107007/diff/50001/41005
File chrome/browser/views/browser_actions_container.h (left):

http://codereview.chromium.org/1107007/diff/50001/41005#oldcode261
chrome/browser/views/browser_actions_container.h:261:
ExtensionActionContextMenu* GetContextMenu();
> this works.

On what platform?

> suggestions on how to test [crashing extension]?

I wrote a little Windows utility that shoots down any new process of name 'foo',
checking every 20 seconds. This gives you time to load an extension and
right-click on it before the extension process gets shot down. I can share that
with you if you are interested.

> if the button is deleted, that will take the menu with it, since the button
owns
> the menu. Unless you are suggesting that deleting a views::Menu2 while it is
> showing without first calling CancelMenu() is wrong; I just assumed it was ok.

I'm not sure. Probably is. But, assuming it is - won't you now have menu's
closing on the user if the implementation decided that the view vector needed to
be recreated? Before, this wasn't an issue because the lifetime was not tied to
the vector.

http://codereview.chromium.org/1107007/diff/50001/41005
File chrome/browser/views/browser_actions_container.h (right):

http://codereview.chromium.org/1107007/diff/50001/41005#newcode123
chrome/browser/views/browser_actions_container.h:123: scoped_ptr<views::Menu2>
context_menu_menu_;
> just following convention.

Are you saying I should ask you to change it everywhere? :)

I don't think there is a rule that says you have to name your members the same
thing as someone else decides to do, especially not if the naming is weird. :)

[Evan Stade, 2010-03-19 21:16:38]

http://codereview.chromium.org/1107007/diff/50001/41004
File chrome/browser/extensions/extension_context_menu_model.cc (right):

http://codereview.chromium.org/1107007/diff/50001/41004#newcode62
chrome/browser/extensions/extension_context_menu_model.cc:62: void
ExtensionContextMenuModel::InitCommonCommands() {
On 2010/03/19 19:43:19, Finnur wrote:
> Initialize does not imply that the object reaches a state that is never
modified
> afterwards. However, now that I think about this, I would prefer not to split
up
> the constructor like this. It gives you little benefit and introduces risk of
> the initialization lists getting out of sync.

on the contrary, not having a common initializing function is what introduces
the risk of the lists getting out of sync.

> It is also a bit confusing why
> this class needs two constructors.
> I would prefer adding an AddInspectedPopupItem function
> that does the only thing that is different between these functions (add one
item
> to the list).

the whole point of this refactor is that what started as a class for *extension
action* context menus started being used for extensions that had no extension
action. I think it is really ugly to have one huge constructor that takes 4
parameters, and either uses the first 1 and expects the others to be null, or
uses the last 3 and expects the first one to be null. This is a perfect example
of when you would need more than one constructor for one class.

> 
> On 2010/03/19 17:32:32, Evan Stade wrote:
> > But I am open to suggestions for better
> > names that maintain this property.
> 
>

http://codereview.chromium.org/1107007/diff/50001/41013
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/1107007/diff/50001/41013#newcode233
chrome/browser/views/browser_actions_container.cc:233:
context_menu_menu_->RunContextMenuAt(point);
On 2010/03/19 19:43:19, Finnur wrote:
> > works
> 
> ... on which platforms?

windows

> 
> > will test
> 
> ... what was the outcome?

success

> 
> On 2010/03/19 17:32:32, Evan Stade wrote:
> > On 2010/03/19 16:58:20, Finnur wrote:
> > > This reminds me of another tricky issue. Make sure this sequence works:
> > > 
> > > 1) right-click browser action (test inside and outside of the overflow).
> > 
> > works
> > 
> > > 2) with the context menu still open, right click a bookmark/omnibox and
make
> > > sure the right menu appears.
> > 
> > will test
> 
>

http://codereview.chromium.org/1107007/diff/50001/41005
File chrome/browser/views/browser_actions_container.h (left):

http://codereview.chromium.org/1107007/diff/50001/41005#oldcode261
chrome/browser/views/browser_actions_container.h:261:
ExtensionActionContextMenu* GetContextMenu();
On 2010/03/19 19:43:19, Finnur wrote:
> > this works.
> 
> On what platform?
> 
> > suggestions on how to test [crashing extension]?
> 
> I wrote a little Windows utility that shoots down any new process of name
'foo',
> checking every 20 seconds. This gives you time to load an extension and
> right-click on it before the extension process gets shot down. I can share
that
> with you if you are interested.
> 
> > if the button is deleted, that will take the menu with it, since the button
> owns
> > the menu. Unless you are suggesting that deleting a views::Menu2 while it is
> > showing without first calling CancelMenu() is wrong; I just assumed it was
ok.
> 
> I'm not sure. Probably is. But, assuming it is - won't you now have menu's
> closing on the user if the implementation decided that the view vector needed
to
> be recreated? Before, this wasn't an issue because the lifetime was not tied
to
> the vector.

well actually it was tied to the vector because any time you added or removed an
extension you were closing the menu. I think it's fine that the context menu for
a view is tied to the lifetime of that view. That is the case for almost every
context menu in chrome.

http://codereview.chromium.org/1107007/diff/50001/41005
File chrome/browser/views/browser_actions_container.h (right):

http://codereview.chromium.org/1107007/diff/50001/41005#newcode123
chrome/browser/views/browser_actions_container.h:123: scoped_ptr<views::Menu2>
context_menu_menu_;
On 2010/03/19 19:43:19, Finnur wrote:
> > just following convention.
> 
> Are you saying I should ask you to change it everywhere? :)
> 
> I don't think there is a rule that says you have to name your members the same
> thing as someone else decides to do, especially not if the naming is weird. :)

I'm saying you should ask me to file a bug if the convention is wrong. Sticking
to convention is important; changing the convention is outside the scope of this
cleanup effort.

[Finnur, 2010-03-19 21:47:22]

http://codereview.chromium.org/1107007/diff/50001/41004
File chrome/browser/extensions/extension_context_menu_model.cc (right):

http://codereview.chromium.org/1107007/diff/50001/41004#newcode62
chrome/browser/extensions/extension_context_menu_model.cc:62: void
ExtensionContextMenuModel::InitCommonCommands() {
> on the contrary, not having a common initializing 
> function is what introduces
> the risk of the lists getting out of sync.

I don't think you understand my point. When I say initialization, I'm not
talking about InitCommonConstants. I'm talking about having two sets of member
initialization lists because you now have two constructors. *Those* two can get
out of sync. 

My point was: have one constructor, which takes one parameter and doesn't add
the extra menu item and supply a function that adds the extra menu item (if
needed) using some of the parameters you are currently passing into the second
constructor.

Then you can get rid of this InitCommonCommands function and just add the items
in the constructor.

http://codereview.chromium.org/1107007/diff/50001/41005
File chrome/browser/views/browser_actions_container.h (right):

http://codereview.chromium.org/1107007/diff/50001/41005#newcode123
chrome/browser/views/browser_actions_container.h:123: scoped_ptr<views::Menu2>
context_menu_menu_;
> I'm saying you should ask me to file a bug if 
> the convention is wrong.

*sigh*

Whatever. This is pointless. I think we've spent more time and mental effort
discussing this than it takes to change all of these stupid naming instances. I
think we both know filing a bug is the same as doing nothing. So, forget it.
I'll do this myself, next time I run into this.

[Evan Stade, 2010-03-19 22:09:30]

http://codereview.chromium.org/1107007/diff/50001/41004
File chrome/browser/extensions/extension_context_menu_model.cc (right):

http://codereview.chromium.org/1107007/diff/50001/41004#newcode62
chrome/browser/extensions/extension_context_menu_model.cc:62: void
ExtensionContextMenuModel::InitCommonCommands() {
> My point was: have one constructor, which takes one parameter and doesn't add
> the extra menu item and supply a function that adds the extra menu item (if
> needed) using some of the parameters you are currently passing into the second
> constructor.

Sorry, I am really not trying to be combative. The dual constructor usage just
seems to me the perfect hammer for this situation. Please bear with me :)

Just for the purpose of illustration, I want to point out the following: without
two constructors, it will look like this in the extension action case:

extension = UseExtensionServiceToLookupExtensionFromExtensionAction(action);
context_menu_model_ = new ExtensionContextMenuModel(extension);
context_menu_model_->SetExtensionAction(action, profile, delegate);

Or with multiple contsructors,

context_menu_model_ = new ExtensionContextMenuModel(action, profile, delegate);

In general it is good practice to put initialization code in a helper function
rather than the constructor itself. Dean explained why on the mailing list a
while back, and I can't remember all the details. For my part I think it is
cleaner and easier to read.

But moreover I'm not really sure why you are so strongly against having multiple
constructors. The need for separate initialization lists will always exist with
multiple constructors, but more pertinently, using an after-the-fact setter as
shown above does not fix this problem. There will still be two points within the
code that set the value of |delegate_|, so two points for the code to possibly
get "out of sync" (although in this case, given that the initializer lists have
nothing in common, I'm not sure there is any un-syncing possible :)

[Evan Stade, 2010-03-19 23:56:15]

ok here is another option that I think makes sense. ExtensionActionContextMenu
as a subclass of ExtensionContextMenu.

[Finnur, 2010-03-20 18:31:35]

On 2010/03/19 23:56:15, Evan Stade wrote:
> ok here is another option that I think makes sense. ExtensionActionContextMenu
> as a subclass of ExtensionContextMenu.

I appreciate the suggestion, but I think that just makes things more complicated
than they need to be. 

As for the previous comments, I don't see this as a good case for multiple
constructors.

I was initially thinking you should have one simple constructor and a helper
function to add the menu item if needed, which supplies the needed information.
However, with a clear(er) mind today and looking at all the call sites I think
this API can be simplified even further.

Here is my suggestion: Have only one constructor, which takes as params
Extension*, Profile*, Delegate*. Looking at the callers, everyone of them has
all the information requested by the constructor (if you delete two fewer lines
in LocationBarViewGtk).

You don't need to pass in ExtensionAction* because that can be derived from the
Extension* (An extension cannot have both a browser action and a page action so
you can DCHECK on that and pick the one that is available). You should pass in
Profile* because your other constructor is using GetLastActive to try to figure
it out so why not just supply it? And the Delegate should be NULL for those
callers that don't want to add the INSPECT_POPUP item (at the moment there is
only one). Taking a step back, I think ExtensionInfoBar should also pass in a
delegate, assuming we make the INSPECT_POPUP more general as an INSPECT_VIEW
command, but that's outside the scope of this CL.

So, Delegate signifies whether you want the Inspect item added. You can keep
InitCommonCommands if you want, but it should be InitMenuCommands and should
also initialize the PopupMenu for inspecting if delegate is non-NULL.

I think this approach is much simpler than having two constructors/subclassing.
With multiple ctors, it is non-obvious to people which ctor to pick and easy for
developers adding member variables to forget to update both constructors
initialization lists. Having one constructor with a clearly documented Delegate
pointer alleviates both of those concerns.

[Evan Stade, 2010-03-22 04:14:30]

On 2010/03/20 18:31:35, Finnur wrote:
> On 2010/03/19 23:56:15, Evan Stade wrote:
> > ok here is another option that I think makes sense.
ExtensionActionContextMenu
> > as a subclass of ExtensionContextMenu.
> 
> I appreciate the suggestion, but I think that just makes things more
complicated
> than they need to be. 
> 
> As for the previous comments, I don't see this as a good case for multiple
> constructors.
> 
> I was initially thinking you should have one simple constructor and a helper
> function to add the menu item if needed, which supplies the needed
information.
> However, with a clear(er) mind today and looking at all the call sites I think
> this API can be simplified even further.
> 
> Here is my suggestion: Have only one constructor, which takes as params
> Extension*, Profile*, Delegate*. Looking at the callers, everyone of them has
> all the information requested by the constructor (if you delete two fewer
lines
> in LocationBarViewGtk).
> 
> You don't need to pass in ExtensionAction* because that can be derived from
the
> Extension* (An extension cannot have both a browser action and a page action
so
> you can DCHECK on that and pick the one that is available). You should pass in
> Profile* because your other constructor is using GetLastActive to try to
figure
> it out so why not just supply it? And the Delegate should be NULL for those
> callers that don't want to add the INSPECT_POPUP item (at the moment there is
> only one). Taking a step back, I think ExtensionInfoBar should also pass in a
> delegate, assuming we make the INSPECT_POPUP more general as an INSPECT_VIEW
> command, but that's outside the scope of this CL.
> 
> So, Delegate signifies whether you want the Inspect item added. You can keep
> InitCommonCommands if you want, but it should be InitMenuCommands and should
> also initialize the PopupMenu for inspecting if delegate is non-NULL.
> 
> I think this approach is much simpler than having two
constructors/subclassing.
> With multiple ctors, it is non-obvious to people which ctor to pick and easy
for
> developers adding member variables to forget to update both constructors
> initialization lists. Having one constructor with a clearly documented
Delegate
> pointer alleviates both of those concerns.

I like this idea. I wasn't aware of the limitation of one extension action per
extension, or that we'd eventually want an inspect option for the extension info
bar. I will rework this cl accordingly.

[rafaelw, 2010-03-22 18:04:04]

http://codereview.chromium.org/1107007/diff/50001/41003
File chrome/browser/extensions/extension_context_menu_model.h (right):

http://codereview.chromium.org/1107007/diff/50001/41003#newcode27
chrome/browser/extensions/extension_context_menu_model.h:27: virtual void
ShowPopupForDevToolsWindow(ExtensionAction* action) = 0;
Fine by me.

On 2010/03/19 16:58:20, Finnur wrote:
> Eeek... Rafael, can we just call this method InspectPopup?

[Evan Stade, 2010-03-22 20:55:00]

new patch uploaded.

I tested what happens when the extension crashes while the context menu is
showing. In the overflow menu, everything works as expected. In the browser
action toolbar, it causes a browser crash. However I reverted my change and
tried again and it still caused a browser crash in this case. The reason is
because of the OnMousePressed() function. Here is the  version of that function
from before this patch:

bool BrowserActionButton::OnMousePressed(const views::MouseEvent& e) {
  showing_context_menu_ = e.IsRightMouseButton();
  if (showing_context_menu_) {
    SetButtonPushed();

    // Get the top left point of this button in screen coordinates.
    gfx::Point point = gfx::Point(0, 0);
    ConvertPointToScreen(this, &point);

    // Make the menu appear below the button.
    point.Offset(0, height());

    panel_->GetContextMenu()->Run(extension(), extension()->browser_action(),
        panel_, panel_->profile()->GetPrefs(), point);

    SetButtonNotPushed();
    return false;
  } else if (IsPopup()) {
    return MenuButton::OnMousePressed(e);
  }
  return TextButton::OnMousePressed(e);
}

If the button is destroyed in the nested message loop created by Run(), then
SetButtonNotPushed() will cause an access violation. This patch neither solves
nor causes this problem so I'd like to tackle that as a separate issue.

[Finnur, 2010-03-22 21:10:09]

You have some try server issues, but apart from that this LGTM, with one nit.

> This patch neither solves nor causes this problem so 
> I'd like to tackle that as a separate issue.

Sounds very reasonable.

http://codereview.chromium.org/1107007/diff/7004/71002
File chrome/browser/extensions/extension_context_menu_model.cc (right):

http://codereview.chromium.org/1107007/diff/7004/71002#newcode34
chrome/browser/extensions/extension_context_menu_model.cc:34: :
ALLOW_THIS_IN_INITIALIZER_LIST(SimpleMenuModel(this)),
nit: I believe you need 4 cols before the colon in the initializer list.

[Finnur, 2010-03-22 21:10:59]

And thanks for the cleanup and patience too. :)

On 2010/03/22 21:10:09, Finnur wrote:
> You have some try server issues, but apart from that this LGTM, with one nit.
> 
> > This patch neither solves nor causes this problem so 
> > I'd like to tackle that as a separate issue.
> 
> Sounds very reasonable.
> 
> http://codereview.chromium.org/1107007/diff/7004/71002
> File chrome/browser/extensions/extension_context_menu_model.cc (right):
> 
> http://codereview.chromium.org/1107007/diff/7004/71002#newcode34
> chrome/browser/extensions/extension_context_menu_model.cc:34: :
> ALLOW_THIS_IN_INITIALIZER_LIST(SimpleMenuModel(this)),
> nit: I believe you need 4 cols before the colon in the initializer list.