Add a main menu to the shim that matches the one in Chrome.

Add a main menu to the shim that matches the one in Chrome.

Currently the shim has no main menu. We construct one by adding a first
item (which gets replaced by OSX with the app's title) and hiding it.
Then a second item is added that will be used as the main menu.

BUG=168080
TEST=Start an app and close its window.
Cmd+Tab to the shim.
The main menu bar should have one unbolded item with the name of the app.

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

[tapted, 2013-07-17 07:48:24]

https://codereview.chromium.org/19490002/diff/1/apps/app_shim/chrome_main_app...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/19490002/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:139: base::scoped_nsobject<NSMenu>
mainMenu([[NSMenu alloc] initWithTitle:title]);
nit: mainMenu -> main_menu, since we're in a C++ class

https://codereview.chromium.org/19490002/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:140: for (int i = 0; i < 2; ++i) {
Not sure you need the loop.. can you explain the approach some more. E.g. are
you replacing the whole [NSApp mainMenu] to avoid a conflict with the
keyEquivalent on the existing Quit item?

E.g. does something like this work, or are items without submenus skipped in the
mainMenu somehow:

// Put a dummy item at index 0, and hide it.
[[main_menu addItemWithTitle:title
                      action:nil
               keyEquivalent:@""] setHidden:YES];

// Construct an unbolded app menu, to match how it appears with Chrome active.
base::scoped_nsobject<NSMenu> submenu([[NSMenu alloc] initWithTitle:title]);
[submenu addItemWithTitle:[quit stringByAppendingString:title]
                   action:@selector(terminate:)
            keyEquivalent:@"q"];
NSMenuItem* app_name_item = [main_menu addItemWithTitle:title
                                                 action:nil
                                          keyEquivalent:@""];
[app_name_item setSubmenu:submenu];
[NSApp setMainMenu:mainMenu];


And.. is it worth adding Quit at all? i.e. Should the user ever see it? And,
when they do try to use it, is it expected to work, or will it always freeze
(since otherwise it would have focussed Chrome).

https://codereview.chromium.org/19490002/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:153: [[NSMenuItem alloc]
initWithTitle:[@"Quit " stringByAppendingString:title]
I think you need localization on the string "Quit"

[jackhou1, 2013-07-22 00:51:46]

https://codereview.chromium.org/19490002/diff/1/apps/app_shim/chrome_main_app...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/19490002/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:139: base::scoped_nsobject<NSMenu>
mainMenu([[NSMenu alloc] initWithTitle:title]);
On 2013/07/17 07:48:24, tapted wrote:
> nit: mainMenu -> main_menu, since we're in a C++ class

Done.

https://codereview.chromium.org/19490002/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:140: for (int i = 0; i < 2; ++i) {
On 2013/07/17 07:48:24, tapted wrote:
> Not sure you need the loop.. can you explain the approach some more. E.g. are
> you replacing the whole [NSApp mainMenu] to avoid a conflict with the
> keyEquivalent on the existing Quit item?
> 
> E.g. does something like this work, or are items without submenus skipped in
the
> mainMenu somehow:
> 
> // Put a dummy item at index 0, and hide it.
> [[main_menu addItemWithTitle:title
>                       action:nil
>                keyEquivalent:@""] setHidden:YES];
> 
> // Construct an unbolded app menu, to match how it appears with Chrome active.
> base::scoped_nsobject<NSMenu> submenu([[NSMenu alloc] initWithTitle:title]);
> [submenu addItemWithTitle:[quit stringByAppendingString:title]
>                    action:@selector(terminate:)
>             keyEquivalent:@"q"];
> NSMenuItem* app_name_item = [main_menu addItemWithTitle:title
>                                                  action:nil
>                                           keyEquivalent:@""];
> [app_name_item setSubmenu:submenu];
> [NSApp setMainMenu:mainMenu];
> 
> 
> And.. is it worth adding Quit at all? i.e. Should the user ever see it? And,
> when they do try to use it, is it expected to work, or will it always freeze
> (since otherwise it would have focussed Chrome).

Done.

Yeah, when all of the app's windows are closed, you can still Cmd+Tab to the app
shim. Chrome does not focus because there are no windows for that app.

https://codereview.chromium.org/19490002/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:153: [[NSMenuItem alloc]
initWithTitle:[@"Quit " stringByAppendingString:title]
On 2013/07/17 07:48:24, tapted wrote:
> I think you need localization on the string "Quit"

This turns out to be harder than I thought because the shim doesn't have access
to Chrome's localized resources. The solution will probably be to add IPC
messages to get UI strings from Chrome. I'll do that in another CL.

[tapted, 2013-07-22 02:35:09]

https://codereview.chromium.org/19490002/diff/1/apps/app_shim/chrome_main_app...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/19490002/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:153: [[NSMenuItem alloc]
initWithTitle:[@"Quit " stringByAppendingString:title]
On 2013/07/22 00:51:46, jackhou1 wrote:
> On 2013/07/17 07:48:24, tapted wrote:
> > I think you need localization on the string "Quit"
> 
> This turns out to be harder than I thought because the shim doesn't have
access
> to Chrome's localized resources. The solution will probably be to add IPC
> messages to get UI strings from Chrome. I'll do that in another CL.

Can you just call

   ResourceBundle::InitSharedInstanceLocaleOnly(
      const std::string& pref_locale, Delegate* delegate);

A NULL delegate might work, otherwise you can probably implement implement a
ResourceBundle::Delegate that implements GetPathForLocalePack, or call one of
the SetOverride*BundlePath functions.

It looks like app_mode_app already depends on the ui library via
infoplist_strings_tool, so you might not even have to add and dependencies (or
you might need a direct dependency to pick up an include path).

https://codereview.chromium.org/19490002/diff/5001/apps/app_shim/chrome_main_...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/19490002/diff/5001/apps/app_shim/chrome_main_...
apps/app_shim/chrome_main_app_mode_mac.mm:146: [dummy_item
setSubmenu:dummy_submenu];
just checking - what happens if you delete this line? Does it cause OSX to
replace the second item in main_menu with a bolded app name insead?

[jackhou1, 2013-07-22 11:27:04]

PTAL. 

Figured out how to get the localized strings.

https://codereview.chromium.org/19490002/diff/1/apps/app_shim/chrome_main_app...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/19490002/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:153: [[NSMenuItem alloc]
initWithTitle:[@"Quit " stringByAppendingString:title]
On 2013/07/22 02:35:09, tapted wrote:
> On 2013/07/22 00:51:46, jackhou1 wrote:
> > On 2013/07/17 07:48:24, tapted wrote:
> > > I think you need localization on the string "Quit"
> > 
> > This turns out to be harder than I thought because the shim doesn't have
> access
> > to Chrome's localized resources. The solution will probably be to add IPC
> > messages to get UI strings from Chrome. I'll do that in another CL.
> 
> Can you just call
> 
>    ResourceBundle::InitSharedInstanceLocaleOnly(
>       const std::string& pref_locale, Delegate* delegate);
> 
> A NULL delegate might work, otherwise you can probably implement implement a
> ResourceBundle::Delegate that implements GetPathForLocalePack, or call one of
> the SetOverride*BundlePath functions.
> 
> It looks like app_mode_app already depends on the ui library via
> infoplist_strings_tool, so you might not even have to add and dependencies (or
> you might need a direct dependency to pick up an include path).

Ah yup, this is necessary. I also needed to add an include to the gypi, and set
the framework bundle path.

https://codereview.chromium.org/19490002/diff/5001/apps/app_shim/chrome_main_...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/19490002/diff/5001/apps/app_shim/chrome_main_...
apps/app_shim/chrome_main_app_mode_mac.mm:146: [dummy_item
setSubmenu:dummy_submenu];
On 2013/07/22 02:35:09, tapted wrote:
> just checking - what happens if you delete this line? Does it cause OSX to
> replace the second item in main_menu with a bolded app name insead?

The first item is still replaced, but it doesn't get hidden, even after
[dummy_item setHidden:YES].

[tapted, 2013-07-23 04:51:16]

+benwells for an opinion on the DEPS changes. Some things that crossed my mind:
 - would chrome/common/* ever belong in apps/DEPS ?
 - should we make an apps/app_shim/DEPS, that isolates some the deps that are
coming in for ui-things
 - do we need an apps/ui/ ?

[tapted, 2013-07-23 05:24:34]

https://codereview.chromium.org/19490002/diff/10001/apps/DEPS
File apps/DEPS (right):

https://codereview.chromium.org/19490002/diff/10001/apps/DEPS#newcode35
apps/DEPS:35: "+grit/generated_resources.h",
How does everything else avoid adding this to their DEPS? i.e., there doesn't
appear to be precedent for 'grit' appearing in a DEPS file, according to `git
grep grit -- DEPS`

https://codereview.chromium.org/19490002/diff/10001/apps/app_shim/chrome_main...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/19490002/diff/10001/apps/app_shim/chrome_main...
apps/app_shim/chrome_main_app_mode_mac.mm:414: // [base::mac::OuterBundle()
preferredLocalizations] seems to be equivalent to
'seems to be' -> 'is' ? (i.e. we should make sure this is the explanation)

And, I'm not too clear on why them being equivalent implies that we need to
calculate it separately. (i.e. my first thought is that it would mean we could
use either, rather than neither).

[jackhou1, 2013-07-23 06:22:22]

https://codereview.chromium.org/19490002/diff/10001/apps/DEPS
File apps/DEPS (right):

https://codereview.chromium.org/19490002/diff/10001/apps/DEPS#newcode35
apps/DEPS:35: "+grit/generated_resources.h",
On 2013/07/23 05:24:35, tapted wrote:
> How does everything else avoid adding this to their DEPS? i.e., there doesn't
> appear to be precedent for 'grit' appearing in a DEPS file, according to `git
> grep grit -- DEPS`

They do have it. It shows up on Codesearch:
https://code.google.com/p/chromium/codesearch#search/&q=grit%20file:DEPS

https://codereview.chromium.org/19490002/diff/10001/apps/app_shim/chrome_main...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/19490002/diff/10001/apps/app_shim/chrome_main...
apps/app_shim/chrome_main_app_mode_mac.mm:414: // [base::mac::OuterBundle()
preferredLocalizations] seems to be equivalent to
On 2013/07/23 05:24:35, tapted wrote:
> 'seems to be' -> 'is' ? (i.e. we should make sure this is the explanation)
> 
> And, I'm not too clear on why them being equivalent implies that we need to
> calculate it separately. (i.e. my first thought is that it would mean we could
> use either, rather than neither).

Updated the comment.

[tapted, 2013-07-23 07:39:40]

code change looks good, but it should have a TEST= line

I'm leaning towards suggesting a separate DEPS file under app_shim, just because
chrome_main_app_mode_mac.mm is a bit special, but let's talk to benwells. Maybe
it would make more sense putting that file in a level deeper, like
apps/app_shim/app (too much?), so it can have special deps :/

https://codereview.chromium.org/19490002/diff/10001/apps/DEPS
File apps/DEPS (right):

https://codereview.chromium.org/19490002/diff/10001/apps/DEPS#newcode35
apps/DEPS:35: "+grit/generated_resources.h",
On 2013/07/23 06:22:22, jackhou1 wrote:
> On 2013/07/23 05:24:35, tapted wrote:
> > How does everything else avoid adding this to their DEPS? i.e., there
doesn't
> > appear to be precedent for 'grit' appearing in a DEPS file, according to
`git
> > grep grit -- DEPS`
> 
> They do have it. It shows up on Codesearch:
> https://code.google.com/p/chromium/codesearch#search/&q=grit%2520file:DEPS

ohhh derp. TIL: git grep is not recursive if it finds a file in the current path
matching the pathspec exactly. Seems the trick is `git grep grit -- '*DEPS'`

[benwells, 2013-07-24 01:29:40]

lgtm

[tapted, 2013-07-24 03:01:25]

lgtm if ben's happy with those DEPS

[benwells, 2013-07-24 03:30:16]

On 2013/07/24 03:01:25, tapted wrote:
> lgtm if ben's happy with those DEPS

to be clear, i'm happy. lgtm.

Long term I think app_shims should move to chrome/browser/apps, which should
exist soon.

[jackhou1, 2013-07-24 03:52:20]

thakis, please review adding chrome/common/chrome_constants.h to apps/DEPS

BTW, presubmit said I needed lgtm for adding chrome/common/ to DEPS, is it still
necessary if there are other chrome/common/ dependencies in apps/DEPS?

Also, presubmit also said I needed lgtm for adding grit to DEPS, is that
necessary? If so, who should I ask to review?

[Nico, 2013-07-24 20:34:24]

On 2013/07/24 03:30:16, benwells wrote:
> On 2013/07/24 03:01:25, tapted wrote:
> > lgtm if ben's happy with those DEPS
> 
> to be clear, i'm happy. lgtm.
> 
> Long term I think app_shims should move to chrome/browser/apps, which should
> exist soon.

app_shims run in their own process, so they probably shouldn't be in
chrome/browser (which is for the browser process).

> thakis, please review adding chrome/common/chrome_constants.h to apps/DEPS

benwells needs to do that, not I :-)

No idea about grit, that's up to apps/OWNERS. It means you depend on deps
– alternatively, if you have only few UI strings, you could get them injected
(kind of like ContentClient), then the build of apps/ will be faster and it'll
be less coupled to the rest of chrome. So that's up to apps/OWNERS.

[benwells, 2013-07-25 00:53:08]

On 2013/07/24 20:34:24, Nico wrote:
> On 2013/07/24 03:30:16, benwells wrote:
> > On 2013/07/24 03:01:25, tapted wrote:
> > > lgtm if ben's happy with those DEPS
> > 
> > to be clear, i'm happy. lgtm.
> > 
> > Long term I think app_shims should move to chrome/browser/apps, which should
> > exist soon.
> 
> app_shims run in their own process, so they probably shouldn't be in
> chrome/browser (which is for the browser process).

Some runs in the browser process, some in the shim process, but the point is the
chrome specific stuff should move out of apps/ and into chrome/. Maybe that
needs to be chrome/something-other-than-browser. It might be useful to separate
the stuff that runs in the shim process from the stuff that runs in the browser
process for clarity. Anyways, this is all orthogonal to this change which I
think lgtm as is.

> 
> > thakis, please review adding chrome/common/chrome_constants.h to apps/DEPS
> 
> benwells needs to do that, not I :-)

There is some new rule that API providers need to review new clients of their
code. So if developer X owns API foo, and developer Y wants to add a dependency
into bar to foo, then developer X has to review.

> 
> No idea about grit, that's up to apps/OWNERS. It means you depend on deps
> – alternatively, if you have only few UI strings, you could get them
injected
> (kind of like ContentClient), then the build of apps/ will be faster and it'll
> be less coupled to the rest of chrome. So that's up to apps/OWNERS.

My thinking here is that some of the app shim stuff is chrome specific so should
move out of apps/, which is meant to be generic enough to integrate into any
application which wants to have an app system. E.g. there could be an app_shell
which can run the same apps as chrome but would have different UI and browser
integration.

When that change happens the dependency should go. I can't see any problem
having it now (it is one of a bunch of similar things).

[jackhou1, 2013-07-25 01:20:37]

On 2013/07/24 20:34:24, Nico wrote:
> On 2013/07/24 03:30:16, benwells wrote:
> > On 2013/07/24 03:01:25, tapted wrote:
> > > lgtm if ben's happy with those DEPS
> > 
> > to be clear, i'm happy. lgtm.
> > 
> > Long term I think app_shims should move to chrome/browser/apps, which should
> > exist soon.
> 
> app_shims run in their own process, so they probably shouldn't be in
> chrome/browser (which is for the browser process).
> 
> > thakis, please review adding chrome/common/chrome_constants.h to apps/DEPS
> 
> benwells needs to do that, not I :-)

I thought I needed your lgtm because presubmit said this:

Missing LGTM from OWNERS of directories added to DEPS:
    '+chrome/common',
    '+grit',

But I'll take your word over presubmits =).

> No idea about grit, that's up to apps/OWNERS. It means you depend on deps
> – alternatively, if you have only few UI strings, you could get them
injected
> (kind of like ContentClient), then the build of apps/ will be faster and it'll
> be less coupled to the rest of chrome. So that's up to apps/OWNERS.

Ah interesting. It's only needed by chrome_app_mode_main_mac.mm though, so I
think we should first separate apps/, browser-side app shim handling, and
shim-side code.

[commit-bot: I haz the power, 2013-07-25 01:25:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/19490002/25001

[jackhou1, 2013-07-25 05:38:56]

Ah that didn't work. Looks like it does need a chrome/common/ lgtm. And
apparently also one for grit/ =(

[tapted, 2013-07-25 06:10:21]

The context:

Running presubmit commit checks ...
Running /mnt/scratch0/b_used/build/slave/linux/build/src/PRESUBMIT.py
Running
/mnt/scratch0/b_used/build/slave/linux/build/src/third_party/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from OWNERS of directories added to DEPS:
    '+chrome/common',
    '+grit',

On 2013/07/25 05:38:56, jackhou1 wrote:
> Ah that didn't work. Looks like it does need a chrome/common/ lgtm. And
> apparently also one for grit/ =(

I don't think you can get one for adding grit -- this seems like a bug in the
presubmit (filed http://crbug.com/264120 ). You might need to use your recently
acquired committer powers to dcommit the DEPS change before landing this via the
CQ (because of http://crbug.com/242900 ). But I think thakis has the power to
make the +chrome/common line go away.

[benwells, 2013-07-25 06:10:31]

On 2013/07/25 05:38:56, jackhou1 wrote:
> Ah that didn't work. Looks like it does need a chrome/common/ lgtm. And
> apparently also one for grit/ =(

I think TBR is warranted in this case, as Nico is a chrome/common owner and
seemed OK with this change if I was. But I don't know if grit has owners ...
does git cl suggest anyone?

[commit-bot: I haz the power, 2013-07-25 09:30:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/19490002/25001

[commit-bot: I haz the power, 2013-07-25 12:36:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/19490002/25001

[Nico, 2013-07-25 18:09:49]

joi@ or tony@ are grit owners.

[Nico, 2013-07-25 18:10:31]

chrome/common lgtm

[commit-bot: I haz the power, 2013-07-25 19:21:58]

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

[jackhou1, 2013-07-25 23:20:42]

tony, could you please review apps/DEPS for OWNERS of grit

[tony, 2013-07-25 23:26:12]

DEPS LGTM

[commit-bot: I haz the power, 2013-07-26 03:48:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/19490002/25001

[commit-bot: I haz the power, 2013-07-26 06:38:19]

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

[commit-bot: I haz the power, 2013-07-27 20:45:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/19490002/25001

[commit-bot: I haz the power, 2013-07-27 20:57:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/19490002/25001

[commit-bot: I haz the power, 2013-07-28 00:42:41]

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

[commit-bot: I haz the power, 2013-07-30 07:23:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/19490002/25001

[commit-bot: I haz the power, 2013-07-30 10:43:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/19490002/25001

[commit-bot: I haz the power, 2013-07-30 22:33:01]

Change committed as 214445