Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(15)

Issue 20245: Port the Menu class to GTK. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 3 months ago by Elliot Glaysher
Modified:
6 years ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Create a menu wrapper class for GTK menus, and use it for the page and app menus.

Patch Set 1 #

Patch Set 2 : Fix obvious problems... #

Total comments: 3

Patch Set 3 : Align menus correctly, and get enable/disable working. #

Total comments: 6

Patch Set 4 : First version where I'd like reviews, please. #

Patch Set 5 : Fix cpplint errors. #

Total comments: 22

Patch Set 6 : GTK specific menu class instead #

Patch Set 7 : Revert files that shouldn't be changed in this new system. #

Patch Set 8 : Fix star placement. #

Total comments: 7

Patch Set 9 : Fixes for estade #

Total comments: 8

Patch Set 10 : Fixes for evanm #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+470 lines, -31 lines) Patch
M chrome/browser/browser.scons View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/gtk/browser_toolbar_view_gtk.h View 1 2 3 4 5 5 chunks +33 lines, -10 lines 2 comments Download
M chrome/browser/gtk/browser_toolbar_view_gtk.cc View 1 2 3 4 5 6 7 8 9 6 chunks +85 lines, -21 lines 0 comments Download
A chrome/browser/gtk/menu_gtk.h View 6 7 1 chunk +61 lines, -0 lines 0 comments Download
A chrome/browser/gtk/menu_gtk.cc View 1 2 3 6 7 8 9 1 chunk +151 lines, -0 lines 1 comment Download
A chrome/browser/gtk/standard_menus.h View 6 7 8 9 1 chunk +44 lines, -0 lines 0 comments Download
A chrome/browser/gtk/standard_menus.cc View 6 7 8 9 1 chunk +94 lines, -0 lines 0 comments Download
Trybot results:
Commit queue not available (can’t edit this change).

Messages

Total messages: 16 (0 generated)
Jo
Your hack to build menu.cc in browser.scons can be fixed by removing the line 214 ...
8 years, 3 months ago (2009-02-11 03:53:32 UTC) #1
Elliot Glaysher
http://codereview.chromium.org/20245/diff/26/1001 File chrome/browser/browser.scons (right): http://codereview.chromium.org/20245/diff/26/1001#newcode806 Line 806: # build it. For now, do this ugly ...
8 years, 3 months ago (2009-02-11 07:23:20 UTC) #2
Elliot Glaysher
I have just realized what you meant and that you are correct. Not being tired ...
8 years, 3 months ago (2009-02-12 02:04:32 UTC) #3
Evan Stade
So are we porting views then? I don't quite see the point of porting the ...
8 years, 3 months ago (2009-02-12 05:00:24 UTC) #4
Elliot Glaysher
I've just uploaded the first version I'd like to get reviews on. (I'll make sure ...
8 years, 3 months ago (2009-02-12 21:50:32 UTC) #5
Evan Martin
http://codereview.chromium.org/20245/diff/1052/1056 File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right): http://codereview.chromium.org/20245/diff/1052/1056#newcode26 Line 26: explicit CustomDrawButton(const std::string& filename); nice catch! http://codereview.chromium.org/20245/diff/1052/1056#newcode247 Line ...
8 years, 3 months ago (2009-02-12 22:13:35 UTC) #6
Evan Stade
http://codereview.chromium.org/20245/diff/1052/1064 File chrome/views/menu.h (right): http://codereview.chromium.org/20245/diff/1052/1064#newcode26 Line 26: // A Note: You can't actually *do* this ...
8 years, 3 months ago (2009-02-12 22:23:12 UTC) #7
Evan Martin
As I was saying in chat, I think what we want to do is: - ...
8 years, 3 months ago (2009-02-12 23:04:36 UTC) #8
Elliot Glaysher
I've redone how the Menus work so the MenuGtk class isn't an implementation of Menu ...
8 years, 3 months ago (2009-02-13 22:45:32 UTC) #9
Evan Stade
I like! LG with the following nits http://codereview.chromium.org/20245/diff/1052/1064 File chrome/views/menu.h (right): http://codereview.chromium.org/20245/diff/1052/1064#newcode26 Line 26: // ...
8 years, 3 months ago (2009-02-13 23:05:21 UTC) #10
Elliot Glaysher
http://codereview.chromium.org/20245/diff/1052/1064 File chrome/views/menu.h (right): http://codereview.chromium.org/20245/diff/1052/1064#newcode26 Line 26: // A Note: You can't actually *do* this ...
8 years, 3 months ago (2009-02-13 23:21:53 UTC) #11
Evan Stade
http://codereview.chromium.org/20245/diff/72/1092 File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right): http://codereview.chromium.org/20245/diff/72/1092#newcode289 Line 289: DCHECK(tag != -1) << "Impossible button click callback."; ...
8 years, 3 months ago (2009-02-13 23:28:46 UTC) #12
Evan Martin
looks good to me http://codereview.chromium.org/20245/diff/1099/1101 File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right): http://codereview.chromium.org/20245/diff/1099/1101#newcode119 Line 119: back_(NULL), FWIW, the default ...
8 years, 3 months ago (2009-02-13 23:37:21 UTC) #13
Elliot Glaysher
http://codereview.chromium.org/20245/diff/1099/1101 File chrome/browser/gtk/browser_toolbar_view_gtk.cc (right): http://codereview.chromium.org/20245/diff/1099/1101#newcode119 Line 119: back_(NULL), On 2009/02/13 23:37:21, Evan Martin wrote: > ...
8 years, 3 months ago (2009-02-17 17:59:27 UTC) #14
Evan Martin
LGTM, minor nits follow http://codereview.chromium.org/20245/diff/1110/1113 File chrome/browser/gtk/browser_toolbar_view_gtk.h (right): http://codereview.chromium.org/20245/diff/1110/1113#newcode55 Line 55: // Gtk callback to ...
8 years, 3 months ago (2009-02-17 19:08:24 UTC) #15
Dean McNamee
8 years, 3 months ago (2009-02-18 12:35:17 UTC) #16
http://codereview.chromium.org/20245/diff/1110/1114
File chrome/browser/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/20245/diff/1110/1114#newcode94
Line 94: /* static */
I've seen this in a few places now.  I don't think we ever use C style /* */
comments.  This should probably be a // static comment to match the rest of our
code.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06