Mac: Generate App Mode Loader bundle + cleanup

Mac: Generate App Mode Loader bundle + cleanup

* Revised the .gyp files so that the app mode loader is built as a bundle "template".
* Add new data to loader Info.plist and fill it in.
* Improve unit tests and disable them for now (fix will be in a followup CL).
* Various small cleanup tasks all over Mac app mode loader code.

BUG=112651
TEST=Covered by existing tests (which when enabled, will test this).


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

[jeremy, 2012-02-16 12:55:34]

Mark: Do the gyp changes look sane to you?
Sail: Everything else.

https://chromiumcodereview.appspot.com/9416012/diff/1/chrome/app/app_mode-Inf...
File chrome/app/app_mode-Info.plist (right):

https://chromiumcodereview.appspot.com/9416012/diff/1/chrome/app/app_mode-Inf...
chrome/app/app_mode-Info.plist:10: <string>????</string>
If we don't include this field then the ninja build breaks.

https://chromiumcodereview.appspot.com/9416012/diff/1/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app_mac_unittest.mm (right):

https://chromiumcodereview.appspot.com/9416012/diff/1/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_mac_unittest.mm:45:
TEST(WebAppShortcutCreatorTest, DISABLED_CreateShortcut) {
Sorry I'm not fixing the test as part of this CL but I didn't want the CL to
grow past it's current size.

[Nico, 2012-02-16 15:57:23]

https://chromiumcodereview.appspot.com/9416012/diff/1/chrome/app/app_mode-Inf...
File chrome/app/app_mode-Info.plist (right):

https://chromiumcodereview.appspot.com/9416012/diff/1/chrome/app/app_mode-Inf...
chrome/app/app_mode-Info.plist:10: <string>????</string>
On 2012/02/16 12:55:34, jeremy wrote:
> If we don't include this field then the ninja build breaks.

Can you instead file a bug on me to get this fixed? Please don't work around
ninja/make build bugs. (Make should have the same problem.)

[Mark Mentovai, 2012-02-16 16:24:46]

Nico beat me to the punch with this.
On Feb 16, 2012 10:57 AM, <thakis@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/9416012/diff/1/**
>
chrome/app/app_mode-Info.plist<https://chromiumcodereview.appspot.com/9416012/diff/1/chrome/app/app_mode-Info.plist>
> File chrome/app/app_mode-Info.plist (right):
>
> https://chromiumcodereview.**appspot.com/9416012/diff/1/**
>
chrome/app/app_mode-Info.**plist#newcode10<https://chromiumcodereview.appspot.com/9416012/diff/1/chrome/app/app_mode-Info.plist#newcode10>
> chrome/app/app_mode-Info.**plist:10: <string>????</string>
> On 2012/02/16 12:55:34, jeremy wrote:
>
>> If we don't include this field then the ninja build breaks.
>>
>
> Can you instead file a bug on me to get this fixed? Please don't work
> around ninja/make build bugs. (Make should have the same problem.)
>
>
https://chromiumcodereview.**appspot.com/9416012/<https://chromiumcodereview....
>

[sail, 2012-02-17 16:42:09]

http://codereview.chromium.org/9416012/diff/1/chrome/app/app_mode_loader_mac.mm
File chrome/app/app_mode_loader_mac.mm (right):

http://codereview.chromium.org/9416012/diff/1/chrome/app/app_mode_loader_mac....
chrome/app/app_mode_loader_mac.mm:42: NSString* cr_bundle_id = [app_bundle
this should probably use base::mac::ObjCCast<NSString>

http://codereview.chromium.org/9416012/diff/1/chrome/app/app_mode_loader_mac....
chrome/app/app_mode_loader_mac.mm:97: CHECK(info->app_mode_url.size()) <<
"couldn't get app shortcut URL";
I think you can remove this. URL is optional for platform apps

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
File chrome/browser/web_applications/web_app_mac.h (right):

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
chrome/browser/web_applications/web_app_mac.h:28: bool CreateShortcut(NSString*
chrome_bundle_id);
Instead of passing the bundle ID to create shortcut it would be better to pass
it to the constructor and save it in a member variable. If you do that it would
also be better to use a string16 instead of a Objective-C object.

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
File chrome/browser/web_applications/web_app_mac.mm (right):

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
chrome/browser/web_applications/web_app_mac.mm:86: NSString* const
kCFBundleIdentifer = @"CFBundleIdentifier";
you could also use kCFBundleIdentifierKey here

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
chrome/browser/web_applications/web_app_mac.mm:100:
stringByReplacingOccurrencesOfString:app_mode::kShortcutIdPlaceholder
this function is becoming a bit long, could you change it to something like
this:
  [dict setObject:GetBundleIdentifier()
               forKey:kCFBundleIdentifierKey];

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
chrome/browser/web_applications/web_app_mac.mm:132:
shortcut_creator.CreateShortcut([chrome::OuterAppBundle() bundleIdentifier]);
bug 24842 seems to imply that NSBundle isn't thread safe

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
File chrome/browser/web_applications/web_app_mac_unittest.mm (right):

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
chrome/browser/web_applications/web_app_mac_unittest.mm:78:
TEST(WebAppShortcutCreatorTest, DISABLED_CreateFailure) {
Is this failing now? If so can you put a comment about why it's failing?

[jeremy, 2012-02-19 14:36:41]

All done, ready for another look.

http://codereview.chromium.org/9416012/diff/1/chrome/app/app_mode_loader_mac.mm
File chrome/app/app_mode_loader_mac.mm (right):

http://codereview.chromium.org/9416012/diff/1/chrome/app/app_mode_loader_mac....
chrome/app/app_mode_loader_mac.mm:42: NSString* cr_bundle_id = [app_bundle
On 2012/02/17 16:42:09, sail wrote:
> this should probably use base::mac::ObjCCast<NSString>

Done.

http://codereview.chromium.org/9416012/diff/1/chrome/app/app_mode_loader_mac....
chrome/app/app_mode_loader_mac.mm:97: CHECK(info->app_mode_url.size()) <<
"couldn't get app shortcut URL";
On 2012/02/17 16:42:09, sail wrote:
> I think you can remove this. URL is optional for platform apps

Done.

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
File chrome/browser/web_applications/web_app_mac.h (right):

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
chrome/browser/web_applications/web_app_mac.h:28: bool CreateShortcut(NSString*
chrome_bundle_id);
On 2012/02/17 16:42:09, sail wrote:
> Instead of passing the bundle ID to create shortcut it would be better to pass
> it to the constructor and save it in a member variable. If you do that it
would
> also be better to use a string16 instead of a Objective-C object.

Done.

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
File chrome/browser/web_applications/web_app_mac.mm (right):

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
chrome/browser/web_applications/web_app_mac.mm:86: NSString* const
kCFBundleIdentifer = @"CFBundleIdentifier";
On 2012/02/17 16:42:09, sail wrote:
> you could also use kCFBundleIdentifierKey here

Done.

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
chrome/browser/web_applications/web_app_mac.mm:100:
stringByReplacingOccurrencesOfString:app_mode::kShortcutIdPlaceholder
On 2012/02/17 16:42:09, sail wrote:
> this function is becoming a bit long, could you change it to something like
> this:
>   [dict setObject:GetBundleIdentifier()
>                forKey:kCFBundleIdentifierKey];

Done.

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
chrome/browser/web_applications/web_app_mac.mm:132:
shortcut_creator.CreateShortcut([chrome::OuterAppBundle() bundleIdentifier]);
Good catch! Thanks!

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
File chrome/browser/web_applications/web_app_mac_unittest.mm (right):

http://codereview.chromium.org/9416012/diff/1/chrome/browser/web_applications...
chrome/browser/web_applications/web_app_mac_unittest.mm:78:
TEST(WebAppShortcutCreatorTest, DISABLED_CreateFailure) {
You're right, reverted.

[sail, 2012-02-19 21:08:54]

some small nits but otherwise LGTM!

http://codereview.chromium.org/9416012/diff/7004/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app_mac.mm (right):

http://codereview.chromium.org/9416012/diff/7004/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:93: [dict
setObject:GetBundleIdentifier(base::mac::ObjCCast<NSDictionary>(dict))
you don't need a base::mac::ObjCCast() here since NSMutableDictionary derives
from NSDictionary

http://codereview.chromium.org/9416012/diff/7004/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:95: NSString* extension_id =
base::SysUTF8ToNSString(info_.extension_id);
you don't need this temp variable

http://codereview.chromium.org/9416012/diff/7004/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:114: NSString* bundle_id_template
=
I didn't know about BaseBundleID(). That's very cool. If you wanted you could
simplify things by just appending extension_id to BaseBundleID().

http://codereview.chromium.org/9416012/diff/7004/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/9416012/diff/7004/chrome/chrome.gyp#newcode89
chrome/chrome.gyp:89: 'app_mode_app_bundle_id':
'com.google.Chrome.app.@APP_MODE_SHORTCUT_ID@',
as above, it might be simpler to get rid of this and simply append extension ID
to BaseBundleID()

[jeremy, 2012-02-20 05:41:00]

Mark: I still need an owner LGTM, thanks!

http://codereview.chromium.org/9416012/diff/7004/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app_mac.mm (right):

http://codereview.chromium.org/9416012/diff/7004/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:93: [dict
setObject:GetBundleIdentifier(base::mac::ObjCCast<NSDictionary>(dict))
On 2012/02/19 21:08:55, sail wrote:
> you don't need a base::mac::ObjCCast() here since NSMutableDictionary derives
> from NSDictionary

Done.

http://codereview.chromium.org/9416012/diff/7004/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:95: NSString* extension_id =
base::SysUTF8ToNSString(info_.extension_id);
On 2012/02/19 21:08:55, sail wrote:
> you don't need this temp variable

Done.

http://codereview.chromium.org/9416012/diff/7004/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:114: NSString* bundle_id_template
=
I prefer to keep it this way since:
1. It's easier to see from the Info.plist that this isn't the final bundle
identifier.
2. We can detect whether the field was properly filled in from the unit tests.

[sail, 2012-02-20 21:17:49]

http://codereview.chromium.org/9416012/diff/13002/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app_mac.h (right):

http://codereview.chromium.org/9416012/diff/13002/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_mac.h:24: explicit
WebAppShortcutCreator(
This doesn't need to be explicit anymore.

[Mark Mentovai, 2012-02-22 16:43:22]

This mostly looks good. I’ll want to look at a sample .app that this creates.

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/app/app_mode...
File chrome/app/app_mode-Info.plist (right):

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/app/app_mode...
chrome/app/app_mode-Info.plist:9: <key>CFBundleSignature</key>
Do we need to include this at all? I’d prefer not including it, and I think we
can get away with not including it.

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.h (right):

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.h:13: @class NSDictionary;
This file looks like it could reasonably be #included by a C++ (not Objective-C)
file. We should take care to do:

#ifdef __OBJC__
@class NSDictionary;
@class NSString;
#else  // __OBJC__
class NSDictionary;
class NSString;
#endif  // __OBJC__

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:116: NSString* extension_id =
base::SysUTF8ToNSString(info_.extension_id);
What do these look like? How do we know they’re valid for use as a bundle ID?
How do we know they only contain good characters? Do we want to ensure that
everything we output here is prefixed by something?

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac_unittest.mm (right):

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac_unittest.mm:71: if (![value
isKindOfClass:[NSString class]])
Use ObjCCast<NSString>.

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/chrome.gyp#n...
chrome/chrome.gyp:89: 'app_mode_app_bundle_id':
'com.google.Chrome.app.@APP_MODE_SHORTCUT_ID@',
I think you can just move this out one level to be a sibling to 'conditions' and
'tweak_info_plist_path' and write:

'app_mode_app_bundle_id': '<(mac_bundle_id).@APP_MODE_SHORTCUT_ID@',

Give it a try.

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/chrome.gyp#n...
chrome/chrome.gyp:865: 'app/app_mode-Info.plist',
Is this needed here?

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/chrome.gyp#n...
chrome/chrome.gyp:884: 'APP_MODE_APP_BUNDLE_ID': '<(app_mode_app_bundle_id)',
It would also be reasonable to just write
'<(mac_bundle_id).@APP_MODE_SHORTCUT_ID@' here and not have a separate variable
for it, since it’s only used in one location.

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/chrome.gyp#n...
chrome/chrome.gyp:892: # placed into the helper.
Not helper, but app mode loader.

[Mark Mentovai, 2012-02-22 18:56:12]

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/app/app_mode...
File chrome/app/app_mode-Info.plist (right):

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/app/app_mode...
chrome/app/app_mode-Info.plist:16: <string>${APP_MODE_APP_BUNDLE_ID}</string>
In the sample that you sent me, I see

        <key>CFBundleIdentifier</key>
    <string>com.google.Chromium.app.@kpgejacgnpnommlmkfagpccmcfkikdol@</string>

The @ signs in there are incorrect.

[Mark Mentovai, 2012-02-22 18:57:02]

Even stripped down, the app_mode_loader binary is 250kB. That’s larger than I
was expecting. Is it what you’r expecting?

[sail, 2012-02-22 19:02:04]

On 2012/02/22 18:57:02, Mark Mentovai wrote:
> Even stripped down, the app_mode_loader binary is 250kB. That’s larger than I
> was expecting. Is it what you’r expecting?

This is for a debug build. I think the release build is much smaller.

I'm doing a release build right now. I'll send you a copy of the binary shortly.

[sail, 2012-02-22 19:02:05]

On 2012/02/22 18:57:02, Mark Mentovai wrote:
> Even stripped down, the app_mode_loader binary is 250kB. That’s larger than I
> was expecting. Is it what you’r expecting?

This is for a debug build. I think the release build is much smaller.

I'm doing a release build right now. I'll send you a copy of the binary shortly.

[jeremy, 2012-02-23 14:21:13]

Fixed, ready for another look

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/app/app_mode...
File chrome/app/app_mode-Info.plist (right):

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/app/app_mode...
chrome/app/app_mode-Info.plist:9: <key>CFBundleSignature</key>
If I don't want to break the build with this CL then I do - see crbug.com/114970
.

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/app/app_mode...
chrome/app/app_mode-Info.plist:16: <string>${APP_MODE_APP_BUNDLE_ID}</string>
On 2012/02/22 18:56:12, Mark Mentovai wrote:
> In the sample that you sent me, I see
> 
>         <key>CFBundleIdentifier</key>
>    
<string>com.google.Chromium.app.@kpgejacgnpnommlmkfagpccmcfkikdol@</string>
> 
> The @ signs in there are incorrect.

Done.

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.h (right):

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.h:13: @class NSDictionary;
On 2012/02/22 16:43:22, Mark Mentovai wrote:
> This file looks like it could reasonably be #included by a C++ (not
Objective-C)
> file. We should take care to do:
> 
> #ifdef __OBJC__
> @class NSDictionary;
> @class NSString;
> #else  // __OBJC__
> class NSDictionary;
> class NSString;
> #endif  // __OBJC__

Done.

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:116: NSString* extension_id =
base::SysUTF8ToNSString(info_.extension_id);
I've added a TODO, I'd like to land this CL since it's blocking a bunch of other
changes.  I'll take care of this as a followup if that's ok.

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac_unittest.mm (right):

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac_unittest.mm:71: if (![value
isKindOfClass:[NSString class]])
On 2012/02/22 16:43:22, Mark Mentovai wrote:
> Use ObjCCast<NSString>.

Done.

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/chrome.gyp#n...
chrome/chrome.gyp:89: 'app_mode_app_bundle_id':
'com.google.Chrome.app.@APP_MODE_SHORTCUT_ID@',
On 2012/02/22 16:43:22, Mark Mentovai wrote:
> I think you can just move this out one level to be a sibling to 'conditions'
and
> 'tweak_info_plist_path' and write:
> 
> 'app_mode_app_bundle_id': '<(mac_bundle_id).@APP_MODE_SHORTCUT_ID@',
> 
> Give it a try.

Done.

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/chrome.gyp#n...
chrome/chrome.gyp:884: 'APP_MODE_APP_BUNDLE_ID': '<(app_mode_app_bundle_id)',
On 2012/02/22 16:43:22, Mark Mentovai wrote:
> It would also be reasonable to just write
> '<(mac_bundle_id).@APP_MODE_SHORTCUT_ID@' here and not have a separate
variable
> for it, since it’s only used in one location.

Done.

https://chromiumcodereview.appspot.com/9416012/diff/13003/chrome/chrome.gyp#n...
chrome/chrome.gyp:892: # placed into the helper.
On 2012/02/22 16:43:22, Mark Mentovai wrote:
> Not helper, but app mode loader.

Done.

[Mark Mentovai, 2012-02-23 17:46:02]

LGTM with two easy changes now plus one change to be made in the future.

https://chromiumcodereview.appspot.com/9416012/diff/20001/chrome/app/app_mode...
File chrome/app/app_mode-Info.plist (right):

https://chromiumcodereview.appspot.com/9416012/diff/20001/chrome/app/app_mode...
chrome/app/app_mode-Info.plist:9: <key>CFBundleSignature</key>
Provided that you remove this as soon as bug 114970 is fixed.

https://chromiumcodereview.appspot.com/9416012/diff/20001/chrome/browser/web_...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9416012/diff/20001/chrome/browser/web_...
chrome/browser/web_applications/web_app_mac.mm:116: // TODO(jeremy): Make sure
that the extension ID consists of characters that
You can just remove this comment now, as you’ve resolved bug 115470.

https://chromiumcodereview.appspot.com/9416012/diff/20001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://chromiumcodereview.appspot.com/9416012/diff/20001/chrome/chrome.gyp#n...
chrome/chrome.gyp:882: 'APP_MODE_APP_BUNDLE_ID':
'<(mac_bundle_id).@APP_MODE_SHORTCUT_ID@',
To be safe, we should put all of these into their own namespace. Use

'<(mac_bundle_id).app.@APP_MODE_SHORTCUT_ID@'

[commit-bot: I haz the power, 2012-02-28 12:21:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/9416012/24001

[commit-bot: I haz the power, 2012-02-28 14:04:09]

Try job failure for 9416012-24001 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-28 14:31:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/9416012/27002

[commit-bot: I haz the power, 2012-02-28 17:09:43]

Try job failure for 9416012-27002 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-02-29 06:11:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/9416012/37001

[commit-bot: I haz the power, 2012-02-29 08:08:24]

Change committed as 124151