AppShell support for Mac.

AppShell support for Mac.

Very basic support for app_shell on Mac cocoa. It allows building and running app_shell, app_shell_unittests, and app_shell_browsertests on Mac. It introduces app_shell_framework and app_shell_helper targets. Much of the work is based on the content_shell equivalents.

The above targets build as part of 'all' on Mac, but are not yet covered by trybots.

Shortcomings compared to Aura:
- The AppShell desktop controller doesn't support tracking multiple app windows.
- Running App Shell --load-apps works (at least for the Calculator sample) but can only be exited via ^C at the command line, and there is no Mac menu.

BUG=399363
Committed: https://crrev.com/289ee7c3b41766e6996b6a7d6ecd5a574b2a147e
Cr-Commit-Position: refs/heads/master@{#309256}

[Yoyo Zhou, 2014-12-10 01:58:38]

Patchset #1 (id:1) has been deleted

[Yoyo Zhou, 2014-12-10 01:58:45]

Patchset #1 (id:20001) has been deleted

[Yoyo Zhou, 2014-12-10 02:06:40]

yoz@chromium.org changed reviewers:
+ jamescook@chromium.org, mark@chromium.org

[Yoyo Zhou, 2014-12-10 02:06:53]

Patchset #1 (id:40001) has been deleted

[Yoyo Zhou, 2014-12-10 02:07:04]

Patchset #1 (id:60001) has been deleted

[Yoyo Zhou, 2014-12-10 02:18:19]

James: you're the main AppShell reviewer. Can you review the appropriate
app_shell changes?
Mark: you're the main Mac reviewer. Can you review especially app_shell.gyp and
ObjC UI files?

https://codereview.chromium.org/745093002/diff/80001/extensions/shell/app/pat...
File extensions/shell/app/paths_mac.mm (right):

https://codereview.chromium.org/745093002/diff/80001/extensions/shell/app/pat...
extensions/shell/app/paths_mac.mm:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
This is very similar to content/shell/app/paths_mac.cc (and it's only one
example of such). Both depend very much on the respective definitions of the
helper targets in the .gyp, but I wonder if it'd make sense to combine them.

https://codereview.chromium.org/745093002/diff/80001/extensions/shell/browser...
File extensions/shell/browser/shell_app_delegate.cc (left):

https://codereview.chromium.org/745093002/diff/80001/extensions/shell/browser...
extensions/shell/browser/shell_app_delegate.cc:94: NOTIMPLEMENTED();
I kept hitting this in tests. Since there's a TODO here already, I decided to
just remove it.

[James Cook, 2014-12-10 21:05:01]

Overall approach looks good. Lots of questions.

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/ap...
File extensions/shell/app/app-Info.plist (right):

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/ap...
extensions/shell/app/app-Info.plist:14: <string>org.chromium.AppShell</string>
tabs vs. spaces?

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/fr...
File extensions/shell/app/framework-Info.plist (right):

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/fr...
extensions/shell/app/framework-Info.plist:10:
<string>org.chromium.AppShell.framework</string>
ditto

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/he...
File extensions/shell/app/helper-Info.plist (right):

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/he...
extensions/shell/app/helper-Info.plist:12:
<string>org.chromium.AppShell.helper</string>
ditto

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/pa...
File extensions/shell/app/paths_mac.mm (right):

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/pa...
extensions/shell/app/paths_mac.mm:22: // transform it into \
nit: no trailing \

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/pa...
extensions/shell/app/paths_mac.mm:52: base::FilePath GetResourcesPakFilePath() {
Can this live in ShellMainDelegate?

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/sh...
File extensions/shell/app/shell_main.cc (right):

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/sh...
extensions/shell/app/shell_main.cc:26: #else
This is turned into ifdef soup.  What do you think about either writing out
three small main functions (Win, Mac, Linux)?  Or writing three bodies for main?
 I think a couple lines of duplicate code might make this easier to read.

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/sh...
File extensions/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/sh...
extensions/shell/app/shell_main_delegate.cc:139:
extensions_shell_and_test_pak_path = GetResourcesPakFilePath();
It feels a little weird that sometimes you call a function for this and
sometimes you don't.  It seems like there should either get one
GetResourcesPakFilePath() with #ifdef'd lines in the body, or separate
implementations of the function, or it should all be inlined.

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/sh...
File extensions/shell/app/shell_main_mac.cc (right):

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/sh...
extensions/shell/app/shell_main_mac.cc:12: extensions::ShellMainDelegate
delegate;
ugh, this seems like the worst of both worlds -- ifdef soup in the other file
and code duplication in this file.  Is this the way content_shell handles it? 
What about either duplicating the ShellMainDelegate part three times, or
consolidating all the code in shell_main.cc?

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/sh...
File extensions/shell/app/shell_main_mac.h (right):

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app/sh...
extensions/shell/app/shell_main_mac.h:8: #include "base/basictypes.h"
Does this really need basictypes.h?  Or macros.h?  Or compiler_specific.h?

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app_sh...
File extensions/shell/app_shell.gyp (right):

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app_sh...
extensions/shell/app_shell.gyp:186: 'defines!': ['CONTENT_IMPLEMENTATION'],
Why do we need this?

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app_sh...
extensions/shell/app_shell.gyp:447: # 'app/English.lproj/HttpAuth.xib',
Are these left in intentionally?

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app_sh...
extensions/shell/app_shell.gyp:453: 'app/framework-Info.plist',
So this is both included and excluded?

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/app_sh...
extensions/shell/app_shell.gyp:578: ],
Mark, I need to defer to you on this GYP setup. I'm not familiar at all with the
Mac OS stuff.

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/browse...
File extensions/shell/browser/shell_app_delegate.cc (left):

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/browse...
extensions/shell/browser/shell_app_delegate.cc:94: NOTIMPLEMENTED();
Hooray for cleaning this up!

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_main_parts_mac.mm (right):

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts_mac.mm:11: void
ShellBrowserMainParts::PreMainMessageLoopStart() {
Rather than splitting the implementation of this class across two files, what do
you think about having something like this?

ShellBrowserMainParts::PreMainMessageLoopStart() {
#if defined(OS_MACOSX)
  PreMainMessageLoopStartMac();
#else
  ...other stuff...
#endif
}

and put a utility function here?

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/browse...
File extensions/shell/browser/shell_browsertest.cc (right):

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/browse...
extensions/shell/browser/shell_browsertest.cc:32: #endif
Is there a way to test for this focus on non-Aura?  Getting this right on Aura
was a pain.

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/browse...
File extensions/shell/browser/shell_desktop_controller_mac.h (right):

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/browse...
extensions/shell/browser/shell_desktop_controller_mac.h:15: // Only currently
supports one app window (unlike Aura).
Nice class comment.

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/browse...
extensions/shell/browser/shell_desktop_controller_mac.h:36:
DISALLOW_COPY_AND_ASSIGN(ShellDesktopControllerMac);
#include "base/macros.h"

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/browse...
File extensions/shell/browser/shell_desktop_controller_mac.mm (right):

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/browse...
extensions/shell/browser/shell_desktop_controller_mac.mm:18:
extensions::AppWindowClient::Set(app_window_client_.get());
nit: no extensions::

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/test/s...
File extensions/shell/test/shell_test.cc (right):

https://codereview.chromium.org/745093002/diff/100001/extensions/shell/test/s...
extensions/shell/test/shell_test.cc:22: base::FilePath app_shell_path;
Maybe comment what the goal of this block of code is?

[Yoyo Zhou, 2014-12-11 02:40:50]

Thanks for the review.

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
File extensions/shell/app/app-Info.plist (right):

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/app/app-Info.plist:14: <string>org.chromium.AppShell</string>
On 2014/12/10 21:05:00, James Cook wrote:
> tabs vs. spaces?

Tabs appear to be standard in Chromium for our .plist files.

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
File extensions/shell/app/paths_mac.mm (right):

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/app/paths_mac.mm:22: // transform it into \
On 2014/12/10 21:05:00, James Cook wrote:
> nit: no trailing \

Done.

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/app/paths_mac.mm:52: base::FilePath GetResourcesPakFilePath() {
On 2014/12/10 21:05:00, James Cook wrote:
> Can this live in ShellMainDelegate?

Done.

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
File extensions/shell/app/shell_main.cc (right):

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/app/shell_main.cc:26: #else
On 2014/12/10 21:05:00, James Cook wrote:
> This is turned into ifdef soup.  What do you think about either writing out
> three small main functions (Win, Mac, Linux)?  Or writing three bodies for
main?
>  I think a couple lines of duplicate code might make this easier to read.

You're right - this is completely unreadable.

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
File extensions/shell/app/shell_main_delegate.cc (right):

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/app/shell_main_delegate.cc:139:
extensions_shell_and_test_pak_path = GetResourcesPakFilePath();
On 2014/12/10 21:05:00, James Cook wrote:
> It feels a little weird that sometimes you call a function for this and
> sometimes you don't.  It seems like there should either get one
> GetResourcesPakFilePath() with #ifdef'd lines in the body, or separate
> implementations of the function, or it should all be inlined.

I moved the GetResourcesPakFilePath implementation into this file and moved the
OS-specific logic into it.

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
File extensions/shell/app/shell_main_mac.cc (right):

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/app/shell_main_mac.cc:12: extensions::ShellMainDelegate
delegate;
On 2014/12/10 21:05:00, James Cook wrote:
> ugh, this seems like the worst of both worlds -- ifdef soup in the other file
> and code duplication in this file.  Is this the way content_shell handles it? 
> What about either duplicating the ShellMainDelegate part three times, or
> consolidating all the code in shell_main.cc?

This is the way content_shell does it. It's little enough code that I'm not too
concerned, but I agree it's not ideal.

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
File extensions/shell/app/shell_main_mac.h (right):

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/app/shell_main_mac.h:8: #include "base/basictypes.h"
On 2014/12/10 21:05:00, James Cook wrote:
> Does this really need basictypes.h?  Or macros.h?  Or compiler_specific.h?

I figured out it needed build/build_config.h for OS_MACOSX.

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
File extensions/shell/app_shell.gyp (right):

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/app_shell.gyp:447: # 'app/English.lproj/HttpAuth.xib',
On 2014/12/10 21:05:01, James Cook wrote:
> Are these left in intentionally?

Nope, removed.

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/app_shell.gyp:453: 'app/framework-Info.plist',
On 2014/12/10 21:05:01, James Cook wrote:
> So this is both included and excluded?

See the TODO(mark) above. I don't understand this either. =(

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
File extensions/shell/browser/shell_browser_main_parts_mac.mm (right):

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/browser/shell_browser_main_parts_mac.mm:11: void
ShellBrowserMainParts::PreMainMessageLoopStart() {
On 2014/12/10 21:05:01, James Cook wrote:
> Rather than splitting the implementation of this class across two files, what
do
> you think about having something like this?
> 
> ShellBrowserMainParts::PreMainMessageLoopStart() {
> #if defined(OS_MACOSX)
>   PreMainMessageLoopStartMac();
> #else
>   ...other stuff...
> #endif
> }
> 
> and put a utility function here?

I don't think we can have the Mac and non-Mac implementations in the same file,
unfortunately. This one has to be a .mm (ObjC) because it calls ObjC code. The
other platforms don't compile ObjC.

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
File extensions/shell/browser/shell_browsertest.cc (right):

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/browser/shell_browsertest.cc:32: #endif
On 2014/12/10 21:05:01, James Cook wrote:
> Is there a way to test for this focus on non-Aura?  Getting this right on Aura
> was a pain.

I'd like to. Adding a TODO for now. Right now we don't get OS-level focus, only
focus among our application's windows; see the TODO in
ShellNativeAppWindowMac::Activate. There's a utility function in
chrome/browser/ui/cocoa/browser_window_utils.h that does this; obviously, we'd
need to refactor it out of chrome/ to use it.

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
File extensions/shell/browser/shell_desktop_controller_mac.h (right):

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/browser/shell_desktop_controller_mac.h:36:
DISALLOW_COPY_AND_ASSIGN(ShellDesktopControllerMac);
On 2014/12/10 21:05:01, James Cook wrote:
> #include "base/macros.h"

Done.

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
File extensions/shell/browser/shell_desktop_controller_mac.mm (right):

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/browser/shell_desktop_controller_mac.mm:18:
extensions::AppWindowClient::Set(app_window_client_.get());
On 2014/12/10 21:05:01, James Cook wrote:
> nit: no extensions::

Done.

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
File extensions/shell/test/shell_test.cc (right):

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/test/shell_test.cc:22: base::FilePath app_shell_path;
On 2014/12/10 21:05:01, James Cook wrote:
> Maybe comment what the goal of this block of code is?

Done.

[James Cook, 2014-12-11 20:08:28]

Looking good.

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
File extensions/shell/app/app-Info.plist (right):

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/app/app-Info.plist:14: <string>org.chromium.AppShell</string>
On 2014/12/11 02:40:49, Yoyo Zhou wrote:
> On 2014/12/10 21:05:00, James Cook wrote:
> > tabs vs. spaces?
> 
> Tabs appear to be standard in Chromium for our .plist files.

So they are. I was confused by reitveld only showing the tab marker on the
changed line.

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
File extensions/shell/browser/shell_browser_main_parts_mac.mm (right):

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/browser/shell_browser_main_parts_mac.mm:11: void
ShellBrowserMainParts::PreMainMessageLoopStart() {
On 2014/12/11 02:40:50, Yoyo Zhou wrote:
> On 2014/12/10 21:05:01, James Cook wrote:
> > Rather than splitting the implementation of this class across two files,
what
> do
> > you think about having something like this?
> > 
> > ShellBrowserMainParts::PreMainMessageLoopStart() {
> > #if defined(OS_MACOSX)
> >   PreMainMessageLoopStartMac();
> > #else
> >   ...other stuff...
> > #endif
> > }
> > 
> > and put a utility function here?
> 
> I don't think we can have the Mac and non-Mac implementations in the same
file,
> unfortunately. This one has to be a .mm (ObjC) because it calls ObjC code. The
> other platforms don't compile ObjC.

What I meant was to keep this implementation here, but put it in a utility
function like PreMainMessageLoopStartMac(). Then call it from
ShellBrowserMainParts::PreMainMessageLoopStart(). I find it confusing when
there's a class with most of the methods implemented in one file, but one or two
of them implemented elsewhere.

https://chromiumcodereview.appspot.com/745093002/diff/120001/extensions/shell...
File extensions/shell/app/shell_main.cc (right):

https://chromiumcodereview.appspot.com/745093002/diff/120001/extensions/shell...
extensions/shell/app/shell_main.cc:45: #endif
Yeah, much better.

https://chromiumcodereview.appspot.com/745093002/diff/120001/extensions/shell...
File extensions/shell/app_shell.gyp (right):

https://chromiumcodereview.appspot.com/745093002/diff/120001/extensions/shell...
extensions/shell/app_shell.gyp:186: 'defines!': ['CONTENT_IMPLEMENTATION'],
I still don't think you need this, since we aren't content_shell.

[Yoyo Zhou, 2014-12-16 01:11:19]

Mark: ping. If you can't do this review, can you suggest an alternate reviewer?

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
File extensions/shell/browser/shell_browser_main_parts_mac.mm (right):

https://chromiumcodereview.appspot.com/745093002/diff/100001/extensions/shell...
extensions/shell/browser/shell_browser_main_parts_mac.mm:11: void
ShellBrowserMainParts::PreMainMessageLoopStart() {
On 2014/12/11 20:08:28, James Cook wrote:
> On 2014/12/11 02:40:50, Yoyo Zhou wrote:
> > On 2014/12/10 21:05:01, James Cook wrote:
> > > Rather than splitting the implementation of this class across two files,
> what
> > do
> > > you think about having something like this?
> > > 
> > > ShellBrowserMainParts::PreMainMessageLoopStart() {
> > > #if defined(OS_MACOSX)
> > >   PreMainMessageLoopStartMac();
> > > #else
> > >   ...other stuff...
> > > #endif
> > > }
> > > 
> > > and put a utility function here?
> > 
> > I don't think we can have the Mac and non-Mac implementations in the same
> file,
> > unfortunately. This one has to be a .mm (ObjC) because it calls ObjC code.
The
> > other platforms don't compile ObjC.
> 
> What I meant was to keep this implementation here, but put it in a utility
> function like PreMainMessageLoopStartMac(). Then call it from
> ShellBrowserMainParts::PreMainMessageLoopStart(). I find it confusing when
> there's a class with most of the methods implemented in one file, but one or
two
> of them implemented elsewhere.

Ok. I like that suggestion.

https://chromiumcodereview.appspot.com/745093002/diff/120001/extensions/shell...
File extensions/shell/app_shell.gyp (right):

https://chromiumcodereview.appspot.com/745093002/diff/120001/extensions/shell...
extensions/shell/app_shell.gyp:186: 'defines!': ['CONTENT_IMPLEMENTATION'],
On 2014/12/11 20:08:28, James Cook wrote:
> I still don't think you need this, since we aren't content_shell.

Done.

[Mark Mentovai, 2014-12-16 14:21:13]

Sorry, I’m not sure I’ll be able to get to this. You can try thakis, avi, or
rsesek.

[James Cook, 2014-12-16 16:56:03]

non-Mac bits LGTM with nit

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
File extensions/shell/browser/shell_browser_main_parts_mac.h (right):

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_browser_main_parts_mac.h:13: }
nit: // namespace extensions

[Yoyo Zhou, 2014-12-16 21:02:20]

yoz@chromium.org changed reviewers:
+ thakis@chromium.org

[Yoyo Zhou, 2014-12-16 21:02:22]

Nico, can you take a look at the Mac parts of this?

[Nico, 2014-12-16 23:44:35]

thakis@chromium.org changed reviewers:
+ tapted@chromium.org

[Nico, 2014-12-16 23:44:36]

app shell mac -> tapted (i think?)

[tapted, 2014-12-17 00:00:51]

On 2014/12/16 23:44:36, Nico wrote:
> app shell mac -> tapted (i think?)

Yep - I'm happy to take a look at the cocoa stuff - I should be able to get to
it today (AEDST). I've been poking around in .app bundles recently..

(just had a quick skim, but there's some curly stuff). You might even be
interested in trying out GYP_DEFINES toolkit_views=1, and just using
NativeAppWindowViews . It's not _actually_ that far off.... But I'll dive in to
this CL first to see if that would be helpful.

[tapted, 2014-12-17 08:05:16]

Some of the gyp stuff is a bit over my head TBH. I didn't spot any glaring
errors though. (And, it's a lot of gyp voodoo to maintain separately from the
other app bundle targets, but I doubt it's worth refactoring if it would soon
all be deprecated in favour of a elegant but yet-to-be-written GN
somethingorother.).

On 2014/12/17 00:00:51, tapted wrote:
> You might even be
> interested in trying out GYP_DEFINES toolkit_views=1, and just using
> NativeAppWindowViews . It's not _actually_ that far off.... But I'll dive in
to
> this CL first to see if that would be helpful.

after diving in, it looks like you won't want the ui/views dependency that this
would entail :)

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/app/pa...
File extensions/shell/app/paths_mac.mm (right):

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/app/pa...
extensions/shell/app/paths_mac.mm:10: #include "base/mac/bundle_locations.h"
nit: import. Alternatively, as it is, paths_mac.mm looks like it would compile
fine as .cc (and these includes can stay includes, since they support either),
but that's probably unnecessarily limiting

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/app/pa...
extensions/shell/app/paths_mac.mm:11: #include "base/mac/foundation_util.h"
nit: import

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/app/sh...
File extensions/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/app/sh...
extensions/shell/app/shell_main_delegate.cc:58:
CFSTR("extensions_shell_and_test.pak"));
I think this approach won't handle the way mac does locales. String resources
get split into *.framework/Resources/$LANG.lproj/locale.pak (see e.g.
ui/base/test/run_all_unittests.cc for a way to initialize). But, if locales are
needed, maybe a follow-up is fine.

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/app/sh...
File extensions/shell/app/shell_main_mac.cc (right):

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/app/sh...
extensions/shell/app/shell_main_mac.cc:10: #if defined(OS_MACOSX)
nit: are these #ifdefs needed? (same comment for the header).

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/app/sh...
File extensions/shell/app/shell_main_mac.h (right):

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/app/sh...
extensions/shell/app/shell_main_mac.h:17: #endif  //
CONTENT_SHELL_APP_SHELL_CONTENT_MAIN_H_
  // EXTENSIONS_SHELL_APP_SHELL_MAIN_MAC_H_

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
File extensions/shell/browser/shell_browsertest.cc (right):

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_browsertest.cc:29: // TOOD(yoz): Test for focus
on Cocoa.
for later: maybe (crossplatform) app_window->GetBaseWindow()->IsActive()?

(but note that you'll need an interactive_ui_test and a runloop spun here since
focus changes on Mac are asynchronous)

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
File extensions/shell/browser/shell_desktop_controller_mac.h (right):

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_desktop_controller_mac.h:4: 
include guards #ifndef etc.

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
File extensions/shell/browser/shell_native_app_window_mac.h (right):

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_native_app_window_mac.h:10: #include
"base/mac/scoped_nsobject.h"
nit: import

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_native_app_window_mac.h:19: @interface
ShellNativeAppWindowController
nit: a short comment? (just to satisfy the objc style guide...)

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_native_app_window_mac.h:21:
extensions::ShellNativeAppWindowMac* appWindow_;  // Owns us.
 @private

before this

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
File extensions/shell/browser/shell_native_app_window_mac.mm (right):

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_native_app_window_mac.mm:5: #include
"extensions/shell/browser/shell_native_app_window_mac.h"
nit: import

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_native_app_window_mac.mm:47: // TODO(yoz): Flip
coordinates here?
you could use gfx::ScreenRectToNSRect from #import
"ui/gfx/mac/coordinate_conversion.h"

it might also be good to check for a zero-sized window -- see
http://www.chromium.org/developers/coding-style/cocoa-dos-and-donts#TOC-Zero-...

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_native_app_window_mac.mm:56:
initWithWindow:shell_window.release()]);
The release() looks a bit like a leak, but it's needed unless you call
[shell_window setReleasedWhenClosed:NO].

Usually I find things easier to follow with setReleasedWhenClosed:NO, then
ShellNativeAppWindowMac could have scoped_nsobjects for both the NSWindow and
the NSWindowDelegate classes, and you get better control over the lifetime.

In ~ShellNativeAppWindowMac(), you would just do
  [window() setDelegate:nil];
  [window() close];

That might untangle the lifetime comments in shell_desktop_controller_mac.mm
too.


Also, since the delegate is owned by |this|, it might be nicer to pass that to a
newly declared intializer. i.e.

  window_controller_.reset(
      [[ShellNativeAppWindowController alloc] initWithAppWindow:app_window]);

then in, initWithAppWindow, create the ShellNSWindow and do

  if ((self = [super initWithWindow:shell_window])) {
    appWindow_ = appWindow;
  }
  return self;
}


(then make the property readonly).

This is fine too, but it might be a good follow-up if the initialization gets
more complicated. 


But if you don't like these ideas, and stick with the default (for NSWindow) of
releasedWhenClosed YES, then I'd just put a comment before this like

// Call release(), since [NSWindow releasedWhenClosed] defaults to YES.

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_native_app_window_mac.mm:68: }
this should call [window() setDelegate:nil];  (the window_controller_ data
member is about to go out of scope, but delegates are not retained, and the
window is reference counted, so there could be references to it in event loops
and things that keep it alive)

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_native_app_window_mac.mm:71: NOTIMPLEMENTED();
if you feel like it now.. [window() isKeyWindow] should be enough.

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_native_app_window_mac.mm:80: // Flip coordinates
based on the primary screen.
  return gfx::ScreenRectFromNSRect([window() frame]);

