Mac app mode: locate Chrome + refactor

Mac app mode: locate Chrome + refactor

* Add code to locate the Chrome bundle + unit tests.
* Refactor app_mode_app to use the new code, also now we can use stuff from base, remove hacks from the code.

BUG=None
TEST=Unit tests should pass.


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

[jeremy, 2012-02-07 14:13:56]

Mark: base/file_util.h
sail: Everything else.

Please note that app_mode_loader_mac.mm is still in an interim state, there's a
bunch more work to be done on it but in the interests of keeping this CL small
I'm separating that work out.

[jeremy, 2012-02-07 14:15:07]

avi: Owner review for content/common/sandbox_mac_diraccess_unittest.mm .

[sail, 2012-02-07 17:13:26]

LGTM!

[Mark Mentovai, 2012-02-07 17:28:06]

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/app/app_mode_...
File chrome/app/app_mode_loader_mac.mm (right):

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/app/app_mode_...
chrome/app/app_mode_loader_mac.mm:33: NSBundle* app_bundle = [NSBundle
mainBundle];
Didn’t you add your own functions to wrap this?

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/app/app_mode_...
chrome/app/app_mode_loader_mac.mm:46: [(NSString*)CFPreferencesCopyAppValue(
Don’t cast NS-to-CF like this. Use NSToCFCast<> and CFToNSCast<> from
base/mac/foundation_util.h.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/app/app_mode_...
chrome/app/app_mode_loader_mac.mm:72: (void)raw_version_str;
?

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/app/app_mode_...
chrome/app/app_mode_loader_mac.mm:85: [info_plist
objectForKey:@"CrAppModeShortcutID"]);
Do we not have symbolic constants for any of these?

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/app/app_mode_...
chrome/app/app_mode_loader_mac.mm:100: RTLD_LAZY);
This didn’t need to wrap.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/app/app_mode_...
chrome/app/app_mode_loader_mac.mm:101: CHECK(cr_dylib) << "couldn't load
framework";
Print some more useful information from dlerror()?

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
File chrome/common/mac/app_mode_chrome_locator.mm (right):

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_chrome_locator.mm:14: 
Who calls these functions? They may need their own autorelease pools.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_chrome_locator.mm:17:
absolutePathForAppBundleWithIdentifier:bundle_id];
Weird indentation.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_chrome_locator.mm:53: stringByStandardizingPath];
Do you need -stringByStandardizingPath? Don’t think so.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_chrome_locator.mm:84: 
Get rid of the blank line.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
File chrome/common/mac/app_mode_common.h (right):

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_common.h:37: struct ChromeAppModeInfo {
Should this be a class now?

Should you provide getters and setters instead of allowing unrestricted data
access?

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_common.h:40: virtual ~ChromeAppModeInfo();
There doesn’t appear to be any need for this to be virtual.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_common.h:47: int argc;  // Required: v1.0
Should these two be a single std::vector<std::string> now?

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
File chrome/common/mac/app_mode_common.mm (right):

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_common.mm:12: ChromeAppModeInfo::ChromeAppModeInfo()
{
Deterministically initialize the POD fields?

[Mark Mentovai, 2012-02-07 17:35:12]

sail@chromium.org wrote:
> LGTM!

I don’t mean to pick on either of you, and I don’t mean for this
comment to undercut the work that you’re doing, which I think is
stellar. I’m just wondering how it is that I had many questions based
on a quick pass, and all Sailesh had was “LGTM!” Indeed, the overall
work looks very good to me too, but I hope we’re not sacrificing
anything by applying quick rubber-stamps to each other’s code because
we’re excited about the progress. I encourage all reviewers to treat
coworkers’ work with the respect it deserves by, at the bare minimum,
giving it a careful and thoughtful read and asking questions during
the review process.

[sail, 2012-02-07 17:43:27]

> I’m just wondering how it is that I had many questions based
> on a quick pass, and all Sailesh had was “LGTM!”

Argh, sorry about that. Will take a second pass. Thanks!

[Mark Mentovai, 2012-02-07 19:36:42]

https://chromiumcodereview.appspot.com/9351014/diff/1001/base/file_util.h
File base/file_util.h (right):

https://chromiumcodereview.appspot.com/9351014/diff/1001/base/file_util.h#new...
base/file_util.h:441: // A class to handle auto-deleting a directory.
Would ScopedTempDir (base/scoped_temp_dir.h) work for your purposes?

[Avi (use Gerrit), 2012-02-07 20:07:17]

sandbox_mac_diraccess_unittest.mm LGTM provided you use ScopedTempDir as noted
by Mark.

[jeremy, 2012-02-09 10:52:51]

All fixed, ready for another look.

https://chromiumcodereview.appspot.com/9351014/diff/1001/base/file_util.h
File base/file_util.h (right):

https://chromiumcodereview.appspot.com/9351014/diff/1001/base/file_util.h#new...
base/file_util.h:441: // A class to handle auto-deleting a directory.
On 2012/02/07 19:36:42, Mark Mentovai wrote:
> Would ScopedTempDir (base/scoped_temp_dir.h) work for your purposes?

Done.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/app/app_mode_...
File chrome/app/app_mode_loader_mac.mm (right):

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/app/app_mode_...
chrome/app/app_mode_loader_mac.mm:33: NSBundle* app_bundle = [NSBundle
mainBundle];
Yes, but this code is called from the stub launcher and we really do want to use
[- mainBundle] here.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/app/app_mode_...
chrome/app/app_mode_loader_mac.mm:46: [(NSString*)CFPreferencesCopyAppValue(
On 2012/02/07 17:28:06, Mark Mentovai wrote:
> Don’t cast NS-to-CF like this. Use NSToCFCast<> and CFToNSCast<> from
> base/mac/foundation_util.h.

Done.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/app/app_mode_...
chrome/app/app_mode_loader_mac.mm:72: (void)raw_version_str;
Removed for now, the idea was to mark that we need to have a version check to
make sure the app shortcut doesn't try to launch a version of Chrome which is
too old to support .

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/app/app_mode_...
chrome/app/app_mode_loader_mac.mm:85: [info_plist
objectForKey:@"CrAppModeShortcutID"]);
Sail has a patch ready to land that does that.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/app/app_mode_...
chrome/app/app_mode_loader_mac.mm:100: RTLD_LAZY);
On 2012/02/07 17:28:06, Mark Mentovai wrote:
> This didn’t need to wrap.

Done.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/app/app_mode_...
chrome/app/app_mode_loader_mac.mm:101: CHECK(cr_dylib) << "couldn't load
framework";
On 2012/02/07 17:28:06, Mark Mentovai wrote:
> Print some more useful information from dlerror()?

Done.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
File chrome/common/mac/app_mode_chrome_locator.mm (right):

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_chrome_locator.mm:14: 
They are only called by the app_loader_mac.mm code and the unit tests, I'll add
autorelease pools to the unit tests.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_chrome_locator.mm:17:
absolutePathForAppBundleWithIdentifier:bundle_id];
On 2012/02/07 17:28:06, Mark Mentovai wrote:
> Weird indentation.

Done.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_chrome_locator.mm:53: stringByStandardizingPath];
On 2012/02/07 17:28:06, Mark Mentovai wrote:
> Do you need -stringByStandardizingPath? Don’t think so.

