Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(104)

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

Created:
9 years, 9 months ago by dill
Modified:
9 years, 8 months ago
CC:
chromium-reviews, pam+watch_chromium.org
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 12

Patch Set 2 : "Updates based on code review." #

Total comments: 4

Patch Set 3 : More code review updates. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+92 lines, -4 lines) Patch
M chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.h View 1 3 chunks +8 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.mm View 1 2 chunks +38 lines, -1 line 0 comments Download
M chrome/browser/ui/gtk/menu_gtk.h View 1 3 chunks +8 lines, -2 lines 0 comments Download
M chrome/browser/ui/gtk/menu_gtk.cc View 1 2 4 chunks +17 lines, -0 lines 1 comment Download
M views/controls/menu/native_menu_win.h View 1 2 chunks +5 lines, -1 line 0 comments Download
M views/controls/menu/native_menu_win.cc View 1 2 3 chunks +16 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
dill
9 years, 9 months ago (2011-03-23 22:31:41 UTC) #1
sky
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): ...
9 years, 9 months ago (2011-03-24 22:39:29 UTC) #2
Avi (use Gerrit)
http://codereview.chromium.org/6732007/diff/1/chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.h File chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.h (right): http://codereview.chromium.org/6732007/diff/1/chrome/browser/ui/cocoa/toolbar/back_forward_menu_controller.h#newcode47 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/back_forward_menu_controller.mm ...
9 years, 9 months ago (2011-03-25 00:19:35 UTC) #3
Evan Stade
why couldn't you just use IsItemDynamicAt
9 years, 9 months ago (2011-03-25 00:30:38 UTC) #4
dill
http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu_win.cc File views/controls/menu/native_menu_win.cc (right): http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu_win.cc#newcode314 views/controls/menu/native_menu_win.cc:314: model_->SetMenuModelDelegate(this); I should do model_->SetMenuModelDelegate(NULL); in the deconstructor, correct?
9 years, 9 months ago (2011-03-25 01:47:46 UTC) #5
sky
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_win.cc > File ...
9 years, 9 months ago (2011-03-25 15:23:51 UTC) #6
dill
http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu_win.cc File views/controls/menu/native_menu_win.cc (right): http://codereview.chromium.org/6732007/diff/1/views/controls/menu/native_menu_win.cc#newcode314 views/controls/menu/native_menu_win.cc:314: model_->SetMenuModelDelegate(this); Ah, well that sounded good. But model_ outlives ...
9 years, 9 months ago (2011-03-25 19:06:56 UTC) #7
dill
9 years, 8 months ago (2011-04-04 19:41:09 UTC) #8
sky
http://codereview.chromium.org/6732007/diff/9001/views/controls/menu/native_menu_win.cc File views/controls/menu/native_menu_win.cc (right): http://codereview.chromium.org/6732007/diff/9001/views/controls/menu/native_menu_win.cc#newcode315 views/controls/menu/native_menu_win.cc:315: model_->SetMenuModelDelegate(this); Move this to RUnMenuAt.
9 years, 8 months ago (2011-04-05 22:40:48 UTC) #9
Evan Stade
again, I don't like the delegate class. Do we really need a model, a model ...
9 years, 8 months ago (2011-04-06 17:58:10 UTC) #10
sky
On Wed, Apr 6, 2011 at 10:58 AM, <estade@chromium.org> wrote: > again, I don't like ...
9 years, 8 months ago (2011-04-06 18:24:14 UTC) #11
Evan Stade
ok http://codereview.chromium.org/6732007/diff/9001/chrome/browser/ui/gtk/menu_gtk.cc File chrome/browser/ui/gtk/menu_gtk.cc (right): http://codereview.chromium.org/6732007/diff/9001/chrome/browser/ui/gtk/menu_gtk.cc#newcode405 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_); ...
9 years, 8 months ago (2011-04-07 18:25:00 UTC) #12
dill
http://codereview.chromium.org/6732007/diff/9001/chrome/browser/ui/gtk/menu_gtk.cc File chrome/browser/ui/gtk/menu_gtk.cc (right): http://codereview.chromium.org/6732007/diff/9001/chrome/browser/ui/gtk/menu_gtk.cc#newcode405 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_menu_win.cc File views/controls/menu/native_menu_win.cc ...
9 years, 8 months ago (2011-04-08 15:48:46 UTC) #13
dill
Updated patch.
9 years, 8 months ago (2011-04-08 15:54:33 UTC) #14
sky
Windows LGTM
9 years, 8 months ago (2011-04-08 16:00:23 UTC) #15
Evan Stade
LGTM with the one change http://codereview.chromium.org/6732007/diff/19001/chrome/browser/ui/gtk/menu_gtk.cc File chrome/browser/ui/gtk/menu_gtk.cc (right): http://codereview.chromium.org/6732007/diff/19001/chrome/browser/ui/gtk/menu_gtk.cc#newcode409 chrome/browser/ui/gtk/menu_gtk.cc:409: ui::MenuModel* model = ModelForMenuItem(GTK_MENU_ITEM(menu_item)); ...
9 years, 8 months ago (2011-04-08 16:45:19 UTC) #16
Evan Stade
wait... I don't think this properly supports submenus. Wouldn't you need to add a MenuModel* ...
9 years, 8 months ago (2011-04-08 16:50:53 UTC) #17
dill
It doesn't support submenus - BackForwardMenuModel doesn't either. This is OK for the Mac version ...
9 years, 8 months ago (2011-04-24 04:11:33 UTC) #18
sky
9 years, 8 months ago (2011-04-25 14:00:08 UTC) #19
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/
>
>

Powered by Google App Engine
This is Rietveld 408576698