Add user data dir field to Mac platform apps

Add user data dir field to Mac platform apps

With this CL we now specify an explicit data dir for Mac platform apps.

This CL also cleans up the various GetDataDir* functions in web_app.h.

BUG=112651
TEST=

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

[sail, 2012-02-21 00:06:43]

To make things easier to review I've split the "separate data directory" patch
into two CLs:
  - http://codereview.chromium.org/9374009 : the extension specific part that
deals with installing a platform app on a Mac
  - https://chromiumcodereview.appspot.com/9423048 : (this CL) the general
change to allow a user data directory to be set on a app bundle

[jeremy, 2012-02-21 12:17:59]

Is the correct terminology "data directory" or "profile directory"?  If it's
"profile directory" then the comments and variable names in this CL need
changing.

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_mac.h (right):

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.h:25: // The shortcut store its data
directory in |web_app_path|.
How about:
The shortcut will store data in it's own profile directory located at |...|.

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.h:27: const FilePath& web_app_path,
I'd call this: app_profile_directory_path or app_launcher_path or somesuch , the
current name doesn't make it clear that this is a profile directory.

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.mm:177:
forKey:app_mode::kCrAppModeUserDataDirKey];
Can we fill this in using the variable loop as above?

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/common/mac/app_...
File chrome/common/mac/app_mode_common.h (right):

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/common/mac/app_...
chrome/common/mac/app_mode_common.h:33: extern NSString * const
kCrAppModeUserDataDirKey;
' *' -> '*'

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/common/mac/app_...
File chrome/common/mac/app_mode_common.mm (right):

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/common/mac/app_...
chrome/common/mac/app_mode_common.mm:13: NSString* const
kCrAppModeUserDataDirKey = @"CrAppModeUserDataDir";
You should also add this key to the template Info.plist .

[Mihai Parparita -not on Chrome, 2012-02-21 20:53:21]

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
File chrome/browser/web_applications/web_app.h (right):

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
chrome/browser/web_applications/web_app.h:23: const std::string& extension_id,
Rather than the two overloaded variants of GetWebAppDataDirectory, can you go
back to the one that takes a ShortcutInfo instead? That struct already has
comments about the precedence of extension_id vs. URL (which otherwise you'd
have to repeat here).

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
chrome/browser/web_applications/web_app.h:27: // directory for |profile|.
Nit: Should this be |profile_path| instead?

[sail, 2012-02-22 22:10:08]

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
File chrome/browser/web_applications/web_app.h (right):

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
chrome/browser/web_applications/web_app.h:23: const std::string& extension_id,
On 2012/02/21 20:53:21, Mihai Parparita wrote:
> Rather than the two overloaded variants of GetWebAppDataDirectory, can you go
> back to the one that takes a ShortcutInfo instead? That struct already has
> comments about the precedence of extension_id vs. URL (which otherwise you'd
> have to repeat here).

I added a comment explaining how |extension_id| and |url| are used to create the
path.

I don't think using ShortcutInfo is useful for two reasons:
  - ShortcutInfo has many fields and it wouldn't be clear which fields need to
be filled in to call this function
  - the comment in ShortcutInfo is about the Windows app id which isn't the same
as the profile directory path (the url is concatenated differently).

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
chrome/browser/web_applications/web_app.h:27: // directory for |profile|.
On 2012/02/21 20:53:21, Mihai Parparita wrote:
> Nit: Should this be |profile_path| instead?

Done.

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_mac.h (right):

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.h:25: // The shortcut store its data
directory in |web_app_path|.
On 2012/02/21 12:17:59, jeremy wrote:
> How about:
> The shortcut will store data in it's own profile directory located at |...|.

This should be data directory. I changed the variable name from web_app_path to
data_dir to make it more clear.

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.h:27: const FilePath& web_app_path,
On 2012/02/21 12:17:59, jeremy wrote:
> I'd call this: app_profile_directory_path or app_launcher_path or somesuch ,
the
> current name doesn't make it clear that this is a profile directory.

Same as above.

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.mm:177:
forKey:app_mode::kCrAppModeUserDataDirKey];
On 2012/02/21 12:17:59, jeremy wrote:
> Can we fill this in using the variable loop as above?

