First pass at content shell for the Mac. Doesn't work yet.

First pass at content shell for the Mac. Doesn't work yet.

BUG=115784
TEST=none

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

[Mark Mentovai, 2012-02-28 20:12:40]

This is a very good first cut. Here are my comments.

https://chromiumcodereview.appspot.com/9513006/diff/1/build/mac/copy_framewor...
File build/mac/copy_framework_unversioned (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/build/mac/copy_framewor...
build/mac/copy_framework_unversioned:1: #!/bin/bash
This should now be named copy_framework_unversioned.sh, which I hate but allows
presubmit to enforce file type-based checks.

https://chromiumcodereview.appspot.com/9513006/diff/1/chrome/app/unexported_s...
File chrome/app/unexported_symbols.exp (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/chrome/app/unexported_s...
chrome/app/unexported_symbols.exp:1:
__ZN7content11ContentMainEiPPKcPNS_19ContentMainDelegateE
1. what is “.exp”? Exported? This seems like the opposite of that.

2. This controls exports for the framework, and we already have framework.order,
so I’d suggest framework.unexport.

3. Mangled symbols are bad for exporting like this. Declare ContentMain as
“extern "C"” and then the symbol name would be _ContentMain. It’s probably
cleanest to provide an exported extern "C" ContentMain wrapper function that
calls into the C++ content::ContentMain.

4. Provide a license boilerplate and maybe a description of why you’re doing
this, as in framework.order.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/app/content_mai...
File content/app/content_main.cc (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/content/app/content_mai...
content/app/content_main.cc:17: #if defined(OS_MACOSX)
Refer to my comments in the unexport file for my suggestions here regarding name
mangling and a simple wrapper function.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/content/content_shell.g...
content/content_shell.gypi:123: 'product_name': '<(content_shell_product_name)
Framework',
No Info.plist?

https://chromiumcodereview.appspot.com/9513006/diff/1/content/content_shell.g...
content/content_shell.gypi:130: 'EXPORTED_SYMBOLS_FILE':
'shell/mac/exported_symbols.exp',
That name’s kinda redundant, no? Following the example in Chrome, I’d expect the
name to have something to do with “framework”, and the Info.plist to be
framework-Info.plist, to disambiguate from the names that have to do with the
app or other bundles or linker output.

I don’t think that this is even necessary now that you’re using
__attribute__((visibility("default"))).

https://chromiumcodereview.appspot.com/9513006/diff/1/content/content_shell.g...
content/content_shell.gypi:226: 'shell/shell_main.cc',
In this file:

#if defined(OS_MACOSX)
__attribute__((visibility("default")))
int main(int argc, const char* argv[]) {
#else
int main(int argc, const char** argv) {
#endif  // OS_MACOSX

1. The “const” is technically illegal, and

2. There’s no reason that non-OS_MACOSX can’t use “char* argv[]” like the
OS_MACOSX branch.

As I will mention elsewhere, exposing the delegate from the framework seems kind
of unclean to me. I think that the best approach would be to allow ContentMain
to accept a NULL delegate; in the case of a NULL delegate passed in, it should
use its own built-in ContentShellDelegate as the default. This will eliminate
the need to pass the default delegate in and therefore eliminate the need to
export these extra symbols.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/content_shell.g...
content/content_shell.gypi:230: 'shell/mac/Info.plist',
app-Info.plist. Again on 236 and 239.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/content_shell.g...
content/content_shell.gypi:275: 'postbuilds': [
It’d be nice to run tweak_info_plist just to get the SVN keys set. Some things
in base that do version number lookups actually care about that. You’d run
tweak_info_plist with -s1 and the other things false.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/content_shell.g...
content/content_shell.gypi:275: 'postbuilds': [
The “verify” build step from chrome_exe would be very nice to have here. There
should be no Objective-C in the main executable. See bug 88697.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/public/app/cont...
File content/public/app/content_main.h (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/content/public/app/cont...
content/public/app/content_main.h:38: __attribute__((visibility("default")))
As discussed previously with respect to an “extern "C"” wrapper.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/public/app/cont...
content/public/app/content_main.h:40: CONTENT_EXPORT int ContentMain(int argc,
No new indentation here.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Engli...
File content/shell/mac/English.lproj/InfoPlist.strings (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Engli...
content/shell/mac/English.lproj/InfoPlist.strings:1: /* Localized versions of
Info.plist keys */
Boilerplate license?

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Engli...
content/shell/mac/English.lproj/InfoPlist.strings:3: NSHumanReadableCopyright =
"© Google Inc, 2011";
1. It’s 2012.

2. This ought to be dynamic, in the same way that Chrome uses
infoplist_strings_tool (target infoplist_strings_util).

3. The third option is to not provide this key (and thus file) at all, which for
the purposes of the content shell, is probably more than fine at this point.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Engli...
File content/shell/mac/English.lproj/MainMenu.xib (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Engli...
content/shell/mac/English.lproj/MainMenu.xib:1: <?xml version="1.0"
encoding="UTF-8"?>
I am not reviewing this file but am assuming it’s OK.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
File content/shell/mac/Info.plist (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
content/shell/mac/Info.plist:1: <?xml version="1.0" encoding="UTF-8"?>
app-Info.plist.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
content/shell/mac/Info.plist:6: <string>English</string>
English is deprecated. Use “en” here.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
content/shell/mac/Info.plist:7: <key>CFBundleExecutable</key>
You should provide CFBundleDisplayName, which can also be ${PRODUCT_NAME} like
CFBundleName.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
content/shell/mac/Info.plist:12: <string>org.chromium.ContentShell</string>
Ideally this would be switchable on the basis of whether the build is set to
“official” or not.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
content/shell/mac/Info.plist:17: <key>CFBundlePackageType</key>
Remove CFBundlePackageType and CFBundleSignature.

They’re only relevant in an .app if CFBundleSignature is meaningful, and in this
case, it is not.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
content/shell/mac/Info.plist:22: <string>1.0</string>
This is relevant to Launch Services, so we should do better than using a
hard-coded version number. 1.0 is fine for now, but there should be some sort of
plan for versioning the content shell, and generating the CFBundleVersion will
need to adhere to silly rules for how CFBundleVersions can be numbered.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
content/shell/mac/Info.plist:23: <key>NSMainNibFile</key>
LSMinimumSystemVersion = 10.5.0.

LSFileQuarantineEnabled = true.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
content/shell/mac/Info.plist:26: <string>NSApplication</string>
NSSupportsAutomaticGraphicsSwitching = true. (This is for full Chrome
equivalence.)

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/expor...
File content/shell/mac/exported_symbols.exp (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/expor...
content/shell/mac/exported_symbols.exp:1: __ZN17ShellMainDelegateC1Ev
Can we come up with a better solution than forcing this and its destructor to be
exposed? Perhaps the ContentMain wrapper (the “extern "C"” one I’m strongly
recommending you adopt throughout this review) can take care of this, so that
the application doesn’t need to know or care about it.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/expor...
content/shell/mac/exported_symbols.exp:1: __ZN17ShellMainDelegateC1Ev
Boilerplate lice and a description, as noted elsewhere in this review.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/expor...
content/shell/mac/exported_symbols.exp:3:
__ZN7content11ContentMainEiPPKcPNS_19ContentMainDelegateE
As mentioned previously, it’s unlikely that you need to mention this name here,
as whatever you need to export should be decorated with
__attribute__((visibility("default"))).

[Avi (use Gerrit), 2012-02-29 19:46:49]

https://chromiumcodereview.appspot.com/9513006/diff/1/build/mac/copy_framewor...
File build/mac/copy_framework_unversioned (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/build/mac/copy_framewor...
build/mac/copy_framework_unversioned:1: #!/bin/bash
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> This should now be named copy_framework_unversioned.sh, which I hate but
allows
> presubmit to enforce file type-based checks.

Done.

https://chromiumcodereview.appspot.com/9513006/diff/1/chrome/app/unexported_s...
File chrome/app/unexported_symbols.exp (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/chrome/app/unexported_s...
chrome/app/unexported_symbols.exp:1:
__ZN7content11ContentMainEiPPKcPNS_19ContentMainDelegateE
Mooted.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/app/content_mai...
File content/app/content_main.cc (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/content/app/content_mai...
content/app/content_main.cc:17: #if defined(OS_MACOSX)
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> Refer to my comments in the unexport file for my suggestions here regarding
name
> mangling and a simple wrapper function.

Done.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/content/content_shell.g...
content/content_shell.gypi:123: 'product_name': '<(content_shell_product_name)
Framework',
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> No Info.plist?

Do I need one?

https://chromiumcodereview.appspot.com/9513006/diff/1/content/content_shell.g...
content/content_shell.gypi:130: 'EXPORTED_SYMBOLS_FILE':
'shell/mac/exported_symbols.exp',
mooted.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/content_shell.g...
content/content_shell.gypi:226: 'shell/shell_main.cc',
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> 2. There’s no reason that non-OS_MACOSX can’t use “char* argv[]” like the
> OS_MACOSX branch.

Simplified.

> I think that the best approach would be to allow ContentMain
> to accept a NULL delegate; in the case of a NULL delegate passed in, it should
> use its own built-in ContentShellDelegate as the default.

Now that we have shell_browser_main, we can do that there. Otherwise it would be
a gross layering violation.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/content_shell.g...
content/content_shell.gypi:230: 'shell/mac/Info.plist',
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> app-Info.plist. Again on 236 and 239.

Done.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/content_shell.g...
content/content_shell.gypi:275: 'postbuilds': [
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> It’d be nice to run tweak_info_plist just to get the SVN keys set. Some things
> in base that do version number lookups actually care about that. You’d run
> tweak_info_plist with -s1 and the other things false.

Done.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/content_shell.g...
content/content_shell.gypi:275: 'postbuilds': [
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> The “verify” build step from chrome_exe would be very nice to have here. There
> should be no Objective-C in the main executable. See bug 88697.

Done.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/public/app/cont...
File content/public/app/content_main.h (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/content/public/app/cont...
content/public/app/content_main.h:38: __attribute__((visibility("default")))
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> As discussed previously with respect to an “extern "C"” wrapper.

Done.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/public/app/cont...
content/public/app/content_main.h:40: CONTENT_EXPORT int ContentMain(int argc,
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> No new indentation here.

Done.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Engli...
File content/shell/mac/English.lproj/InfoPlist.strings (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Engli...
content/shell/mac/English.lproj/InfoPlist.strings:3: NSHumanReadableCopyright =
"© Google Inc, 2011";
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> 1. It’s 2012.
> 
> 2. This ought to be dynamic, in the same way that Chrome uses
> infoplist_strings_tool (target infoplist_strings_util).
> 
> 3. The third option is to not provide this key (and thus file) at all, which
for
> the purposes of the content shell, is probably more than fine at this point.

Done.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
File content/shell/mac/Info.plist (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
content/shell/mac/Info.plist:1: <?xml version="1.0" encoding="UTF-8"?>
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> app-Info.plist.

Done.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
content/shell/mac/Info.plist:6: <string>English</string>
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> English is deprecated. Use “en” here.

Done.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
content/shell/mac/Info.plist:7: <key>CFBundleExecutable</key>
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> You should provide CFBundleDisplayName, which can also be ${PRODUCT_NAME} like
> CFBundleName.

Done.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
content/shell/mac/Info.plist:12: <string>org.chromium.ContentShell</string>
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> Ideally this would be switchable on the basis of whether the build is set to
> “official” or not.

I don't believe there is a plan to produce an official Chrome Content Shell.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
content/shell/mac/Info.plist:17: <key>CFBundlePackageType</key>
...
  File "./gyp-mac-tool", line 138, in ExecCopyInfoPlist
    self._WritePkgInfo(dest)
  File "./gyp-mac-tool", line 147, in _WritePkgInfo
  MACTOOL copy-bundle-resource out/Debug/obj/gen/content/shell_resources.pak
    package_type = plist['CFBundlePackageType']
KeyError: 'CFBundlePackageType'

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
content/shell/mac/Info.plist:23: <key>NSMainNibFile</key>
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> LSMinimumSystemVersion = 10.5.0.
> 
> LSFileQuarantineEnabled = true.

Done.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/Info....
content/shell/mac/Info.plist:26: <string>NSApplication</string>
On 2012/02/28 20:12:40, Mark Mentovai wrote:
> NSSupportsAutomaticGraphicsSwitching = true. (This is for full Chrome
> equivalence.)

Done.

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/expor...
File content/shell/mac/exported_symbols.exp (right):

https://chromiumcodereview.appspot.com/9513006/diff/1/content/shell/mac/expor...
content/shell/mac/exported_symbols.exp:1: __ZN17ShellMainDelegateC1Ev
Everything in this file is mooted.

[Mark Mentovai, 2012-03-01 22:28:30]

I don’t see any major issues remaining. Your description still says “doesn‘t
work yet.” Are you pursuing real for-checkin review now and do you want me to
utter the magic four letters?

https://chromiumcodereview.appspot.com/9513006/diff/5002/content/content_shel...
File content/content_shell.gypi (right):

https://chromiumcodereview.appspot.com/9513006/diff/5002/content/content_shel...
content/content_shell.gypi:293: 'unused branding',
Rather than passing two bogus arguments here, can we make tweak_info_plist
accept zero arguments as long as Keystone and Breakpad are both turned off? Or
something like that?

I can see this going in a bad direction if someone ever modifies
tweak_info_plist to start using these two values.

https://chromiumcodereview.appspot.com/9513006/diff/5002/content/shell/mac/ap...
File content/shell/mac/app-Info.plist (right):

https://chromiumcodereview.appspot.com/9513006/diff/5002/content/shell/mac/ap...
content/shell/mac/app-Info.plist:21: <key>CFBundleSignature</key>
We neither need nor want a signature (or package type) here. The problem you
encountered is bug 114970. You should leave this as ???? for now and note it on
that bug so that we can remove the package type and signature once it’s fixed.

https://chromiumcodereview.appspot.com/9513006/diff/5002/content/shell/shell_...
File content/shell/shell_content_main.h (right):

https://chromiumcodereview.appspot.com/9513006/diff/5002/content/shell/shell_...
content/shell/shell_content_main.h:16: }
}  // extern "C"

(like ending a namespace)

[Avi (use Gerrit), 2012-03-05 18:26:45]

Yes, I'm looking for the four letters. The "doesn't work yet" just means I'm
moving mac content shell from utterly broken to completely broken.

https://chromiumcodereview.appspot.com/9513006/diff/5002/content/content_shel...
File content/content_shell.gypi (right):

https://chromiumcodereview.appspot.com/9513006/diff/5002/content/content_shel...
content/content_shell.gypi:293: 'unused branding',
I'm not that familiar with Python. I'd like to land this first and do this as
followup.

https://chromiumcodereview.appspot.com/9513006/diff/5002/content/shell/mac/ap...
File content/shell/mac/app-Info.plist (right):

https://chromiumcodereview.appspot.com/9513006/diff/5002/content/shell/mac/ap...
content/shell/mac/app-Info.plist:21: <key>CFBundleSignature</key>
On 2012/03/01 22:28:30, Mark Mentovai wrote:
> We neither need nor want a signature (or package type) here. The problem you
> encountered is bug 114970. You should leave this as ???? for now and note it
on
> that bug so that we can remove the package type and signature once it’s fixed.

Done.

https://chromiumcodereview.appspot.com/9513006/diff/5002/content/shell/shell_...
File content/shell/shell_content_main.h (right):

https://chromiumcodereview.appspot.com/9513006/diff/5002/content/shell/shell_...
content/shell/shell_content_main.h:16: }
On 2012/03/01 22:28:30, Mark Mentovai wrote:
> }  // extern "C"
> 
> (like ending a namespace)

Done.

[Mark Mentovai, 2012-03-06 22:10:44]

LGTM