https://codereview.chromium.org/745093002/diff/140001/extensions/shell/browse...
extensions/shell/browser/shell_native_app_window_mac.mm:118: CHECK(!window ||
[window isKindOfClass:[ShellNSWindow class]]);
maybe, from base/mac/foundation_util.h

  return base::mac::ObjCCastStrict<ShellNSWindow>([window_controller_ window]);

(note that would make it a DCHECK)

[Yoyo Zhou, 2014-12-18 02:38:34]

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
File extensions/shell/app/paths_mac.mm (right):

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/app/paths_mac.mm:10: #include "base/mac/bundle_locations.h"
On 2014/12/17 08:05:15, tapted wrote:
> nit: import. Alternatively, as it is, paths_mac.mm looks like it would compile
> fine as .cc (and these includes can stay includes, since they support either),
> but that's probably unnecessarily limiting

These files are only #included in the chromium codebase. Am I missing something?
According to the style guide, ObjC headers should be #imported, but these ones
have #ifdef guards around __OBJC__.

You're right that this could be a .cc file. I'm inclined to leave it (at some
point in development, I think I needed some ObjC here).

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
File extensions/shell/app/shell_main_delegate.cc (right):

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/app/shell_main_delegate.cc:58:
CFSTR("extensions_shell_and_test.pak"));
On 2014/12/17 08:05:15, tapted wrote:
> I think this approach won't handle the way mac does locales. String resources
> get split into *.framework/Resources/$LANG.lproj/locale.pak (see e.g.
> ui/base/test/run_all_unittests.cc for a way to initialize). But, if locales
are
> needed, maybe a follow-up is fine.
> 

