[Linux] Add a People menu item to the global menu bar.

[Linux] Add a People menu item to the global menu bar.

This menu should look like the Mac one, and have a list of profiles, with a checkmark for the active profile, and Edit/Add Person buttons.

Screenshots: https://drive.google.com/open?id=0B1B1Up4p2NRMVmhCMXFpQUFSVDQ&authuser=1

BUG=404662
Committed: https://crrev.com/ba026c17fc73a953a549e64a2a50c601072234a7
Cr-Commit-Position: refs/heads/master@{#297864}

[noms (inactive), 2014-10-01 19:00:17]

noms@chromium.org changed reviewers:
+ sky@chromium.org

[noms (inactive), 2014-10-01 19:00:17]

Hi Scott,

This CL is adding a "People" menu item to the Linux global menu. Please take a
look. Thanks!

[sky, 2014-10-01 20:06:30]

sky@chromium.org changed reviewers:
+ erg@google.com
- sky@chromium.org

[sky, 2014-10-01 20:06:30]

I'm punting this to erg

[Elliot Glaysher, 2014-10-01 20:27:52]

erg@chromium.org changed reviewers:
+ erg@chromium.org

[Elliot Glaysher, 2014-10-01 20:27:53]

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

https://codereview.chromium.org/617963005/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:652: // should be disabled.
OK, so I have two windows: one of my normal profile, one with a guest profile. I
just want to verify that this is how this looks from the user point of view
since the menu bar is per window.

1) I have the normal profile active.
2) I click on the profiles menu bar item. Things aren't disabled and there's a
check mark.
3) I switch to the guest mode window.
4) I click on the profiles menu bar item. Things are disabled and there's no
check mark on any of the menu items.

https://codereview.chromium.org/617963005/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:888: int id =
GPOINTER_TO_INT(g_object_get_data(G_OBJECT(sender), "profile-id"));
kProfileId?

https://codereview.chromium.org/617963005/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:894: int id =
GPOINTER_TO_INT(g_object_get_data(G_OBJECT(sender), "profile-id"));
kProfileId?

[noms (inactive), 2014-10-01 20:34:14]

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

https://codereview.chromium.org/617963005/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:652: // should be disabled.
Yup, that's it. The checkmark is "which profile is active from this list of
profiles".

On 2014/10/01 20:27:52, Elliot Glaysher wrote:
> OK, so I have two windows: one of my normal profile, one with a guest profile.
I
> just want to verify that this is how this looks from the user point of view
> since the menu bar is per window.
> 
> 1) I have the normal profile active.
> 2) I click on the profiles menu bar item. Things aren't disabled and there's a
> check mark.
> 3) I switch to the guest mode window.
> 4) I click on the profiles menu bar item. Things are disabled and there's no
> check mark on any of the menu items.

https://codereview.chromium.org/617963005/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:888: int id =
GPOINTER_TO_INT(g_object_get_data(G_OBJECT(sender), "profile-id"));
Oh, of course. Done.
On 2014/10/01 20:27:52, Elliot Glaysher wrote:
> kProfileId?

https://codereview.chromium.org/617963005/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:894: int id =
GPOINTER_TO_INT(g_object_get_data(G_OBJECT(sender), "profile-id"));
On 2014/10/01 20:27:52, Elliot Glaysher wrote:
> kProfileId?

Done.

[Elliot Glaysher, 2014-10-01 20:35:52]

lgtm

[noms (inactive), 2014-10-01 20:37:23]

noms@chromium.org changed reviewers:
+ sky@chromium.org

[noms (inactive), 2014-10-01 20:37:23]

Thanks Elliot!

Scott: adding you back for an owners stamp.

[noms (inactive), 2014-10-01 20:54:12]

Elliot: I made a small change and moved the instantiation of the avatar menu to
a bit later (when the ProfileInfoCache is actually available). Is it still ok?

[Elliot Glaysher, 2014-10-01 21:08:48]

On 2014/10/01 20:54:12, Monica Dinculescu wrote:
> Elliot: I made a small change and moved the instantiation of the avatar menu
to
> a bit later (when the ProfileInfoCache is actually available). Is it still ok?

