Added support for context menus to status icons.

Added support for context menus to status icons.

BUG=37375
TEST=updated StatusIcon unit tests

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

[Andrew T Wilson (Slow), 2010-08-19 05:59:11]

I know you're gardening, but hopefully you'll have the cycles to look at this.
If not, let me know and I'll see if I can dig up somebody else, but you're one
of the few people I know with Win/Views experience so I figured I'd ask you
first...

[tfarina, 2010-08-19 12:56:51]

http://codereview.chromium.org/3189003/diff/5001/6014
File chrome/browser/views/status_icons/status_icon_win.h (right):

http://codereview.chromium.org/3189003/diff/5001/6014#newcode45
chrome/browser/views/status_icons/status_icon_win.h:45: //
menus::SimpleMenuModel::Delegate
Rather than adding this code here, could you move into the status_icons/
directory, so that it can be shared with the gtk side (and mac side if needed)
as well.

See in chrome/browser/notifications/notification_options_menu_model.[cc/h] for
an example of what I did.

Or just add a TODO, to address this later. :-)

[John Gregg, 2010-08-19 16:36:29]

Biggest concern I have is that you seem to be reinventing the wheel for menus
with StatusIcon::SetContextMenu(vector<string>).  

We already have the MenuModel interface and SimpleMenuModel which can be
converted into a menu on all platforms.  Wouldn't it be better to have your new
method be StatusIcon::SetContextMenu(MenuModel*) ?

http://codereview.chromium.org/3189003/diff/5001/6003
File chrome/browser/background_mode_manager_unittest.cc (right):

http://codereview.chromium.org/3189003/diff/5001/6003#newcode48
chrome/browser/background_mode_manager_unittest.cc:48: >>>>>>> trunk
definitely not what you want here.

[Andrew T Wilson (Slow), 2010-08-19 21:26:21]

OK, ready for another look.

On 2010/08/19 16:36:29, John Gregg wrote:
>  Wouldn't it be better to have your new
> method be StatusIcon::SetContextMenu(MenuModel*) ?
>
Good point. I've refactored the code as you suggest - this has the nice result
of also addressing tfarina's comment, since the MenuModel code now lives out in
a common place. Much cleaner now - thanks!

> http://codereview.chromium.org/3189003/diff/5001/6003
> File chrome/browser/background_mode_manager_unittest.cc (right):
> 
> http://codereview.chromium.org/3189003/diff/5001/6003#newcode48
> chrome/browser/background_mode_manager_unittest.cc:48: >>>>>>> trunk
> definitely not what you want here.

Yeah, I caught that locally but apparently forgot to upload the latest patch.

[John Gregg, 2010-08-19 21:41:30]

http://codereview.chromium.org/3189003/diff/1038/15001
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/3189003/diff/1038/15001#newcode191
chrome/browser/background_mode_manager.cc:191: // items, be sure to update the
associated kExitContextMenuItem, etc.
comment out of sync with code?

http://codereview.chromium.org/3189003/diff/1038/15001#newcode236
chrome/browser/background_mode_manager.cc:236: }
can OpenEmptyWindow ever fail? if(browser) or DCHECK(browser) might be nice here

http://codereview.chromium.org/3189003/diff/1038/15012
File chrome/browser/views/status_icons/status_icon_win.h (right):

http://codereview.chromium.org/3189003/diff/1038/15012#newcode39
chrome/browser/views/status_icons/status_icon_win.h:39: void
HandleClickEvent(int x, int y, bool pass_to_observer);
from an interface perspective, why does the caller of HandleClickEvent know
about observers?  

i would suggest changing this method to be (x, y, is_left_button) or something
equivalent that indicates just the click information, and have the status icon
contain the logic about what should happen w.r.t observers and context menus.

[Andrew T Wilson (Slow), 2010-08-19 23:53:57]

OK, addressed your comments. Please take another look.

http://codereview.chromium.org/3189003/diff/1038/15001
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/3189003/diff/1038/15001#newcode191
chrome/browser/background_mode_manager.cc:191: // items, be sure to update the
associated kExitContextMenuItem, etc.
On 2010/08/19 21:41:31, John Gregg wrote:
> comment out of sync with code?

Done.

http://codereview.chromium.org/3189003/diff/1038/15001#newcode236
chrome/browser/background_mode_manager.cc:236: }
On 2010/08/19 21:41:31, John Gregg wrote:
> can OpenEmptyWindow ever fail? if(browser) or DCHECK(browser) might be nice
here
It cannot fail.

http://codereview.chromium.org/3189003/diff/1038/15012
File chrome/browser/views/status_icons/status_icon_win.h (right):

http://codereview.chromium.org/3189003/diff/1038/15012#newcode39
chrome/browser/views/status_icons/status_icon_win.h:39: void
HandleClickEvent(int x, int y, bool pass_to_observer);
On 2010/08/19 21:41:31, John Gregg wrote:
> from an interface perspective, why does the caller of HandleClickEvent know
> about observers?  
> 
> i would suggest changing this method to be (x, y, is_left_button) or something
> equivalent that indicates just the click information, and have the status icon
> contain the logic about what should happen w.r.t observers and context menus.

Renamed this. Agreed that there's no need to expose observer behavior explicitly
in this call.

[John Gregg, 2010-08-20 00:10:00]

One more thing I noticed: it might be nice to have the scoped_ptr<MenuModel>
live in the cross-platform part of StatusIcon, so you don't have to duplicate
that in each subclass.  You could use a protected virtual method to delegate the
platform-specific behavior when the menu is changed. 

The current effect that I was noticing is that if the BackgroundModeManager code
runs on Mac or Linux with those stub functions, the menu pointer will leak.

If you don't want to do that (or don't want to do it now), I would at least put
NOTREACHED() in those stub functions so you don't end up with surprise leaks
later.

With either of those changes this LGTM.