linux_aura: Implement the static part of the dbus menu for Unity.

linux_aura: Implement the static part of the dbus menu for Unity.

In GTK, we get a menu in the Unity bar for free as long as we have a
GtkMenuBar object in our GtkWindow. linux_aura doesn't use gtk widgets
in the interface. So opportunistically dlopen libdbusmenu-glib and
manually construct a hidden DbusmenuMenuItem* object tree.

This only ports the static parts of the menus; it doesn't port the
History menu, which changes based on user actions.

BUG=265560
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215769

[sadrul, 2013-08-01 20:57:45]

I am not really familiar with how the unity thingy works. But the overall
approach looks good. I have left some comments.

Does this code have any effect if someone running on ubuntu is not running
unity?

https://codereview.chromium.org/21187006/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/browser_desktop_root_window_host_x11.cc
(right):

https://codereview.chromium.org/21187006/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/browser_desktop_root_window_host_x11.cc:43: //    
views::DesktopRootWindowHostX implementation:
X11

https://codereview.chromium.org/21187006/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/global_menu_bar_x11.cc (right):

https://codereview.chromium.org/21187006/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:189: if (attempted_load)
attempted_load could be a static bool inside this function

https://codereview.chromium.org/21187006/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:233: EnsureMethodsLoaded();
Should this happen in InitServer() instead?

https://codereview.chromium.org/21187006/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:342: if (command_id ==
MENU_DISABLED_LABEL) {
The menus are disabled afterwards in InitServer appropriately. Is this
necessary?

https://codereview.chromium.org/21187006/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/global_menu_bar_x11.h (right):

https://codereview.chromium.org/21187006/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/global_menu_bar_x11.h:41: explicit
GlobalMenuBarX11(BrowserView* browser_view,
Don't need explicit

https://codereview.chromium.org/21187006/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/global_menu_bar_x11.h:57: void
BuildMenuFrom(DbusmenuMenuitem* parent,
Doc

https://codereview.chromium.org/21187006/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/global_menu_bar_x11.h:62: DbusmenuMenuitem*
BuildMenuItem(
Doc

[Elliot Glaysher, 2013-08-01 21:27:18]

In general, I appear to be the first person other than the original Unity team
to have written something that interfaces with the lower level libdbusmenu-glib
library. (The Gtk, Qt and Firefox implementations were all done by Canonical.)
So basically nobody has enough knowledge to give a deep review of this patch.

I've tested this on XFCE too. There's no effect on startup.

https://codereview.chromium.org/21187006/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/global_menu_bar_x11.cc (right):

https://codereview.chromium.org/21187006/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:233: EnsureMethodsLoaded();
On 2013/08/01 20:57:45, sadrul wrote:
> Should this happen in InitServer() instead?

No. We will only get InitServer() in response to a window mapped message. And we
only want to subscribe to window mapped messages if we can handle them.
(Otherwise we'll fast path through the empty observer dispatch.)

https://codereview.chromium.org/21187006/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:342: if (command_id ==
MENU_DISABLED_LABEL) {
On 2013/08/01 20:57:45, sadrul wrote:
> The menus are disabled afterwards in InitServer appropriately. Is this
> necessary?

The set of items disabled here and the set of items disabled in InitServer do
not overlap. This disables non-functional labels in menus (which are used in the
history menu). The items in InitServer are normal command_ids.

(Also, this is a fairly straightforward translation of the code in
c/b/u/gtk/global_menu_bar.cc.)

[sadrul, 2013-08-02 02:56:36]

LGTM

[commit-bot: I haz the power, 2013-08-02 17:11:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/21187006/30001

[commit-bot: I haz the power, 2013-08-02 17:27:20]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Elliot Glaysher, 2013-08-02 17:36:29]

+sky for owners c/b/u/v/frame/

[Elliot Glaysher, 2013-08-05 20:11:04]

On 2013/08/02 17:36:29, Elliot Glaysher wrote:
> +sky for owners c/b/u/v/frame/

pingity ping, sky!

[sky, 2013-08-05 20:19:41]

LGTM

https://codereview.chromium.org/21187006/diff/30001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/global_menu_bar_x11.cc (right):

https://codereview.chromium.org/21187006/diff/30001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:232: EnsureMethodsLoaded();
member initialize root_item_ too.

[commit-bot: I haz the power, 2013-08-05 20:26:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/21187006/43001

[commit-bot: I haz the power, 2013-08-06 02:48:59]

Change committed as 215769