GTK: Implement the global bookmarks menu.

GTK: Implement the global bookmarks menu.

BUG=30213
TEST=The bookmarks menu under Unity is populated with the user's bookmarks.

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

[Evan Stade, 2011-05-10 17:09:18]

mostly nits

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
File chrome/browser/ui/gtk/global_bookmarks_menu.cc (right):

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_bookmarks_menu.cc:74: // Only display the other
bookmarks folder in the menu if it has items in it.
wouldn't this make it impossible to add thing #1 to "other bookmarks"? The bar
displays "other bookmarks" regardless of whether it contains anything, what's
different about the global menu

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_bookmarks_menu.cc:178: void* /*unused*/) {
I don't think this is within style guidelines

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_bookmarks_menu.cc:210: RebuildMenuInFuture();
when you get a batch of NodeMoved/Added calls, aren't they at least synchronous?
meaning you wouldn't need to set a timeout, you could just PostTask

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
File chrome/browser/ui/gtk/global_bookmarks_menu.h (right):

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_bookmarks_menu.h:23: // Manages the global
Bookmarks menu.
odd to capitalize bookmarks here

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_bookmarks_menu.h:25: // There are a few subtilties
here: we can't rely on accurate event
subtleties

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_bookmarks_menu.h:26: // dispositions being sent
back, right click menus on menu items were placed
confusing change of tense here ("were" -- when?)

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_bookmarks_menu.h:28: // need to update the menu in
the background (instead of building it on showing
can you separate some of these comments into a TODO section if you plan to fix
them

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_bookmarks_menu.h:30: // making these dragable since
these items aren't displayed in our process.
draggable

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_bookmarks_menu.h:31: class GlobalBookmarksMenu :
public NotificationObserver,
this should be GlobalBookmarkMenu (the only class name I can see which diverges
from the singular form for bookmark is BookmarksTabHelper, which I also think is
in error)

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_bookmarks_menu.h:42: // rebuilds the menu during
their pre-show callback. We don't have that
s/that//

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_bookmarks_menu.h:44: // has already been displayed
and its multiple dbus calls to add the menu
s/its/it takes/

(?)

[Elliot Glaysher, 2011-05-10 18:29:28]

TAL?

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
File chrome/browser/ui/gtk/global_bookmarks_menu.cc (right):

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_bookmarks_menu.cc:74: // Only display the other
bookmarks folder in the menu if it has items in it.
On 2011/05/10 17:09:18, Evan Stade wrote:
> wouldn't this make it impossible to add thing #1 to "other bookmarks"? The bar
> displays "other bookmarks" regardless of whether it contains anything, what's
> different about the global menu 

"Other Bookmarks" won't be shown when empty for long. Also for M13:
crbug.com/64997 . 

These menus can't be drag targets anyway so I'm not sure why it'd be impossible
to add things to other bookmarks...

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
File chrome/browser/ui/gtk/global_bookmarks_menu.h (right):

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_bookmarks_menu.h:28: // need to update the menu in
the background (instead of building it on showing
On 2011/05/10 17:09:18, Evan Stade wrote:
> can you separate some of these comments into a TODO section if you plan to fix
> them

I put these here to document the limitations. I don't really plan to address any
of this because it's out of scope. These menus should act as vanilla as
possible.

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_bookmarks_menu.h:31: class GlobalBookmarksMenu :
public NotificationObserver,
On 2011/05/10 17:09:18, Evan Stade wrote:
> this should be GlobalBookmarkMenu (the only class name I can see which
diverges
> from the singular form for bookmark is BookmarksTabHelper, which I also think
is
> in error)

The class implements a menu that whose title is the plural form. I'd prefer to
keep it as is so I'm not converting back and forth in my head.

[Evan Stade, 2011-05-10 18:50:34]

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
File chrome/browser/ui/gtk/global_bookmarks_menu.h (right):

http://codereview.chromium.org/6980011/diff/3001/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_bookmarks_menu.h:31: class GlobalBookmarksMenu :
public NotificationObserver,
On 2011/05/10 18:29:28, Elliot Glaysher wrote:
> On 2011/05/10 17:09:18, Evan Stade wrote:
> > this should be GlobalBookmarkMenu (the only class name I can see which
> diverges
> > from the singular form for bookmark is BookmarksTabHelper, which I also
think
> is
> > in error)
> 
> The class implements a menu that whose title is the plural form. I'd prefer to
> keep it as is so I'm not converting back and forth in my head.

I think consistency with other code is more important than consistency with UI
strings. Otherwise we end up with situations like JavaScript in half the places
and Javascript in the other half, which is super annoying.

[Elliot Glaysher, 2011-05-10 19:45:14]

On 2011/05/10 18:50:34, Evan Stade wrote:
> I think consistency with other code is more important than consistency with UI
> strings. Otherwise we end up with situations like JavaScript in half the
places
> and Javascript in the other half, which is super annoying.

Done

[Evan Stade, 2011-05-10 19:52:53]

lgtm