Right, I think we don't need locales right now. For mac we're mainly interested
in using app_shell as a test environment anyway, similar to content_shell.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
File extensions/shell/app/shell_main_mac.cc (right):

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/app/shell_main_mac.cc:10: #if defined(OS_MACOSX)
On 2014/12/17 08:05:15, tapted wrote:
> nit: are these #ifdefs needed? (same comment for the header).

No.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
File extensions/shell/app/shell_main_mac.h (right):

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/app/shell_main_mac.h:17: #endif  //
CONTENT_SHELL_APP_SHELL_CONTENT_MAIN_H_
On 2014/12/17 08:05:16, tapted wrote:
>   // EXTENSIONS_SHELL_APP_SHELL_MAIN_MAC_H_

Woops.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
File extensions/shell/browser/shell_browser_main_parts_mac.h (right):

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_browser_main_parts_mac.h:13: }
On 2014/12/16 16:56:03, James Cook wrote:
> nit: // namespace extensions

Done.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
File extensions/shell/browser/shell_browsertest.cc (right):

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_browsertest.cc:29: // TOOD(yoz): Test for focus
on Cocoa.
On 2014/12/17 08:05:16, tapted wrote:
> for later: maybe (crossplatform) app_window->GetBaseWindow()->IsActive()?
> 
> (but note that you'll need an interactive_ui_test and a runloop spun here
since
> focus changes on Mac are asynchronous)

