Mac: app mode loader (shim).

Mac: app mode loader (shim).

Define an interface using which Chrome can be loaded from a minimal loader.
Produce a binary for this loader (which can be put into a bundle).

BUG=13148
TEST=not yet

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=47576

[viettrungluu, 2010-05-14 18:56:28]

Still a work in progress (obviously), but let's start getting some code in....

[Mark Mentovai, 2010-05-14 20:18:21]

I’ll likely have more comments when I take a closer look, but it’s nice to have
a direction here.

http://codereview.chromium.org/2066004/diff/4001/5001
File chrome/app/app_mode-Info.plist (right):

http://codereview.chromium.org/2066004/diff/4001/5001#newcode14
chrome/app/app_mode-Info.plist:14: <string>APP_MODE_BUNDLE_ID</string>
Bundle IDs should be registered with Apple. We’re not going to register a new
bundle ID for each app mode shortcut. I think we should leave CFBundleIdentifier
out altogether or leave it set to ????.

http://codereview.chromium.org/2066004/diff/4001/5001#newcode18
chrome/app/app_mode-Info.plist:18: <string>APP_MODE_SHORTCUT_ID</string>
What form will these take?

http://codereview.chromium.org/2066004/diff/4001/5001#newcode23
chrome/app/app_mode-Info.plist:23: <key>LSFileQuarantineEnabled</key>
Be nice. Sort your keys.

http://codereview.chromium.org/2066004/diff/4001/5001#newcode25
chrome/app/app_mode-Info.plist:25: <key>LSHasLocalizedDisplayName</key>
You can leave this out.

http://codereview.chromium.org/2066004/diff/4001/5001#newcode29
chrome/app/app_mode-Info.plist:29: <key>AppModeShortcutID</key>
ChromeAppMode? CrAppMode?

http://codereview.chromium.org/2066004/diff/4001/5001#newcode30
chrome/app/app_mode-Info.plist:30: <string>APP_MODE_SHORTCUT_ID</string>
Some traditions say to wrap your replacements in some characters to make them
less likely to be confused for something else, like @APP_MODE_SHORTCUT_ID@.

http://codereview.chromium.org/2066004/diff/4001/5002
File chrome/app/app_mode_loader_mac.mm (right):

http://codereview.chromium.org/2066004/diff/4001/5002#newcode33
chrome/app/app_mode_loader_mac.mm:33: // returns |NULL|.
How is it possible for s to not look like an NSString? This function only takes
NSStrings.

I suspect that what you wanted to do here was strdup only if the UTF8String is
not NULL?

