Native menu implementation for bug 5679. Followup to http://codereview.chromium.org/2928005/

Native menu implementation for bug 5679. Followup to http://codereview.chromium.org/2928005/


BUG=5679
TEST=Restore a tab with a navigation history, check that correct favicons are displayed in back / forwward menus.

[sky, 2011-03-24 22:39:29]

I've also added OWNERS for the Mac and Linux sides.

  -Scott

http://codereview.chromium.org/6732007/diff/1/chrome/browser/ui/gtk/menu_gtk.h
File chrome/browser/ui/gtk/menu_gtk.h (right):

http://codereview.chromium.org/6732007/diff/1/chrome/browser/ui/gtk/menu_gtk....
chrome/browser/ui/gtk/menu_gtk.h:105: static void UpdateMenuIcon(GtkWidget*
widget, gpointer userdata);
Why is this public?

http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu...
File views/controls/menu/native_menu_win.cc (right):

http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_win.cc:314: model_->SetMenuModelDelegate(this);
I don't see you ever removing the delegate.

http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu...
File views/controls/menu/native_menu_win.h (right):

http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_win.h:38: // MenuModel:Delegate called when a
favicon is loaded from history.
Newline between 37/38. Also, add OVERRIDE.

[Avi (use Gerrit), 2011-03-25 00:19:35]

http://codereview.chromium.org/6732007/diff/1/chrome/browser/ui/cocoa/toolbar...
File chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.h (right):

http://codereview.chromium.org/6732007/diff/1/chrome/browser/ui/cocoa/toolbar...
chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.h:47: -
(void)onIconChanged:(int) modelIndex;
No space after the ).

http://codereview.chromium.org/6732007/diff/1/chrome/browser/ui/cocoa/toolbar...
File chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.mm (right):

http://codereview.chromium.org/6732007/diff/1/chrome/browser/ui/cocoa/toolbar...
chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.mm:32: virtual void
OnIconChanged(int modelIndex) {
OVERRIDE

http://codereview.chromium.org/6732007/diff/1/chrome/browser/ui/cocoa/toolbar...
chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.mm:65: -
(void)onIconChanged:(int) modelIndex {
No space after ).

http://codereview.chromium.org/6732007/diff/1/chrome/browser/ui/cocoa/toolbar...
chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.mm:68:
[backForwardMenu_ itemAtIndex:(NSInteger)modelIndex + 1];
Why the cast? Drop the "(NSInteger)".

http://codereview.chromium.org/6732007/diff/1/chrome/browser/ui/cocoa/toolbar...
chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.mm:70: // Icon (if
it has one).
Comment is obvious; drop.

http://codereview.chromium.org/6732007/diff/1/chrome/browser/ui/gtk/menu_gtk.h
File chrome/browser/ui/gtk/menu_gtk.h (right):

http://codereview.chromium.org/6732007/diff/1/chrome/browser/ui/gtk/menu_gtk....
chrome/browser/ui/gtk/menu_gtk.h:102: virtual void OnIconChanged(int
model_index);
OVERRIDE

http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu...
File views/controls/menu/native_menu_win.cc (right):

http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_win.cc:314: model_->SetMenuModelDelegate(this);
On 2011/03/24 22:39:29, sky wrote:
> I don't see you ever removing the delegate.

Good point. Ditto for all implementations.

[Evan Stade, 2011-03-25 00:30:38]

why couldn't you just use IsItemDynamicAt

[dill, 2011-03-25 01:47:46]

http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu...
File views/controls/menu/native_menu_win.cc (right):

http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_win.cc:314: model_->SetMenuModelDelegate(this);
I should do model_->SetMenuModelDelegate(NULL); in the deconstructor, correct?

[sky, 2011-03-25 15:23:51]

On Thu, Mar 24, 2011 at 6:47 PM,  <dill.sellars@gmail.com> wrote:
>
>
http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu...
> File views/controls/menu/native_menu_win.cc (right):
>
>
http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu...
> views/controls/menu/native_menu_win.cc:314:
> model_->SetMenuModelDelegate(this);
> I should do model_->SetMenuModelDelegate(NULL); in the deconstructor,
> correct?

Yes.

  -Scott

[dill, 2011-03-25 19:06:56]

http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu...
File views/controls/menu/native_menu_win.cc (right):

http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu...
views/controls/menu/native_menu_win.cc:314: model_->SetMenuModelDelegate(this);
Ah, well that sounded good. But model_ outlives the native menu implementation,
so the next instantiation of the native menu from the scoped_ptr calls the
constructor for the new menu then calls the deconstructor of the old menu and
that sets the delegate to null for the new native menu. 

In windows I am removing the delegate at the end of RunMenuAt which blocks at
TrackPopupMenuEx until the menu closes. For Linux, the only way to do it seems
to be in OnMenuHidden which is called when the menu is dismissed or and item is
selected. Not sure about Mac yet.

[sky, 2011-04-05 22:40:48]

http://codereview.chromium.org/6732007/diff/9001/views/controls/menu/native_m...
File views/controls/menu/native_menu_win.cc (right):