Ugh, noted in a TODO.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
File extensions/shell/browser/shell_desktop_controller_mac.h (right):

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_desktop_controller_mac.h:4: 
On 2014/12/17 08:05:16, tapted wrote:
> include guards #ifndef etc.

Done.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
File extensions/shell/browser/shell_native_app_window_mac.h (right):

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_native_app_window_mac.h:10: #include
"base/mac/scoped_nsobject.h"
On 2014/12/17 08:05:16, tapted wrote:
> nit: import

This is also only ever #included, not #imported.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_native_app_window_mac.h:19: @interface
ShellNativeAppWindowController
On 2014/12/17 08:05:16, tapted wrote:
> nit: a short comment? (just to satisfy the objc style guide...)

Done.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_native_app_window_mac.h:21:
extensions::ShellNativeAppWindowMac* appWindow_;  // Owns us.
On 2014/12/17 08:05:16, tapted wrote:
>  @private
> 
> before this

Done.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
File extensions/shell/browser/shell_native_app_window_mac.mm (right):

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_native_app_window_mac.mm:5: #include
"extensions/shell/browser/shell_native_app_window_mac.h"
On 2014/12/17 08:05:16, tapted wrote:
> nit: import

Done.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_native_app_window_mac.mm:47: // TODO(yoz): Flip
coordinates here?
On 2014/12/17 08:05:16, tapted wrote:
> you could use gfx::ScreenRectToNSRect from #import
> "ui/gfx/mac/coordinate_conversion.h"