http://codereview.chromium.org/2066004/diff/4001/5002#newcode50
chrome/app/app_mode_loader_mac.mm:50: {
What’s the new {scope} about? On a quick glance, it doesn’t seem to be
necessary. You don’t need it just for the autorelease pool action, that stuff
doesn’t rely on a {scope} (unless you rip off ScopedNSAutoreleasePool here).

http://codereview.chromium.org/2066004/diff/4001/5002#newcode51
chrome/app/app_mode_loader_mac.mm:51: // We'd rather use Cocoa, since the APIs
are a bit more convenient, but for
WE

http://codereview.chromium.org/2066004/diff/4001/5002#newcode75
chrome/app/app_mode_loader_mac.mm:75: @"Contents/Versions",
Split out Contents and Versions.

http://codereview.chromium.org/2066004/diff/4001/5002#newcode83
chrome/app/app_mode_loader_mac.mm:83: info.chrome_versioned_path =
NSStringToCString(cr_versioned_path);
For paths, you want to use fileSystemRepresentation and not UTF8String.

http://codereview.chromium.org/2066004/diff/4001/5002#newcode85
chrome/app/app_mode_loader_mac.mm:85: // Get our bundle.
OUR.

http://codereview.chromium.org/2066004/diff/4001/5002#newcode95
chrome/app/app_mode_loader_mac.mm:95: info.app_mode_id = NSStringToCString(
Blank before.

http://codereview.chromium.org/2066004/diff/4001/5002#newcode98
chrome/app/app_mode_loader_mac.mm:98: info.app_mode_short_name =
NSStringToCString(
Blank line before. Vertical space is cheap.

http://codereview.chromium.org/2066004/diff/4001/5002#newcode108
chrome/app/app_mode_loader_mac.mm:108: #if defined(GOOGLE_CHROME_BUILD)
Instead of doing this, get the app bundle’s CFBundleExecutable and append "
Framework.framework". The less hard-coded junk, the better. The fewer places we
have this stupid #ifdef, the better.

http://codereview.chromium.org/2066004/diff/4001/5002#newcode116
chrome/app/app_mode_loader_mac.mm:116: if (!cr_dylib) {
Why isn’t this one of your CHECK_MSGs?

http://codereview.chromium.org/2066004/diff/4001/5002#newcode128
chrome/app/app_mode_loader_mac.mm:128: return ChromeAppModeStart(&info);
I wonder if the strings are ever freed.

[viettrungluu, 2010-05-15 04:06:56]

http://codereview.chromium.org/2066004/diff/4001/5001
File chrome/app/app_mode-Info.plist (right):

http://codereview.chromium.org/2066004/diff/4001/5001#newcode14
chrome/app/app_mode-Info.plist:14: <string>APP_MODE_BUNDLE_ID</string>
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> Bundle IDs should be registered with Apple. We’re not going to register a new
> bundle ID for each app mode shortcut. I think we should leave
CFBundleIdentifier
> out altogether or leave it set to ????.

Done (????).

http://codereview.chromium.org/2066004/diff/4001/5001#newcode18
chrome/app/app_mode-Info.plist:18: <string>APP_MODE_SHORTCUT_ID</string>
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> What form will these take?

I plan on deriving the ID from the short name (but making sure it's file-system
safe).

http://codereview.chromium.org/2066004/diff/4001/5001#newcode23
chrome/app/app_mode-Info.plist:23: <key>LSFileQuarantineEnabled</key>
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> Be nice. Sort your keys.

Done. Probably we should go and sort our other .plist files.

http://codereview.chromium.org/2066004/diff/4001/5001#newcode25
chrome/app/app_mode-Info.plist:25: <key>LSHasLocalizedDisplayName</key>
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> You can leave this out.

Done.

http://codereview.chromium.org/2066004/diff/4001/5001#newcode29
chrome/app/app_mode-Info.plist:29: <key>AppModeShortcutID</key>
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> ChromeAppMode? CrAppMode?

Done. (CrAppMode, in case it's Chromium. And for the humour value.)

http://codereview.chromium.org/2066004/diff/4001/5001#newcode30
chrome/app/app_mode-Info.plist:30: <string>APP_MODE_SHORTCUT_ID</string>
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> Some traditions say to wrap your replacements in some characters to make them
> less likely to be confused for something else, like @APP_MODE_SHORTCUT_ID@.

Done.

http://codereview.chromium.org/2066004/diff/4001/5002
File chrome/app/app_mode_loader_mac.mm (right):

http://codereview.chromium.org/2066004/diff/4001/5002#newcode33
chrome/app/app_mode_loader_mac.mm:33: // returns |NULL|.
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> How is it possible for s to not look like an NSString? This function only
takes
> NSStrings.

Well, because -objectForKey: is kind of evil. But I replaced it with a
type-check.

> I suspect that what you wanted to do here was strdup only if the UTF8String is
> not NULL?

I should have done that too.

http://codereview.chromium.org/2066004/diff/4001/5002#newcode50
chrome/app/app_mode_loader_mac.mm:50: {
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> What’s the new {scope} about? On a quick glance, it doesn’t seem to be
> necessary. You don’t need it just for the autorelease pool action, that stuff
> doesn’t rely on a {scope} (unless you rip off ScopedNSAutoreleasePool here).

It once was a ScopedNSAutoreleasePool, which was a bad idea.

http://codereview.chromium.org/2066004/diff/4001/5002#newcode51
chrome/app/app_mode_loader_mac.mm:51: // We'd rather use Cocoa, since the APIs
are a bit more convenient, but for
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> WE

I put that in just for you.

http://codereview.chromium.org/2066004/diff/4001/5002#newcode75
chrome/app/app_mode_loader_mac.mm:75: @"Contents/Versions",
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> Split out Contents and Versions.

Done.

http://codereview.chromium.org/2066004/diff/4001/5002#newcode83
chrome/app/app_mode_loader_mac.mm:83: info.chrome_versioned_path =
NSStringToCString(cr_versioned_path);
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> For paths, you want to use fileSystemRepresentation and not UTF8String.

Done.

http://codereview.chromium.org/2066004/diff/4001/5002#newcode85
chrome/app/app_mode_loader_mac.mm:85: // Get our bundle.
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> OUR.

Done. (But what?)

http://codereview.chromium.org/2066004/diff/4001/5002#newcode95
chrome/app/app_mode_loader_mac.mm:95: info.app_mode_id = NSStringToCString(
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> Blank before.

Done.

http://codereview.chromium.org/2066004/diff/4001/5002#newcode98
chrome/app/app_mode_loader_mac.mm:98: info.app_mode_short_name =
NSStringToCString(
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> Blank line before. Vertical space is cheap.

Man, and shess was charging me a buck apiece.

http://codereview.chromium.org/2066004/diff/4001/5002#newcode108
chrome/app/app_mode_loader_mac.mm:108: #if defined(GOOGLE_CHROME_BUILD)
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> Instead of doing this, get the app bundle’s CFBundleExecutable and append "
> Framework.framework". The less hard-coded junk, the better. The fewer places
we
> have this stupid #ifdef, the better.

Done.

http://codereview.chromium.org/2066004/diff/4001/5002#newcode116
chrome/app/app_mode_loader_mac.mm:116: if (!cr_dylib) {
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> Why isn’t this one of your CHECK_MSGs?

I, uh, got bored of converting old code. I guess.

http://codereview.chromium.org/2066004/diff/4001/5002#newcode128
chrome/app/app_mode_loader_mac.mm:128: return ChromeAppModeStart(&info);
On 2010/05/14 20:18:21, Mark Mentovai wrote:
> I wonder if the strings are ever freed.

Oh noes!

[Mark Mentovai, 2010-05-17 21:56:17]

lg with these changes

http://codereview.chromium.org/2066004/diff/9001/8003
File chrome/app/app_mode_loader_mac.mm (right):

http://codereview.chromium.org/2066004/diff/9001/8003#newcode5
chrome/app/app_mode_loader_mac.mm:5: // On Mac, one can't make shortcuts with
command-line arguments. Instead, we
On the Mac, shortcuts can't have command-line arguments. Instead, produce

http://codereview.chromium.org/2066004/diff/9001/8003#newcode36
chrome/app/app_mode_loader_mac.mm:36: return [s UTF8String] ? strdup([s
UTF8String]) : NULL;
Don’t call -UTF8String twice. Use a temporary. They’re nearly as cheap as
vertical whitespace.

http://codereview.chromium.org/2066004/diff/9001/8003#newcode44
chrome/app/app_mode_loader_mac.mm:44: return [s fileSystemRepresentation] ?
strdup([s fileSystemRepresentation]) :
Likewise.

http://codereview.chromium.org/2066004/diff/9001/8003#newcode50
chrome/app/app_mode_loader_mac.mm:50: int main(int argc, char** argv) {
__attribute__((visibility("default")))

http://codereview.chromium.org/2066004/diff/9001/8003#newcode57
chrome/app/app_mode_loader_mac.mm:57: // The Cocoa APIs are a bit more
convenient; for an autorelease pool is
for this

http://codereview.chromium.org/2066004/diff/9001/8003#newcode103
chrome/app/app_mode_loader_mac.mm:103: info.app_mode_id = NSStringToFSCString(
Should this use the UTF-8 getter? It’s not immediately apparent if app_mode_id
is eventually used as a pathname component.

http://codereview.chromium.org/2066004/diff/9001/8003#newcode128
chrome/app/app_mode_loader_mac.mm:128: void* cr_dylib =
dlopen([cr_framework_path UTF8String], RTLD_LAZY);
Not -UTF8String.

http://codereview.chromium.org/2066004/diff/9001/8003#newcode138
chrome/app/app_mode_loader_mac.mm:138: return ChromeAppModeStart(&info);
int rv = ChromeAppModeStart(&info); and exit(rv). See
chrome/app/chrome_exe_main.mm for the explanation.

http://codereview.chromium.org/2066004/diff/9001/8004
File chrome/app/chrome_dll_app_mode_mac.mm (right):

http://codereview.chromium.org/2066004/diff/9001/8004#newcode33
chrome/app/chrome_dll_app_mode_mac.mm:33: return -1;
-1? Probably want just 1.

http://codereview.chromium.org/2066004/diff/9001/8004#newcode36
chrome/app/chrome_dll_app_mode_mac.mm:36: RAW_LOG(ERROR, "Browser Framework too
old to.");
Too old to what? Too old to care?

http://codereview.chromium.org/2066004/diff/9001/8004#newcode48
chrome/app/chrome_dll_app_mode_mac.mm:48: char argv1[1000] = "--app=";
This is C++, you know. You can use std::string and its .c_str() to build these.
It’s still a cheesey hack, but it at least relieves you of “managing” buffers
this way.

http://codereview.chromium.org/2066004/diff/9001/8008
File chrome/common/app_mode_common_mac.h (right):

http://codereview.chromium.org/2066004/diff/9001/8008#newcode48
chrome/common/app_mode_common_mac.h:48: // Short ID string, preferably derived
from |app_mode_short_name|. Should be
Whitespace costs what, now?

[Mark Mentovai, 2010-05-17 22:02:42]

http://codereview.chromium.org/2066004/diff/9001/8003
File chrome/app/app_mode_loader_mac.mm (right):

http://codereview.chromium.org/2066004/diff/9001/8003#newcode16
chrome/app/app_mode_loader_mac.mm:16: #include <Cocoa/Cocoa.h>
#import <Foundation/Foundation.h>

http://codereview.chromium.org/2066004/diff/9001/8003#newcode129
chrome/app/app_mode_loader_mac.mm:129: CHECK_MSG(cr_dylib, "couldn't load
browser framework");
and "browser" is sort of a misnomer.