Use the shim bundle's user_data_dir instead of GetUserDataDirectoryForBrowserBundle.

Use the shim bundle's user_data_dir instead of GetUserDataDirectoryForBrowserBundle.

Currently app shims have no support for --user-data-dir, and can only ever
connect to the default user_data_dir for a particular Chrome product (e.g.
Canary/Chromium).

This allows shims made by Chrome running in a different user_data_dir to
start and connect with the correct Chrome.

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

[tapted, 2013-07-17 08:34:21]

maybe a "motivation" comment in the CL description as well, since there's no
specific bug.

E.g.

"Currently app shims have no support for --user-data-dir, and can only ever
connect to the default user data dir for a particular Chrome product (e.g.
Canary/Chromium)".

https://codereview.chromium.org/19503003/diff/1/apps/app_shim/chrome_main_app...
File apps/app_shim/chrome_main_app_mode_mac.mm (left):

https://codereview.chromium.org/19503003/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:104: if
(!chrome::GetUserDataDirectoryForBrowserBundle(chrome_bundle,
I started wondering whether this approach was chosen because it gives a stronger
guarantee that "this" running chrome_main_app_mode_mac.mm process, and the
Chrome process it tries to connect to, will come from the same release.

E.g. if the IPC protocol changes between releases, does this change increase the
chance that it could find and connect to a Chrome talking a different language.

But I think that's actually not the case.

https://codereview.chromium.org/19503003/diff/1/apps/app_shim/chrome_main_app...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/19503003/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:99: if (g_info->user_data_dir.empty())
{
Can a shim ever be created like this? Maybe just CHECK, and ditch the early
Close().

https://codereview.chromium.org/19503003/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:105: base::FilePath maybe_profile_dir
=
Perhaps a helper function, which describes the logic here in some comments..

https://codereview.chromium.org/19503003/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:107: if (maybe_profile_dir ==
g_info->profile_dir)
What happens if someone's Chrome user data dir is two levels below some random
folder called 'Default'?

[jackhou1, 2013-07-30 07:34:33]

https://codereview.chromium.org/19503003/diff/1/apps/app_shim/chrome_main_app...
File apps/app_shim/chrome_main_app_mode_mac.mm (left):

https://codereview.chromium.org/19503003/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:104: if
(!chrome::GetUserDataDirectoryForBrowserBundle(chrome_bundle,
On 2013/07/17 08:34:21, tapted wrote:
> I started wondering whether this approach was chosen because it gives a
stronger
> guarantee that "this" running chrome_main_app_mode_mac.mm process, and the
> Chrome process it tries to connect to, will come from the same release.
> 
> E.g. if the IPC protocol changes between releases, does this change increase
the
> chance that it could find and connect to a Chrome talking a different
language.
> 
> But I think that's actually not the case.

The Info.plist has Chrome's bundle id, which app_mode_loader uses to find the
framework bundle. So I think you're guaranteed to use the same version is if
there's only one app bundle with that id, and you don't share user_data_dirs
between different app bundles.

https://codereview.chromium.org/19503003/diff/1/apps/app_shim/chrome_main_app...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/19503003/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:99: if (g_info->user_data_dir.empty())
{
On 2013/07/17 08:34:21, tapted wrote:
> Can a shim ever be created like this? Maybe just CHECK, and ditch the early
> Close().

Done.

https://codereview.chromium.org/19503003/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:105: base::FilePath maybe_profile_dir
=
On 2013/07/17 08:34:21, tapted wrote:
> Perhaps a helper function, which describes the logic here in some comments..

Since we're not changing the user_data_dir field, this logic is much simpler.

https://codereview.chromium.org/19503003/diff/1/apps/app_shim/chrome_main_app...
apps/app_shim/chrome_main_app_mode_mac.mm:107: if (maybe_profile_dir ==
g_info->profile_dir)
On 2013/07/17 08:34:21, tapted wrote:
> What happens if someone's Chrome user data dir is two levels below some random
> folder called 'Default'?

Obsolete since we're leaving the user_data_dir unchanged for now.

[tapted, 2013-07-30 07:47:16]

lgtm. We should rename app_mode::ChromeAppModeInfo::user_data_dir, but I think
there are other cleanups to do in there, so good to do orthogonally in a
follow-up.

[jackhou1, 2013-07-30 12:28:43]

benwells, please review apps/DEPS

[benwells, 2013-07-31 01:16:03]

lgtm

[commit-bot: I haz the power, 2013-07-31 01:37:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/19503003/10001

[commit-bot: I haz the power, 2013-07-31 01:37:23]

Failed to apply patch for apps/app_shim/chrome_main_app_mode_mac.mm:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file apps/app_shim/chrome_main_app_mode_mac.mm
  Hunk #1 succeeded at 25 with fuzz 2 (offset 2 lines).
  Hunk #2 FAILED at 96.
  1 out of 2 hunks FAILED -- saving rejects to file
apps/app_shim/chrome_main_app_mode_mac.mm.rej

Patch:       apps/app_shim/chrome_main_app_mode_mac.mm
Index: apps/app_shim/chrome_main_app_mode_mac.mm
diff --git a/apps/app_shim/chrome_main_app_mode_mac.mm
b/apps/app_shim/chrome_main_app_mode_mac.mm
index
6aa92c213284e1e0a33fb283be6df7f00b288238..0c1cfb9017327d6b8cf8f3e1e85b87cb8d96b28d
100644
--- a/apps/app_shim/chrome_main_app_mode_mac.mm
+++ b/apps/app_shim/chrome_main_app_mode_mac.mm
@@ -23,7 +23,6 @@
 #include "base/strings/sys_string_conversions.h"
 #include "base/threading/thread.h"
 #include "chrome/common/chrome_paths.h"
-#include "chrome/common/chrome_paths_internal.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/mac/app_mode_common.h"
 #include "ipc/ipc_channel_proxy.h"
@@ -97,15 +96,11 @@ AppShimController::AppShimController() : channel_(NULL),
 
 void AppShimController::Init() {
   DCHECK(g_io_thread);
-  NSString* chrome_bundle_path =
-      base::SysUTF8ToNSString(g_info->chrome_outer_bundle_path.value());
-  NSBundle* chrome_bundle = [NSBundle bundleWithPath:chrome_bundle_path];
-  base::FilePath user_data_dir;
-  if (!chrome::GetUserDataDirectoryForBrowserBundle(chrome_bundle,
-                                                    &user_data_dir)) {
-    Close();
-    return;
-  }
+  // The user_data_dir for shims actually contains the app_data_path.
+  // I.e. <user_data_dir>/<profile_dir>/Web Applications/_crx_extensionid/
+  base::FilePath user_data_dir =
+      g_info->user_data_dir.DirName().DirName().DirName();
+  CHECK(!user_data_dir.empty());
 
   base::FilePath socket_path =
       user_data_dir.Append(app_mode::kAppShimSocketName);

[commit-bot: I haz the power, 2013-07-31 02:51:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/19503003/20001

[commit-bot: I haz the power, 2013-07-31 06:22:38]

Change committed as 214582