Done.

> it might also be good to check for a zero-sized window -- see
>
http://www.chromium.org/developers/coding-style/cocoa-dos-and-donts#TOC-Zero-...

What should we do if there is a zero-sized window? I'm using
NativeAppWIndowCocoa as a reference implementation, and it doesn't do anything
in that case.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_native_app_window_mac.mm:56:
initWithWindow:shell_window.release()]);
On 2014/12/17 08:05:16, tapted wrote:
> The release() looks a bit like a leak, but it's needed unless you call
> [shell_window setReleasedWhenClosed:NO].
> 
> Usually I find things easier to follow with setReleasedWhenClosed:NO, then
> ShellNativeAppWindowMac could have scoped_nsobjects for both the NSWindow and
> the NSWindowDelegate classes, and you get better control over the lifetime.
> 
> In ~ShellNativeAppWindowMac(), you would just do
>   [window() setDelegate:nil];
>   [window() close];
> 
> That might untangle the lifetime comments in shell_desktop_controller_mac.mm
> too.
> 
> 
> Also, since the delegate is owned by |this|, it might be nicer to pass that to
a
> newly declared intializer. i.e.
> 
>   window_controller_.reset(
>       [[ShellNativeAppWindowController alloc] initWithAppWindow:app_window]);
> 
> then in, initWithAppWindow, create the ShellNSWindow and do
> 
>   if ((self = [super initWithWindow:shell_window])) {
>     appWindow_ = appWindow;
>   }
>   return self;
> }
> 
> 
> (then make the property readonly).
> 
> This is fine too, but it might be a good follow-up if the initialization gets
> more complicated. 