Done.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_chrome_locator.mm:84: 
On 2012/02/07 17:28:06, Mark Mentovai wrote:
> Get rid of the blank line.

Done.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
File chrome/common/mac/app_mode_common.h (right):

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_common.h:37: struct ChromeAppModeInfo {
This code is due for a big refactor, and I'll likely remove this struct or
change it pretty heavily.  Hope it's ok to keep as is for now.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_common.h:40: virtual ~ChromeAppModeInfo();
On 2012/02/07 17:28:06, Mark Mentovai wrote:
> There doesn’t appear to be any need for this to be virtual.

Done.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_common.h:47: int argc;  // Required: v1.0
See comment above.

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
File chrome/common/mac/app_mode_common.mm (right):

https://chromiumcodereview.appspot.com/9351014/diff/1001/chrome/common/mac/ap...
chrome/common/mac/app_mode_common.mm:12: ChromeAppModeInfo::ChromeAppModeInfo()
{
On 2012/02/07 17:28:06, Mark Mentovai wrote:
> Deterministically initialize the POD fields?

Done.

[sail, 2012-02-09 23:31:37]

Second LGTM!

A couple of NSString to FilePath and FilePath to NSString conversions can be
replaced by the base/mac/foundation_util.h functions I added. I'll ping you when
that lands (sometime today hopefully).

http://codereview.chromium.org/9351014/diff/6001/chrome/app/app_mode_loader_m...
File chrome/app/app_mode_loader_mac.mm (right):

http://codereview.chromium.org/9351014/diff/6001/chrome/app/app_mode_loader_m...
chrome/app/app_mode_loader_mac.mm:51: cr_bundle_path_ns &&
file_util::DirectoryExists(cr_bundle_path);
this would be easier to read as:
  !cr_bundle_path.empty() && ...

http://codereview.chromium.org/9351014/diff/6001/chrome/app/app_mode_loader_m...
chrome/app/app_mode_loader_mac.mm:54: // If no such bundle path exists, try to
search by bundle ID.
since this is a very unusual case can we do a LOG(ERROR) here?

http://codereview.chromium.org/9351014/diff/6001/chrome/common/mac/app_mode_c...
File chrome/common/mac/app_mode_chrome_locator.h (right):

http://codereview.chromium.org/9351014/diff/6001/chrome/common/mac/app_mode_c...
chrome/common/mac/app_mode_chrome_locator.h:21: bool FindBundleById(NSString*
bundle_id, FilePath* out_bundle);
Could you add a comment that normally we don't use this to find the bundle.
Even better would be to move the entire find code into one function.

http://codereview.chromium.org/9351014/diff/6001/chrome/common/mac/app_mode_c...
File chrome/common/mac/app_mode_chrome_locator.mm (right):

http://codereview.chromium.org/9351014/diff/6001/chrome/common/mac/app_mode_c...
chrome/common/mac/app_mode_chrome_locator.mm:40: [cr_bundle
objectForInfoDictionaryKey:@"CFBundleShortVersionString"];
use base::mac::ObjCCast?

http://codereview.chromium.org/9351014/diff/6001/chrome/common/mac/app_mode_c...
chrome/common/mac/app_mode_chrome_locator.mm:56: [cr_bundle
objectForInfoDictionaryKey:@"CFBundleExecutable"];
same (base::mac::ObjCCast)

[commit-bot: I haz the power, 2012-02-13 10:14:48]

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

[commit-bot: I haz the power, 2012-02-13 11:49:18]

Change committed as 121693