Mmmaaaybbeeee. What happens if the profile menu is brought up before the profile
information is loaded?

(This doesn't happen with the history menu since the history menu is generated
synchronously in response to the about-to-show signal.)

[noms (inactive), 2014-10-01 21:12:19]

Hmmm is that even possible? InitServer should be called as soon as you have a
Window (I think, but haven't fully checked, that the previous problem was that
the GlobalMenuBar constructor is before we have a
ProfileManager/ProfileInfoCache available in tests). Would that make sense?

[Elliot Glaysher, 2014-10-01 21:17:44]

On 2014/10/01 21:12:19, Monica Dinculescu wrote:
> Hmmm is that even possible? InitServer should be called as soon as you have a
> Window (I think, but haven't fully checked, that the previous problem was that
> the GlobalMenuBar constructor is before we have a
> ProfileManager/ProfileInfoCache available in tests). Would that make sense?

I don't know. Are you guaranteed to have the cache before you create the window?
The profile, yes, but the cache, seems to be a different thing and I don't know
enough about how that works to say.

[sky, 2014-10-01 23:28:37]

Can you update OWNERS with erg as owner for *x11 in this directory?

[noms (inactive), 2014-10-02 15:09:16]

Done! Added Elliot as an owner.

On 2014/10/01 23:28:37, sky wrote:
> Can you update OWNERS with erg as owner for *x11 in this directory?

[noms (inactive), 2014-10-02 15:31:23]

On 2014/10/01 21:17:44, Elliot Glaysher wrote:
> On 2014/10/01 21:12:19, Monica Dinculescu wrote:
> > Hmmm is that even possible? InitServer should be called as soon as you have
a
> > Window (I think, but haven't fully checked, that the previous problem was
that
> > the GlobalMenuBar constructor is before we have a
> > ProfileManager/ProfileInfoCache available in tests). Would that make sense?
> 
> I don't know. Are you guaranteed to have the cache before you create the
window?
> The profile, yes, but the cache, seems to be a different thing and I don't
know
> enough about how that works to say.

I did some investigating. What happens is that in tests:
- profile_manager() isn't available (i.e. it's null) in the constructor, but is
available in InitServer
- if the ProfileInfoCache doesn't exist, it gets recreated when it's needed, so
that part doesn't matter

The ProfileManager gets instantiated the first time profile_manager() is called
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...)

and that's very early in the chrome_browser_main code/browser_process_impl code.
I don't know
why this makes a difference in the unit tests, but I can't repro it on a normal
run of Chrome. 

I don't think that it's a big problem if I move the avatar_menu 
instantiation to InitServer instead of the constructor. From the comments and
the code, InitServer
is called after the x11 has allocated resources for the window, and it's when we
start actually building
the menu anyway (the profile part of that is all synchronous), so I don't think
this would actually
make a difference from a user's perspective (i.e. seeing an empty menu). 

Do you want me to add an if-check to only create this profiles menu if, when we
need it, 
profile_manager() is non-null, or is that being too careful?

[sky, 2014-10-02 15:35:58]

LGTM

[Elliot Glaysher, 2014-10-02 17:30:58]

lgtm++

I think just putting a DCHECK for that condition for documentation purposes
would be sufficient.

[noms (inactive), 2014-10-02 17:41:09]

On 2014/10/02 17:30:58, Elliot Glaysher wrote:
> lgtm++
> 
> I think just putting a DCHECK for that condition for documentation purposes
> would be sufficient.

Done! Thanks!

[noms (inactive), 2014-10-02 17:41:18]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-10-02 17:42:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/617963005/80001

[commit-bot: I haz the power, 2014-10-02 18:09:54]

Committed patchset #5 (id:80001) as c65c362c3c87f62faac8199727babf9f747e2c28

[commit-bot: I haz the power, 2014-10-02 18:10:31]

Patchset 5 (id:??) landed as
https://crrev.com/ba026c17fc73a953a549e64a2a50c601072234a7
Cr-Commit-Position: refs/heads/master@{#297864}