This looks more complicated right now; I was hoping to have just a minimal
NSWindowController.

> 
> But if you don't like these ideas, and stick with the default (for NSWindow)
of
> releasedWhenClosed YES, then I'd just put a comment before this like
> 
> // Call release(), since [NSWindow releasedWhenClosed] defaults to YES.

I think this release() and the Cocoa -release are not the same:

// scoped_nsprotocol<>::release() is like scoped_ptr<>::release.  It is NOT a
// wrapper for [object_ release].

So my release() here is so the window doesn't get deallocated (-released) when
the scoped_nsobject goes out of scope. I don't have a strong opinion on
released-when-closed, though, so I'll go with that.

I'm less confident about whether it's now a leak.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_native_app_window_mac.mm:68: }
On 2014/12/17 08:05:16, tapted wrote:
> this should call [window() setDelegate:nil];  (the window_controller_ data
> member is about to go out of scope, but delegates are not retained, and the
> window is reference counted, so there could be references to it in event loops
> and things that keep it alive)

Done.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_native_app_window_mac.mm:71: NOTIMPLEMENTED();
On 2014/12/17 08:05:16, tapted wrote:
> if you feel like it now.. [window() isKeyWindow] should be enough.

Done.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_native_app_window_mac.mm:80: // Flip coordinates
based on the primary screen.
On 2014/12/17 08:05:16, tapted wrote:
>   return gfx::ScreenRectFromNSRect([window() frame]);

