Fix the new First Run sentinel file path determination.

Fix the new First Run sentinel file path determination.

Add first_run/InstallHelper GetFirstRunSentinelFilePath helpers.
Use PathService to get the correct value of DIR_USER_DATA.
(may be overridden by a policy, env var, or commandline)
(InstallUtil::GetSentinelFilePath just uses the default dir)

Make the IsFirstRunSentinelPresent helper platform-specific.
(simplifies its predecessor's Win-specific legacy abstractions)
Some related refactoring, cleanup, and test updates.

TODO(msw): Fix InstallUtil::GetSentinelFilePath.
TODO(msw): Fix/Nix installer::GetChromeUserDataPaths.
TODO(msw): Further consolidate related/duplicated code.

BUG=354634, 341049
TEST=Chrome install and first run work as intended; respect user-data-dir commandline, env var, and policy overrides.
R=grt@chromium.org
TBR=gab@chromium.org

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

[msw, 2014-03-24 23:40:56]

Hey Greg, please take a look; thanks!
Gabriel, please take a look when you return; thanks!

[grt (UTC plus 2), 2014-03-25 17:37:06]

looks good overall.

https://codereview.chromium.org/208393020/diff/20001/chrome/browser/first_run...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/208393020/diff/20001/chrome/browser/first_run...
chrome/browser/first_run/first_run.cc:577: base::FilePath first_run_sentinel;
first_run_sentinel -> user_data_dir

https://codereview.chromium.org/208393020/diff/20001/chrome/installer/setup/i...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/208393020/diff/20001/chrome/installer/setup/i...
chrome/installer/setup/install.cc:680: if
(!PathService::Get(chrome::DIR_USER_DATA, &first_run_sentinel))
i think you mean:
  if (PathService::Get(...))
    first_run_sentinel = first_run_sentinel.Append(chrome::kFirstRunSentinel);
but it's not clear to me what the right thing is for the else case. NOTREACHED
and early return?

on second thought, util/eula_util.cc has a "IsChromeFirstRunPending" method that
seems to answer exactly the question that's being asked in the code here. how
about making that function public and using it here (and anywhere else that does
something similar)?

https://codereview.chromium.org/208393020/diff/20001/chrome/installer/util/eu...
File chrome/installer/util/eula_util.cc (right):

https://codereview.chromium.org/208393020/diff/20001/chrome/installer/util/eu...
chrome/installer/util/eula_util.cc:27: base::FilePath first_run_sentinel;
first_run_sentinel -> user_data_dir

[msw, 2014-03-25 23:33:13]

Thanks for the tips, please take a fresh look!
TBR'ing estade@ for the c/b/ui/webui update.

https://codereview.chromium.org/208393020/diff/20001/chrome/browser/first_run...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/208393020/diff/20001/chrome/browser/first_run...
chrome/browser/first_run/first_run.cc:577: base::FilePath first_run_sentinel;
On 2014/03/25 17:37:06, grt wrote:
> first_run_sentinel -> user_data_dir

Done.

https://codereview.chromium.org/208393020/diff/20001/chrome/installer/setup/i...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/208393020/diff/20001/chrome/installer/setup/i...
chrome/installer/setup/install.cc:680: if
(!PathService::Get(chrome::DIR_USER_DATA, &first_run_sentinel))
On 2014/03/25 17:37:06, grt wrote:
> i think you mean:
>   if (PathService::Get(...))
>     first_run_sentinel = first_run_sentinel.Append(chrome::kFirstRunSentinel);
> but it's not clear to me what the right thing is for the else case. NOTREACHED
> and early return?
> 
> on second thought, util/eula_util.cc has a "IsChromeFirstRunPending" method
that
> seems to answer exactly the question that's being asked in the code here. how
> about making that function public and using it here (and anywhere else that
does
> something similar)?

Good catch and recommendation. I've reorganized and consolidated the first run
sentinel code around InstallUtil. I double-checked the logic and tested
developer Chromium builds (with and without the sentinel at default and
overridden user-data-dirs), as well as Chrome branded mini_installer instances
installed with setup.exe --chrome and --chrome-sxs (also with default and
overridden user-data-dirs). I also ensured that making Chrome the default
browser on first run also removed the sentinel to show the first-run experience
on the re-launch, and ensured that both --force-first-run and --no-first-run
worked as expected. As far as I can tell, the first run sentinel is being
created at (or copied to) the correct locations and respected by the application
correctly.

I did not verify that environment variable or policy overrides of DIR_USER_DATA
worked as expected, but they're handled in the same function as the commandline
overrides that I tested, so they should work fine. I did not test scenarios with
mixed system-level and user-level installations, nor deprecated
kMetroChromeUserDataSubDir scenarios, and may have missed some other corner
cases, but on the whole I think this is much better than it was.

[msw, 2014-03-26 00:57:08]

Gah, install_util is windows-specific... I'll need to change that, find a
platform-agnostic location for this code, or have some logic (for
HasFirstRunSentinel) duplicated across first_run and installer. I'll ping here
when I have a new patch.

[msw, 2014-03-26 14:17:45]

Okay, please take a look now.

[grt (UTC plus 2), 2014-03-26 16:41:21]

On 2014/03/26 14:17:45, msw wrote:
> Okay, please take a look now.

If there's no time pressure on this, I'd like to wait until Friday to have
another look. Let me know if this is a problem. Cheers.

[msw, 2014-03-27 01:16:05]

On 2014/03/26 16:41:21, grt wrote:
> On 2014/03/26 14:17:45, msw wrote:
> > Okay, please take a look now.
> 
> If there's no time pressure on this, I'd like to wait until Friday to have
> another look. Let me know if this is a problem. Cheers.

No real time pressure, but it'd be nice to get in for M-35 (branch fri).

[grt (UTC plus 2), 2014-03-27 15:35:19]

just some small comments remaining

https://codereview.chromium.org/208393020/diff/150002/chrome/browser/first_ru...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/208393020/diff/150002/chrome/browser/first_ru...
chrome/browser/first_run/first_run.cc:549: return !path->empty();
it's not safe to assume that the caller passes in an empty path.

https://codereview.chromium.org/208393020/diff/150002/chrome/browser/first_ru...
chrome/browser/first_run/first_run.cc:555: base::WriteFile(first_run_sentinel,
"", 0) != -1;
nit: either 4-space indent or wrap the whole thing in parens:
  return foo &&
      bar;
or:
  return (foo &&
          bar);

https://codereview.chromium.org/208393020/diff/150002/chrome/browser/first_ru...
chrome/browser/first_run/first_run.cc:619: base::DeleteFile(first_run_sentinel,
false);
same indentation nit

https://codereview.chromium.org/208393020/diff/150002/chrome/browser/first_ru...
File chrome/browser/first_run/first_run_internal_win.cc (right):

https://codereview.chromium.org/208393020/diff/150002/chrome/browser/first_ru...
chrome/browser/first_run/first_run_internal_win.cc:164: return true;
please preserve this comment from first_run.cc:
// Sentinel is present regardless of whether or not it was migrated.
since it explains why true is returned even when !migrated

https://codereview.chromium.org/208393020/diff/150002/chrome/installer/util/i...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/208393020/diff/150002/chrome/installer/util/i...
chrome/installer/util/install_util.cc:406: base::FilePath user_data_dir;
please add something like:
  // This duplicates portions of code in first_run, but is required
  // here for use by the installer.

https://codereview.chromium.org/208393020/diff/150002/chrome/installer/util/i...
chrome/installer/util/install_util.cc:412: bool
InstallUtil::GetSentinelFilePath(const base::FilePath::CharType* file,
this function is now only used to get the path for the EULA sentinel. in a
follow-on CL, please at least remove GetEULASentinelFilePath in
first_run_internal_win.cc and change this to be EULA-specific. maybe something
more grand makes sense, too. in a quick look at how it's used, i don't think it
needs to take a BrowserDistribution any longer.

[msw, 2014-03-28 00:04:10]

Comments addressed; please take another look; thanks!

https://codereview.chromium.org/208393020/diff/150002/chrome/browser/first_ru...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/208393020/diff/150002/chrome/browser/first_ru...
chrome/browser/first_run/first_run.cc:549: return !path->empty();
On 2014/03/27 15:35:20, grt wrote:
> it's not safe to assume that the caller passes in an empty path.

Done (restored the original pattern).

https://codereview.chromium.org/208393020/diff/150002/chrome/browser/first_ru...
chrome/browser/first_run/first_run.cc:555: base::WriteFile(first_run_sentinel,
"", 0) != -1;
On 2014/03/27 15:35:20, grt wrote:
> nit: either 4-space indent or wrap the whole thing in parens:
>   return foo &&
>       bar;
> or:
>   return (foo &&
>           bar);

Done (4-space indent).

https://codereview.chromium.org/208393020/diff/150002/chrome/browser/first_ru...
chrome/browser/first_run/first_run.cc:619: base::DeleteFile(first_run_sentinel,
false);
On 2014/03/27 15:35:20, grt wrote:
> same indentation nit

Done.

https://codereview.chromium.org/208393020/diff/150002/chrome/browser/first_ru...
File chrome/browser/first_run/first_run_internal_win.cc (right):

https://codereview.chromium.org/208393020/diff/150002/chrome/browser/first_ru...
chrome/browser/first_run/first_run_internal_win.cc:164: return true;
On 2014/03/27 15:35:20, grt wrote:
> please preserve this comment from first_run.cc:
> // Sentinel is present regardless of whether or not it was migrated.
> since it explains why true is returned even when !migrated

Done.

https://codereview.chromium.org/208393020/diff/150002/chrome/installer/util/i...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/208393020/diff/150002/chrome/installer/util/i...
chrome/installer/util/install_util.cc:406: base::FilePath user_data_dir;
On 2014/03/27 15:35:20, grt wrote:
> please add something like:
>   // This duplicates portions of code in first_run, but is required
>   // here for use by the installer.

Done (added a simple TODO).

https://codereview.chromium.org/208393020/diff/150002/chrome/installer/util/i...
chrome/installer/util/install_util.cc:412: bool
InstallUtil::GetSentinelFilePath(const base::FilePath::CharType* file,
On 2014/03/27 15:35:20, grt wrote:
> this function is now only used to get the path for the EULA sentinel. in a
> follow-on CL, please at least remove GetEULASentinelFilePath in
> first_run_internal_win.cc and change this to be EULA-specific. maybe something
> more grand makes sense, too. in a quick look at how it's used, i don't think
it
> needs to take a BrowserDistribution any longer.

Yup, I'll follow up this CL with similar changes for the EULA sentinel.
It should use the proper user data dir, and there's room for cleanup.

[grt (UTC plus 2), 2014-03-28 01:37:42]

lgtm

[commit-bot: I haz the power, 2014-03-28 01:39:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/208393020/200001

[msw, 2014-03-28 17:19:41]

The CQ bit was unchecked by msw@chromium.org

[msw, 2014-03-28 17:21:00]

The CQ bit was checked by msw@chromium.org

[msw, 2014-03-28 19:32:04]

The CQ bit was checked by msw@chromium.org

[msw, 2014-03-28 19:37:44]

On 2014/03/28 19:32:04, msw wrote:
> The CQ bit was checked by mailto:msw@chromium.org

The CQ isn't picking up this change... I'm going to dcommit.

[msw, 2014-03-28 19:39:59]

Committed patchset #7 manually as r260242 (presubmit successful).

[gab, 2014-04-02 15:06:50]

This won't work for setup.exe; see comment below. It looks like this made M35
too (which cut at r260298); please fix it on branch too.

Cheers,
Gab

https://codereview.chromium.org/208393020/diff/220001/chrome/installer/util/i...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/208393020/diff/220001/chrome/installer/util/i...
chrome/installer/util/install_util.cc:408: return
!PathService::Get(chrome::DIR_USER_DATA, &user_data_dir) ||
This won't work inside setup.exe. Only chrome.exe registers Chrome's
PathProvider with PathService [1]. Therefore this will always return true in
setup.exe as PathService::Get(chrome::DIR_USER_DATA, ...) will always fail. To
make this work DIR_USER_DATA would have to be in a PathProvider used by both
chrome.exe and setup.exe.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/chrome_...

[grt (UTC plus 2), 2014-04-02 16:42:11]

https://codereview.chromium.org/208393020/diff/220001/chrome/installer/util/i...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/208393020/diff/220001/chrome/installer/util/i...
chrome/installer/util/install_util.cc:408: return
!PathService::Get(chrome::DIR_USER_DATA, &user_data_dir) ||
On 2014/04/02 15:06:51, gab wrote:
> This won't work inside setup.exe. Only chrome.exe registers Chrome's
> PathProvider with PathService [1]. Therefore this will always return true in
> setup.exe as PathService::Get(chrome::DIR_USER_DATA, ...) will always fail. To
> make this work DIR_USER_DATA would have to be in a PathProvider used by both
> chrome.exe and setup.exe.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/chrome_...

Nice catch. setup depends on common_constants, so it may suffice to stick a call
to chrome::RegisterPathProvider() in setup_main.cc.

[msw, 2014-04-02 16:44:10]

On 2014/04/02 16:42:11, grt wrote:
>
https://codereview.chromium.org/208393020/diff/220001/chrome/installer/util/i...
> File chrome/installer/util/install_util.cc (right):
> 
>
https://codereview.chromium.org/208393020/diff/220001/chrome/installer/util/i...
> chrome/installer/util/install_util.cc:408: return
> !PathService::Get(chrome::DIR_USER_DATA, &user_data_dir) ||
> On 2014/04/02 15:06:51, gab wrote:
> > This won't work inside setup.exe. Only chrome.exe registers Chrome's
> > PathProvider with PathService [1]. Therefore this will always return true in
> > setup.exe as PathService::Get(chrome::DIR_USER_DATA, ...) will always fail.
To
> > make this work DIR_USER_DATA would have to be in a PathProvider used by both
> > chrome.exe and setup.exe.
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/chrome_...
> 
> Nice catch. setup depends on common_constants, so it may suffice to stick a
call
> to chrome::RegisterPathProvider() in setup_main.cc.

Dang, I'm at jury selection today, so I can't try/investigate/fix this now.

[grt (UTC plus 2), 2014-04-02 17:13:51]

On 2014/04/02 16:44:10, msw wrote:
> Dang, I'm at jury selection today, so I can't try/investigate/fix this now.

Gab will file a bug and I'll send up a fix. I think it's a two-liner, so merging
will be simple. Have fun fulfilling your civic responsibility!