If possible, use the PathService instead of the --user-data-dir flag directly

If possible, use the PathService instead of the --user-data-dir flag directly

Some classes still using it directly are:
- cloud_print_proxy_process_browsertest.cc (it's not a browser test so it doesn't
call ChromeMainDelegate::PreSandboxStartup(), which sets up the PathService
to know about the user-data-dir

- in_process_browser_test.cc, which needs to set up the user-data-dir directory
before PreSandboxStartup() is called

- diagnostics_model and diagnostics_controller, which are used in
BasicStartupComplete(), which is called before PreSandboxStartup() where the
PathService is set up

- shell_integration_win.cc which has a function that's used in unit tests, which
don't call PreSandboxStartup()

- chrome/browser/shell_integration.cc is used differently on linux than on other
platforms, and on the former a desktop app shortcut uses a different command line
that that of Chrome's

- cloud_print/service/*, since it seems to be its own thing, and have a separate
user_data_dir

BUG=464616
Committed: https://crrev.com/c527ac73268c6f603dfa6aa41ee9a0c3d79d5095
Cr-Commit-Position: refs/heads/master@{#322697}

[noms (inactive), 2015-03-20 21:14:25]

Patchset #1 (id:1) has been deleted

[noms (inactive), 2015-03-20 21:14:53]

Patchset #1 (id:20001) has been deleted

[noms (inactive), 2015-03-20 21:15:03]

Patchset #1 (id:40001) has been deleted

[noms (inactive), 2015-03-20 21:15:17]

Patchset #1 (id:60001) has been deleted

[noms (inactive), 2015-03-20 21:16:22]

Patchset #1 (id:80001) has been deleted

[noms (inactive), 2015-03-20 21:16:30]

Patchset #1 (id:100001) has been deleted

[noms (inactive), 2015-03-20 21:16:37]

Patchset #1 (id:120001) has been deleted

[noms (inactive), 2015-03-20 21:17:34]

Patchset #1 (id:140001) has been deleted

[noms (inactive), 2015-03-20 21:17:43]

Patchset #1 (id:160001) has been deleted

[noms (inactive), 2015-03-24 20:04:00]

Patchset #1 (id:180001) has been deleted

[noms (inactive), 2015-03-24 20:04:06]

Patchset #1 (id:200001) has been deleted

[noms (inactive), 2015-03-24 20:04:15]

Patchset #1 (id:220001) has been deleted

[noms (inactive), 2015-03-24 20:42:52]

Patchset #1 (id:240001) has been deleted

[noms (inactive), 2015-03-24 20:43:55]

noms@chromium.org changed reviewers:
+ grt@chromium.org

[noms (inactive), 2015-03-24 20:43:56]

Hiya Greg!

While I'm chasing down the last failing test (grrr), would you mind taking a
look at this? There's 4 places that we still have to use the path directly (see
the description), but these other ones seem like they're doing ok.

[grt (UTC plus 2), 2015-03-26 18:36:22]

Ugh

https://codereview.chromium.org/1010923002/diff/260001/chrome/browser/shell_i...
File chrome/browser/shell_integration.cc (right):

https://codereview.chromium.org/1010923002/diff/260001/chrome/browser/shell_i...
chrome/browser/shell_integration.cc:91:
policy::path_parser::CheckUserDataDirPolicy(&user_data_dir);
this isn't needed now, is it?

https://codereview.chromium.org/1010923002/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (left):

https://codereview.chromium.org/1010923002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/app_list/win/app_list_service_win.cc:129:
base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
i hate to say it, but leave this one alone. it would be bad to change the app
model id in this case.

https://codereview.chromium.org/1010923002/diff/260001/chrome/installer/util/...
File chrome/installer/util/auto_launch_util.cc (right):

https://codereview.chromium.org/1010923002/diff/260001/chrome/installer/util/...
chrome/installer/util/auto_launch_util.cc:188: if
(command_line.HasSwitch(switches::kUserDataDir)) {
this is subtle, so i think it needs a comment. i suggest:
      // Propagate --user-data-dir if it was specified on the command line.
      // Retrieve the value from the PathService since some sanitation may have
      // taken place. There is no need to add it to the command line in the
      // event that the dir was overridden by Group Policy since the GP override
      // will be in force when Chrome is launched.
but feel free to reword if i'm missing something.

https://codereview.chromium.org/1010923002/diff/260001/chrome/service/cloud_p...
File chrome/service/cloud_print/cloud_print_proxy.cc (left):

https://codereview.chromium.org/1010923002/diff/260001/chrome/service/cloud_p...
chrome/service/cloud_print/cloud_print_proxy.cc:37: const base::CommandLine&
process_command_line =
i think this is okay as-is since the launched process will get the value from GP
if it is overridden there. i suggest:
  // Propagate an explicit --user-data-dir value if one was given. The new
  // browser process will pick up a policy override during initialization.

https://codereview.chromium.org/1010923002/diff/260001/cloud_print/service/wi...
File cloud_print/service/win/chrome_launcher.cc (left):

https://codereview.chromium.org/1010923002/diff/260001/cloud_print/service/wi...
cloud_print/service/win/chrome_launcher.cc:159:
cmd.GetSwitchValuePath(switches::kUserDataDir));
i wouldn't touch this one. it seems to me that cloud_print_service.exe is its
own thing with its own user data dir. it doesn't appear to register the chrome
path provider, so chrome::DIR_USER_DATA won't work.

https://codereview.chromium.org/1010923002/diff/260001/cloud_print/service/wi...
File cloud_print/service/win/cloud_print_service.cc (left):

https://codereview.chromium.org/1010923002/diff/260001/cloud_print/service/wi...
cloud_print/service/win/cloud_print_service.cc:212: user_data_dir_switch_ =
leave this one alone, too

[noms, 2015-03-27 17:03:57]

noms@google.com changed reviewers:
+ noms@google.com

[noms, 2015-03-27 17:03:58]

This CL is going great! Only 3 files to go until it's a no-op. Done all the
things, added their reasons to the CL.

I'm thinking about adding a comment in chrome_switches.cc, where kUserDataDir is
defined with instructions to use the PathService unless you really, really want
the path directly from the command line. What do you think?

[grt (UTC plus 2), 2015-03-27 19:13:28]

lgtm

[noms, 2015-03-27 19:16:29]

noms@google.com changed reviewers:
+ sky@chromium.org

[noms, 2015-03-27 19:16:30]

+ sky for owners stamp on chrome/service. Also as proof that I actually finished
this CL :)

[sky, 2015-03-27 21:18:38]

Nice, LGTM

[noms (inactive), 2015-03-28 03:31:12]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2015-03-28 03:31:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1010923002/280001

[commit-bot: I haz the power, 2015-03-28 03:35:12]

Committed patchset #2 (id:280001)

[commit-bot: I haz the power, 2015-03-28 03:35:57]

Patchset 2 (id:??) landed as
https://crrev.com/c527ac73268c6f603dfa6aa41ee9a0c3d79d5095
Cr-Commit-Position: refs/heads/master@{#322697}

[noms (inactive), 2015-03-30 14:06:33]

Oops, I had unsent drafts :/

https://codereview.chromium.org/1010923002/diff/260001/chrome/browser/shell_i...
File chrome/browser/shell_integration.cc (right):

https://codereview.chromium.org/1010923002/diff/260001/chrome/browser/shell_i...
chrome/browser/shell_integration.cc:91:
policy::path_parser::CheckUserDataDirPolicy(&user_data_dir);
On 2015/03/26 18:36:22, grt wrote:
> this isn't needed now, is it?

Done.

https://codereview.chromium.org/1010923002/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (left):

https://codereview.chromium.org/1010923002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/app_list/win/app_list_service_win.cc:129:
base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
On 2015/03/26 18:36:22, grt wrote:
> i hate to say it, but leave this one alone. it would be bad to change the app
> model id in this case.

Done.

https://codereview.chromium.org/1010923002/diff/260001/chrome/installer/util/...
File chrome/installer/util/auto_launch_util.cc (right):

https://codereview.chromium.org/1010923002/diff/260001/chrome/installer/util/...
chrome/installer/util/auto_launch_util.cc:188: if
(command_line.HasSwitch(switches::kUserDataDir)) {
On 2015/03/26 18:36:22, grt wrote:
> this is subtle, so i think it needs a comment. i suggest:
>       // Propagate --user-data-dir if it was specified on the command line.
>       // Retrieve the value from the PathService since some sanitation may
have
>       // taken place. There is no need to add it to the command line in the
>       // event that the dir was overridden by Group Policy since the GP
override
>       // will be in force when Chrome is launched.
> but feel free to reword if i'm missing something.

Done.

https://codereview.chromium.org/1010923002/diff/260001/chrome/service/cloud_p...
File chrome/service/cloud_print/cloud_print_proxy.cc (left):

https://codereview.chromium.org/1010923002/diff/260001/chrome/service/cloud_p...
chrome/service/cloud_print/cloud_print_proxy.cc:37: const base::CommandLine&
process_command_line =
On 2015/03/26 18:36:22, grt wrote:
> i think this is okay as-is since the launched process will get the value from
GP
> if it is overridden there. i suggest:
>   // Propagate an explicit --user-data-dir value if one was given. The new
>   // browser process will pick up a policy override during initialization.

Done.

https://codereview.chromium.org/1010923002/diff/260001/cloud_print/service/wi...
File cloud_print/service/win/chrome_launcher.cc (left):

https://codereview.chromium.org/1010923002/diff/260001/cloud_print/service/wi...
cloud_print/service/win/chrome_launcher.cc:159:
cmd.GetSwitchValuePath(switches::kUserDataDir));
On 2015/03/26 18:36:22, grt wrote:
> i wouldn't touch this one. it seems to me that cloud_print_service.exe is its
> own thing with its own user data dir. it doesn't appear to register the chrome
> path provider, so chrome::DIR_USER_DATA won't work.

Done.

https://codereview.chromium.org/1010923002/diff/260001/cloud_print/service/wi...
File cloud_print/service/win/cloud_print_service.cc (left):

https://codereview.chromium.org/1010923002/diff/260001/cloud_print/service/wi...
cloud_print/service/win/cloud_print_service.cc:212: user_data_dir_switch_ =
On 2015/03/26 18:36:22, grt wrote:
> leave this one alone, too

Done.