Done.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_native_app_window_mac.mm:118: CHECK(!window ||
[window isKindOfClass:[ShellNSWindow class]]);
On 2014/12/17 08:05:16, tapted wrote:
> maybe, from base/mac/foundation_util.h
> 
>   return base::mac::ObjCCastStrict<ShellNSWindow>([window_controller_
window]);
> 
> (note that would make it a DCHECK)

Done.

[tapted, 2014-12-18 03:30:55]

lgtm with the .release() removed - sorry for the confusion.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
File extensions/shell/app/paths_mac.mm (right):

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/app/paths_mac.mm:10: #include "base/mac/bundle_locations.h"
On 2014/12/18 02:38:33, Yoyo Zhou wrote:
> On 2014/12/17 08:05:15, tapted wrote:
> > nit: import. Alternatively, as it is, paths_mac.mm looks like it would
compile
> > fine as .cc (and these includes can stay includes, since they support
either),
> > but that's probably unnecessarily limiting
> 
> These files are only #included in the chromium codebase. Am I missing
something?
> According to the style guide, ObjC headers should be #imported, but these ones
> have #ifdef guards around __OBJC__.

style guide says "When including a header that uses Objective-C or
Objective-C++, use #import", I've always interpreted that as meaning to #import
cc/mm-agnostic headers if it's in a .mm. But I guess it doesn't matter.

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
File extensions/shell/browser/shell_native_app_window_mac.h (right):

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_native_app_window_mac.h:10: #include
"base/mac/scoped_nsobject.h"
On 2014/12/18 02:38:33, Yoyo Zhou wrote:
> On 2014/12/17 08:05:16, tapted wrote:
> > nit: import
> 
> This is also only ever #included, not #imported.