I don't it makes sense to make this a template in the info plist. That means
that the key is hardcoded in two places (the template plist file and
app_mode_common.mm.

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/common/mac/app_...
File chrome/common/mac/app_mode_common.h (right):

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/common/mac/app_...
chrome/common/mac/app_mode_common.h:33: extern NSString * const
kCrAppModeUserDataDirKey;
On 2012/02/21 12:17:59, jeremy wrote:
> ' *' -> '*'

Done.

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/common/mac/app_...
File chrome/common/mac/app_mode_common.mm (right):

https://chromiumcodereview.appspot.com/9423048/diff/25/chrome/common/mac/app_...
chrome/common/mac/app_mode_common.mm:13: NSString* const
kCrAppModeUserDataDirKey = @"CrAppModeUserDataDir";
On 2012/02/21 12:17:59, jeremy wrote:
> You should also add this key to the template Info.plist .

As above, I think this is a bad idea.

[sail, 2012-02-22 22:11:41]

On 2012/02/21 12:17:59, jeremy wrote:
> Is the correct terminology "data directory" or "profile directory"?  If it's
> "profile directory" then the comments and variable names in this CL need
> changing.

"data directory" is correct. This is the parent directory to the profile
directory.
To run the browser we need to point it to a data directory. It creates a profile
directory, it creates a profile directory on its own.

[Mihai Parparita -not on Chrome, 2012-02-22 23:24:15]

LGTM

https://chromiumcodereview.appspot.com/9423048/diff/9001/chrome/browser/web_a...
File chrome/browser/web_applications/web_app.h (right):

https://chromiumcodereview.appspot.com/9423048/diff/9001/chrome/browser/web_a...
chrome/browser/web_applications/web_app.h:21: // Returns data directory for
given web app. The path for the directory is
Nit: can this also start with "Returns the user data directory", like the other
overloaded variant? I don't want there to be any confusion about the return
types being different.

https://chromiumcodereview.appspot.com/9423048/diff/9001/chrome/browser/web_a...
File chrome/browser/web_applications/web_app_unittest.cc (right):

https://chromiumcodereview.appspot.com/9423048/diff/9001/chrome/browser/web_a...
chrome/browser/web_applications/web_app_unittest.cc:47:
TEST_F(WebApplicationTest, AppDirWithID) {
Nit: This should be called AppDirWithId.

https://chromiumcodereview.appspot.com/9423048/diff/9001/chrome/browser/web_a...
chrome/browser/web_applications/web_app_unittest.cc:55:
TEST_F(WebApplicationTest, AppDirWithURL) {
Ditto about AppDirWithUrl.

[sail, 2012-02-22 23:38:48]

https://chromiumcodereview.appspot.com/9423048/diff/9001/chrome/browser/web_a...
File chrome/browser/web_applications/web_app.h (right):

https://chromiumcodereview.appspot.com/9423048/diff/9001/chrome/browser/web_a...
chrome/browser/web_applications/web_app.h:21: // Returns data directory for
given web app. The path for the directory is
On 2012/02/22 23:24:15, Mihai Parparita wrote:
> Nit: can this also start with "Returns the user data directory", like the
other
> overloaded variant? I don't want there to be any confusion about the return
> types being different.

Done.

https://chromiumcodereview.appspot.com/9423048/diff/9001/chrome/browser/web_a...
File chrome/browser/web_applications/web_app_unittest.cc (right):

https://chromiumcodereview.appspot.com/9423048/diff/9001/chrome/browser/web_a...
chrome/browser/web_applications/web_app_unittest.cc:47:
TEST_F(WebApplicationTest, AppDirWithID) {
On 2012/02/22 23:24:15, Mihai Parparita wrote:
> Nit: This should be called AppDirWithId.

Done.

https://chromiumcodereview.appspot.com/9423048/diff/9001/chrome/browser/web_a...
chrome/browser/web_applications/web_app_unittest.cc:55:
TEST_F(WebApplicationTest, AppDirWithURL) {
On 2012/02/22 23:24:15, Mihai Parparita wrote:
> Ditto about AppDirWithUrl.

Done.

[Robert Sesek, 2012-02-23 01:30:43]

lgtm with nits

https://chromiumcodereview.appspot.com/9423048/diff/8003/chrome/browser/web_a...
File chrome/browser/web_applications/web_app_mac.h (right):

https://chromiumcodereview.appspot.com/9423048/diff/8003/chrome/browser/web_a...
chrome/browser/web_applications/web_app_mac.h:25: // The shortcut store its data
directory in |data_dir|.
nit: storeS

https://chromiumcodereview.appspot.com/9423048/diff/8003/chrome/browser/web_a...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9423048/diff/8003/chrome/browser/web_a...
chrome/browser/web_applications/web_app_mac.mm:177:
forKey:app_mode::kCrAppModeUserDataDirKey];
nit: align colons. Line above, too.

[sail, 2012-02-23 03:00:41]

https://chromiumcodereview.appspot.com/9423048/diff/8003/chrome/browser/web_a...
File chrome/browser/web_applications/web_app_mac.h (right):

https://chromiumcodereview.appspot.com/9423048/diff/8003/chrome/browser/web_a...
chrome/browser/web_applications/web_app_mac.h:25: // The shortcut store its data
directory in |data_dir|.
On 2012/02/23 01:30:43, rsesek wrote:
> nit: storeS

Done.

https://chromiumcodereview.appspot.com/9423048/diff/8003/chrome/browser/web_a...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://chromiumcodereview.appspot.com/9423048/diff/8003/chrome/browser/web_a...
chrome/browser/web_applications/web_app_mac.mm:177:
forKey:app_mode::kCrAppModeUserDataDirKey];
On 2012/02/23 01:30:43, rsesek wrote:
> nit: align colons. Line above, too.

Done.

[jeremy, 2012-02-23 13:27:54]

Thanks for the clarification - can you add a comment in the code explaining
the distinction and that you specifically mean data directory and not
profile directory.

On Thu, Feb 23, 2012 at 12:11 AM, <sail@chromium.org> wrote:

> On 2012/02/21 12:17:59, jeremy wrote:
>
>> Is the correct terminology "data directory" or "profile directory"?  If
>> it's
>> "profile directory" then the comments and variable names in this CL need
>> changing.
>>
>
> "data directory" is correct. This is the parent directory to the profile
> directory.
> To run the browser we need to point it to a data directory. It creates a
> profile
> directory, it creates a profile directory on its own.
>
>
https://chromiumcodereview.**appspot.com/9423048/<https://chromiumcodereview....
>

[jeremy, 2012-02-23 13:30:54]

lgtm after adding comment per separate email.

https://chromiumcodereview.appspot.com/9423048/diff/15002/chrome/app/app_mode...
File chrome/app/app_mode_loader_mac.mm (right):

https://chromiumcodereview.appspot.com/9423048/diff/15002/chrome/app/app_mode...
chrome/app/app_mode_loader_mac.mm:95: info->user_data_dir =
base::mac::NSStringToFilePath(
What makes this a "user data dir" rather than just a "data dir"?

[sail, 2012-02-23 18:31:17]

https://chromiumcodereview.appspot.com/9423048/diff/15002/chrome/app/app_mode...
File chrome/app/app_mode_loader_mac.mm (right):

https://chromiumcodereview.appspot.com/9423048/diff/15002/chrome/app/app_mode...
chrome/app/app_mode_loader_mac.mm:95: info->user_data_dir =
base::mac::NSStringToFilePath(
On 2012/02/23 13:30:54, jeremy wrote:
> What makes this a "user data dir" rather than just a "data dir"?

Done.

user_data_dir is the correct name. I changed the references to "data dir" to
"user data dir".

[sail, 2012-02-23 18:31:36]

On 2012/02/23 13:27:54, jeremy wrote:
> Thanks for the clarification - can you add a comment in the code explaining
> the distinction and that you specifically mean data directory and not
> profile directory.

Sure, done.

[commit-bot: I haz the power, 2012-02-23 21:25:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/9423048/17002

[commit-bot: I haz the power, 2012-02-23 21:25:29]

Presubmit check for 9423048-17002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/browser/ui/web_applications/web_app_ui.cc

Presubmit checks took 1.6s to calculate.

[sky, 2012-02-23 21:53:13]

LGTM

[commit-bot: I haz the power, 2012-02-23 22:00:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/9423048/17002

[commit-bot: I haz the power, 2012-02-23 23:42:22]

Try job failure for 9423048-17002 (previous was lost) (retry) on win_rel for
step "compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-02-24 03:00:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/9423048/22001