http://codereview.chromium.org/6732007/diff/9001/views/controls/menu/native_m...
views/controls/menu/native_menu_win.cc:315: model_->SetMenuModelDelegate(this);
Move this to RUnMenuAt.

[Evan Stade, 2011-04-06 17:58:10]

again, I don't like the delegate class. Do we really need a model, a model
delegate, a view, and a view delegate?? IsItemDynamicAt was created for this
exact purpose.

[sky, 2011-04-06 18:24:14]

On Wed, Apr 6, 2011 at 10:58 AM,  <estade@chromium.org> wrote:
> again, I don't like the delegate class. Do we really need a model, a model
> delegate, a view, and a view delegate?? IsItemDynamicAt was created for this
> exact purpose.

You can never have enough delegates:) Well, ok, maybe you can.

Anyway. It's my understanding IsItemDynamicAt is meant to indicate
every time you show the menu you need to update any dynamic items. The
view delegate is meant to cover the case where the icon changes while
the menu is shown. This situation occurs with session restore when the
NavigationEntrys don't yet have the favicon. The menu is shown, which
triggers fetching the icons if we don't yet have them.

  -Scott

[Evan Stade, 2011-04-07 18:25:00]

ok

http://codereview.chromium.org/6732007/diff/9001/chrome/browser/ui/gtk/menu_g...
File chrome/browser/ui/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/6732007/diff/9001/chrome/browser/ui/gtk/menu_g...
chrome/browser/ui/gtk/menu_gtk.cc:405:
gtk_container_foreach(GTK_CONTAINER(menu_), UpdateMenuIcon, &model_index);
Try:

GList* items = gtk_container_get_children(menu_);
GtkWidget* menu_item = GTK_WIDGET(g_list_nth(items, model_index)->data);
g_list_free(items);
...

[dill, 2011-04-08 15:48:46]

http://codereview.chromium.org/6732007/diff/9001/chrome/browser/ui/gtk/menu_g...
File chrome/browser/ui/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/6732007/diff/9001/chrome/browser/ui/gtk/menu_g...
chrome/browser/ui/gtk/menu_gtk.cc:405:
gtk_container_foreach(GTK_CONTAINER(menu_), UpdateMenuIcon, &model_index);
Thanks, much cleaner.

http://codereview.chromium.org/6732007/diff/9001/views/controls/menu/native_m...
File views/controls/menu/native_menu_win.cc (right):

http://codereview.chromium.org/6732007/diff/9001/views/controls/menu/native_m...
views/controls/menu/native_menu_win.cc:315: model_->SetMenuModelDelegate(this);
Done.

[dill, 2011-04-08 15:54:33]

Updated patch.

[sky, 2011-04-08 16:00:23]

Windows LGTM

[Evan Stade, 2011-04-08 16:45:19]

LGTM with the one change

http://codereview.chromium.org/6732007/diff/19001/chrome/browser/ui/gtk/menu_...
File chrome/browser/ui/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/6732007/diff/19001/chrome/browser/ui/gtk/menu_...
chrome/browser/ui/gtk/menu_gtk.cc:409: ui::MenuModel* model =
ModelForMenuItem(GTK_MENU_ITEM(menu_item));
I'd just use model_

[Evan Stade, 2011-04-08 16:50:53]

wait... I don't think this properly supports submenus. Wouldn't you need to add
a MenuModel* param to OnIconChanged as well as registering the view as the
delegate for every submenu model?

[dill, 2011-04-24 04:11:33]

It doesn't support submenus - BackForwardMenuModel doesn't either. This is
OK for the Mac version since BackForwardMenuController doesn't handle
submenus, but the Windows and Linux native menus do as they are more
generic. The support for changing icons while the menu is open is specific
to BackForwardMenuModel at this point, so it could be left out without any
functional problems. Let me know if I need to add support for submenus
though in this patch.

On Fri, Apr 8, 2011 at 9:50 AM, <estade@chromium.org> wrote:

> wait... I don't think this properly supports submenus. Wouldn't you need to
> add
> a MenuModel* param to OnIconChanged as well as registering the view as the
> delegate for every submenu model?
>
>
> http://codereview.chromium.org/6732007/
>

[sky, 2011-04-25 14:00:08]

You should be complete and support submenus too.

  -Scott

On Sat, Apr 23, 2011 at 9:11 PM, Dillon Sellars <dill.sellars@gmail.com> wrote:
> It doesn't support submenus - BackForwardMenuModel doesn't either. This is
> OK for the Mac version since BackForwardMenuController doesn't handle
> submenus, but the Windows and Linux native menus do as they are more
> generic. The support for changing icons while the menu is open is specific
> to BackForwardMenuModel at this point, so it could be left out without any
> functional problems. Let me know if I need to add support for submenus
> though in this patch.
>
> On Fri, Apr 8, 2011 at 9:50 AM, <estade@chromium.org> wrote:
>>
>> wait... I don't think this properly supports submenus. Wouldn't you need
>> to add
>> a MenuModel* param to OnIconChanged as well as registering the view as the
>> delegate for every submenu model?
>>
>> http://codereview.chromium.org/6732007/
>
>