scoped_nsobject.h is objective C-only (it won't compile in .cc). style guide
says it should be import, so I always import. I guess include wins a grep-fight
though 396 vs 94.`git grep -h 'scoped_nsobject\.h' | sort | uniq -c`. /shrug

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
File extensions/shell/browser/shell_native_app_window_mac.mm (right):

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_native_app_window_mac.mm:56:
initWithWindow:shell_window.release()]);
On 2014/12/18 02:38:34, Yoyo Zhou wrote:
> On 2014/12/17 08:05:16, tapted wrote:
> > But if you don't like these ideas, and stick with the default (for NSWindow)
> of
> > releasedWhenClosed YES, then I'd just put a comment before this like
> > 
> > // Call release(), since [NSWindow releasedWhenClosed] defaults to YES.
> 
> I think this release() and the Cocoa -release are not the same:
> 
> // scoped_nsprotocol<>::release() is like scoped_ptr<>::release.  It is NOT a
> // wrapper for [object_ release].
> 
> So my release() here is so the window doesn't get deallocated (-released) when
> the scoped_nsobject goes out of scope. I don't have a strong opinion on
> released-when-closed, though, so I'll go with that.
> 
> I'm less confident about whether it's now a leak.

Corect - the releases are not the same. But NSWindow is weird, since it can
effectively own itself.

e.g. This is a leak:

scoped_nsobject<NSString> foo([[NSString alloc] init]);
scoped_nsobject<NSString> bar([[NSString alloc] initWithString:foo.release()];

but a similar thing with NSWindow is not, so long as the NSWindow is later shown
(and then closed).

To fix the leak in the example above, it should be just

scoped_nsobject<NSString> bar([[NSString alloc] initWithString:foo];

https://chromiumcodereview.appspot.com/745093002/diff/160001/extensions/shell...
File extensions/shell/browser/shell_native_app_window_mac.mm (right):

https://chromiumcodereview.appspot.com/745093002/diff/160001/extensions/shell...
extensions/shell/browser/shell_native_app_window_mac.mm:59:
initWithWindow:shell_window.release()]);
It's now properly owned by the scoped_nsobject, so here:

shell_window.release() -> shell_window

[Yoyo Zhou, 2014-12-18 22:42:06]

yoz@chromium.org changed reviewers:
+ mkwst@chromium.org

[Yoyo Zhou, 2014-12-18 22:42:07]

thakis: can I get an LG for build/all.gyp? If you have any other feedback on the
gyp, that would be helpful too.
mkwst: can I get an approval for adding
content/shell/browser/shell_application_mac.h to the app_shell DEPS?

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
File extensions/shell/browser/shell_native_app_window_mac.h (right):

https://chromiumcodereview.appspot.com/745093002/diff/140001/extensions/shell...
extensions/shell/browser/shell_native_app_window_mac.h:10: #include
"base/mac/scoped_nsobject.h"
On 2014/12/18 03:30:55, tapted wrote:
> On 2014/12/18 02:38:33, Yoyo Zhou wrote:
> > On 2014/12/17 08:05:16, tapted wrote:
> > > nit: import
> > 
> > This is also only ever #included, not #imported.
> 
> scoped_nsobject.h is objective C-only (it won't compile in .cc). style guide
> says it should be import, so I always import. I guess include wins a
grep-fight
> though 396 vs 94.`git grep -h 'scoped_nsobject\.h' | sort | uniq -c`. /shrug

Yeah, I miscounted and there are some #imports. As long as it's #imported
somewhere else, I don't feel too out of line. =)

https://chromiumcodereview.appspot.com/745093002/diff/160001/extensions/shell...
File extensions/shell/browser/shell_native_app_window_mac.mm (right):

https://chromiumcodereview.appspot.com/745093002/diff/160001/extensions/shell...
extensions/shell/browser/shell_native_app_window_mac.mm:59:
initWithWindow:shell_window.release()]);
On 2014/12/18 03:30:55, tapted wrote:
> It's now properly owned by the scoped_nsobject, so here:
> 
> shell_window.release() -> shell_window

Done.

[Nico, 2014-12-18 22:50:56]

all.gyp lgtm

I glanced over the other gyp changes; they look similar to other mac framework
goop we have elsewhere. (Maybe it'd be possible to have a
build/mac/framework.gypi file that does most of this with less copy-pasta, but
this is already in many places so I don't think you need to clean this up with
this cl)

[Mike West, 2014-12-19 08:47:08]

content/shell LGTM.

[Yoyo Zhou, 2014-12-19 19:50:35]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-12-19 19:51:43]

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

[commit-bot: I haz the power, 2014-12-19 20:58:52]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2014-12-19 20:59:55]

Patchset 6 (id:??) landed as
https://crrev.com/289ee7c3b41766e6996b6a7d6ecd5a574b2a147e
Cr-Commit-Position: refs/heads/master@{#309256}