Delete both regular and Metro user data dirs on uninstall.

Delete both regular and Metro user data dirs on uninstall.

Also:
- The uninstall survey will use the metro Chrome local state if there is no desktop Chrome local state (although in the future we maybe want to aggregate them).
- Chrome inactivity for toast purposes is now based on the most-recently used of both desktop and metro Chrome.

BUG=125793
TEST=install chrome, make it the default, launch into metro at least once, quit, then uninstall and check the "also clear..." box.  Metro data dir should be gone.

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

[gab, 2012-08-03 17:54:20]

Thanks for digging into this, turns out I had also started the same bug EOD
yesterday and was about to send it for review this morning, at least it sped up
this review given I already have context on everything :)!

See below!

Cheers,
Gab

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:356: for (size_t i = 0; i <
local_state_folders.size(); ++i) {
Is it ok if the Metro directory comes out of this? I'm not convinced from the
usage related to Chrome metrics in DoPostUninstallOperations().

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:380: return DELETE_SUCCEEDED;
I think this should be: {
  NOTREACHED();
  return DELETE_FAILED;
}

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:383: const FilePath *user_local_state =
NULL;
Move this in the for loop (see comment below as to why you don't need it out of
that scope).

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:399: if (user_local_state) {
This check is unnecessary given you only proceed if !local_state_folders.empty()
above.

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:401:
ScheduleParentAndGrandparentForDeletion(*user_local_state);
You can use local_state_folders[0] here instead (given your comment on
GetChromeUserDataPaths() that the first path is the primary one and that the
other ones are guaranteed to be siblings).

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/util/...
File chrome/installer/util/helper.cc (right):

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/util/...
chrome/installer/util/helper.cc:57: chrome::kMetroChromeUserDataSubDir);
We do not want installer_util to depend on chrome (we want the opposite).

Move this constant into chrome/installer/util/util_constants.h.

And change GetUserDataDirectoryForEnvironment() in chrome_paths_win.cc to
something like:

bool GetUserDataDirectoryForEnvironment(bool current, FilePath* result) {
  std::vector<FilePath> data_dir_paths;
  BrowserDistribution* chrome_dist =
BrowserDistribution::GetSpecificDistribution(
          BrowserDistribution::CHROME_BROWSER));
  installer::GetChromeUserDataPaths(chrome_dist, &data_dir_paths);
  
  const bool want_metro = base::win::IsMetroProcess() ? current : !current;
  if (data_dir_paths.size() < (want_metro ? 2 : 1)) {
    NOTREACHED();
    return false;
  }
  *result = want_metro ? data_dir_paths[1] : data_dir_paths[0];
  return true;
}

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/util/...
File chrome/installer/util/product_unittest.cc (right):

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/util/...
chrome/installer/util/product_unittest.cc:76: EXPECT_LE(static_cast<size_t>(1),
user_data_paths.size());
In comparisons don't we usually put the constant on the RHS of the equation?

[grt (UTC plus 2), 2012-08-03 20:18:37]

Thanks for the review.  I've also updated the description to include mention of
the uninstall survey and the toast changes.

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:356: for (size_t i = 0; i <
local_state_folders.size(); ++i) {
On 2012/08/03 17:54:20, gab wrote:
> Is it ok if the Metro directory comes out of this? I'm not convinced from the
> usage related to Chrome metrics in DoPostUninstallOperations().

Yes.  Ideally, we'd aggregate the metrics from both sides, or at least pick the
more recent one, but something (desktop, falling back to metro) is better than
nothing in this case.

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:380: return DELETE_SUCCEEDED;
On 2012/08/03 17:54:20, gab wrote:
> I think this should be: {
>   NOTREACHED();
>   return DELETE_FAILED;
> }

I added a DCHECK to GetChromeUserDataPaths() rather than sprinkling them around
the code.  Do you think that's sufficient?

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:383: const FilePath *user_local_state =
NULL;
On 2012/08/03 17:54:20, gab wrote:
> Move this in the for loop (see comment below as to why you don't need it out
of
> that scope).

Done.

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:399: if (user_local_state) {
On 2012/08/03 17:54:20, gab wrote:
> This check is unnecessary given you only proceed if
!local_state_folders.empty()
> above.

Done.

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:401:
ScheduleParentAndGrandparentForDeletion(*user_local_state);
On 2012/08/03 17:54:20, gab wrote:
> You can use local_state_folders[0] here instead (given your comment on
> GetChromeUserDataPaths() that the first path is the primary one and that the
> other ones are guaranteed to be siblings).

Done.

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/util/...
File chrome/installer/util/helper.cc (right):

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/util/...
chrome/installer/util/helper.cc:57: chrome::kMetroChromeUserDataSubDir);
On 2012/08/03 17:54:20, gab wrote:
> We do not want installer_util to depend on chrome (we want the opposite).
>
> Move this constant into chrome/installer/util/util_constants.h.

not done.  this is in common_constants, which installer_util already depends on
(note the lack of .gypi changes).  all other cases where util_constants
duplicates values defined in common_constants should be removed.  this task is
outside the scope of this CL.

> And change GetUserDataDirectoryForEnvironment() in chrome_paths_win.cc to
> something like:
> 
> bool GetUserDataDirectoryForEnvironment(bool current, FilePath* result) {
>   std::vector<FilePath> data_dir_paths;
>   BrowserDistribution* chrome_dist =
> BrowserDistribution::GetSpecificDistribution(
>           BrowserDistribution::CHROME_BROWSER));

this is incorrect since the browser may not be what is running.

>   installer::GetChromeUserDataPaths(chrome_dist, &data_dir_paths);
>   
>   const bool want_metro = base::win::IsMetroProcess() ? current : !current;
>   if (data_dir_paths.size() < (want_metro ? 2 : 1)) {
>     NOTREACHED();
>     return false;
>   }
>   *result = want_metro ? data_dir_paths[1] : data_dir_paths[0];
>   return true;
> }

i think this is a step in the wrong direction.  the existing logic is very
clear.  the code you propose assumes an implicit contract that the metro
directory comes after the desktop directory in the installer's list.  that seems
dangerous. i also think it's wrong for the product to ask the installer for
these directories.  if anything, i'd like to see a followup CL that puts all
"user data dir" logic in the product and have the installer ask the product,
since the product is the primary producer/consumer and the installer is just a
technician that steps in at uninstall time to do some bookkeeping and cleanup.

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/util/...
File chrome/installer/util/product_unittest.cc (right):

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/util/...
chrome/installer/util/product_unittest.cc:76: EXPECT_LE(static_cast<size_t>(1),
user_data_paths.size());
On 2012/08/03 17:54:20, gab wrote:
> In comparisons don't we usually put the constant on the RHS of the equation?

I'm following the pattern of EXPECT_EQ; see
http://code.google.com/p/googletest/wiki/Primer#Binary_Comparison.

[gab, 2012-08-03 20:42:41]

You seem to have missed my comments on helper.cc and product_unittest.cc?

+ 1 more comment below.

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:380: return DELETE_SUCCEEDED;
On 2012/08/03 20:18:37, grt wrote:
> On 2012/08/03 17:54:20, gab wrote:
> > I think this should be: {
> >   NOTREACHED();
> >   return DELETE_FAILED;
> > }
> 
> I added a DCHECK to GetChromeUserDataPaths() rather than sprinkling them
around
> the code.  Do you think that's sufficient?

Yes, but I still think the return statement here should return DELETE_FAILED.

[grt (UTC plus 2), 2012-08-03 20:56:34]

You seem to have missed my replies in helper.cc and product_unittest.cc.  :-)

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:380: return DELETE_SUCCEEDED;
On 2012/08/03 20:42:41, gab wrote:
> On 2012/08/03 20:18:37, grt wrote:
> > On 2012/08/03 17:54:20, gab wrote:
> > > I think this should be: {
> > >   NOTREACHED();
> > >   return DELETE_FAILED;
> > > }
> > 
> > I added a DCHECK to GetChromeUserDataPaths() rather than sprinkling them
> around
> > the code.  Do you think that's sufficient?
> 
> Yes, but I still think the return statement here should return DELETE_FAILED.

hmm.  i disagree (although it's purely academic since the return value is
ignored).  if the vector was empty, the function was told to delete nothing at
all, which is a noop.  i don't think this should imply that the uninstall should
be aborted.

[gab, 2012-08-03 21:58:57]

Oh oops, missed your replies indeed :)!

lgtm (see replies below)

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:380: return DELETE_SUCCEEDED;
On 2012/08/03 20:56:34, grt wrote:
> On 2012/08/03 20:42:41, gab wrote:
> > On 2012/08/03 20:18:37, grt wrote:
> > > On 2012/08/03 17:54:20, gab wrote:
> > > > I think this should be: {
> > > >   NOTREACHED();
> > > >   return DELETE_FAILED;
> > > > }
> > > 
> > > I added a DCHECK to GetChromeUserDataPaths() rather than sprinkling them
> > around
> > > the code.  Do you think that's sufficient?
> > 
> > Yes, but I still think the return statement here should return
DELETE_FAILED.
> 
> hmm.  i disagree (although it's purely academic since the return value is
> ignored).  if the vector was empty, the function was told to delete nothing at
> all, which is a noop.  i don't think this should imply that the uninstall
should
> be aborted.

Ah ok I see your point, given the returned value is not used though what about
making this function return void?

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/util/...
File chrome/installer/util/helper.cc (right):

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/util/...
chrome/installer/util/helper.cc:57: chrome::kMetroChromeUserDataSubDir);
On 2012/08/03 20:18:37, grt wrote:
> On 2012/08/03 17:54:20, gab wrote:
> > We do not want installer_util to depend on chrome (we want the opposite).
> >
> > Move this constant into chrome/installer/util/util_constants.h.
> 
> not done.  this is in common_constants, which installer_util already depends
on
> (note the lack of .gypi changes).  all other cases where util_constants
> duplicates values defined in common_constants should be removed.  this task is
> outside the scope of this CL.
> 
> > And change GetUserDataDirectoryForEnvironment() in chrome_paths_win.cc to
> > something like:
> > 
> > bool GetUserDataDirectoryForEnvironment(bool current, FilePath* result) {
> >   std::vector<FilePath> data_dir_paths;
> >   BrowserDistribution* chrome_dist =
> > BrowserDistribution::GetSpecificDistribution(
> >           BrowserDistribution::CHROME_BROWSER));
> 
> this is incorrect since the browser may not be what is running.
> 
> >   installer::GetChromeUserDataPaths(chrome_dist, &data_dir_paths);
> >   
> >   const bool want_metro = base::win::IsMetroProcess() ? current : !current;
> >   if (data_dir_paths.size() < (want_metro ? 2 : 1)) {
> >     NOTREACHED();
> >     return false;
> >   }
> >   *result = want_metro ? data_dir_paths[1] : data_dir_paths[0];
> >   return true;
> > }
> 
> i think this is a step in the wrong direction.  the existing logic is very
> clear.  the code you propose assumes an implicit contract that the metro
> directory comes after the desktop directory in the installer's list.  that
seems
> dangerous. i also think it's wrong for the product to ask the installer for
> these directories.  if anything, i'd like to see a followup CL that puts all
> "user data dir" logic in the product and have the installer ask the product,
> since the product is the primary producer/consumer and the installer is just a
> technician that steps in at uninstall time to do some bookkeeping and cleanup.

Ah I see, chrome common constants != chrome :).

Thanks for clarifying!

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/util/...
File chrome/installer/util/product_unittest.cc (right):

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/util/...
chrome/installer/util/product_unittest.cc:76: EXPECT_LE(static_cast<size_t>(1),
user_data_paths.size());
On 2012/08/03 20:18:37, grt wrote:
> On 2012/08/03 17:54:20, gab wrote:
> > In comparisons don't we usually put the constant on the RHS of the equation?
> 
> I'm following the pattern of EXPECT_EQ; see
> http://code.google.com/p/googletest/wiki/Primer#Binary_Comparison.

Yea I realize that, it makes sense for me to do it this way for EXPECT_EQ, but
for EXPECT_LE I feel it's like EXPECT_TRUE(val1 < val2) in which case I feel
val2 should be the constant hence my proposal.

I don't feel strongly about it though so... optional :)!

[commit-bot: I haz the power, 2012-08-03 22:06:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/10826144/4001

[commit-bot: I haz the power, 2012-08-03 23:33:35]

Change committed as 149968

[grt (UTC plus 2), 2012-08-04 19:25:20]

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:380: return DELETE_SUCCEEDED;
On 2012/08/03 21:58:57, gab wrote:
> On 2012/08/03 20:56:34, grt wrote:
> > On 2012/08/03 20:42:41, gab wrote:
> > > On 2012/08/03 20:18:37, grt wrote:
> > > > On 2012/08/03 17:54:20, gab wrote:
> > > > > I think this should be: {
> > > > >   NOTREACHED();
> > > > >   return DELETE_FAILED;
> > > > > }
> > > > 
> > > > I added a DCHECK to GetChromeUserDataPaths() rather than sprinkling them
> > > around
> > > > the code.  Do you think that's sufficient?
> > > 
> > > Yes, but I still think the return statement here should return
> DELETE_FAILED.
> > 
> > hmm.  i disagree (although it's purely academic since the return value is
> > ignored).  if the vector was empty, the function was told to delete nothing
at
> > all, which is a noop.  i don't think this should imply that the uninstall
> should
> > be aborted.
> 
> Ah ok I see your point, given the returned value is not used though what about
> making this function return void?

After more thought, I like this as-is.  The fact that the (current) caller
doesn't care about the result doesn't mean that this function shouldn't have a
clean interface.

[gab, 2012-08-05 06:27:39]

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10826144/diff/1/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:380: return DELETE_SUCCEEDED;
On 2012/08/04 19:25:20, grt wrote:
> On 2012/08/03 21:58:57, gab wrote:
> > On 2012/08/03 20:56:34, grt wrote:
> > > On 2012/08/03 20:42:41, gab wrote:
> > > > On 2012/08/03 20:18:37, grt wrote:
> > > > > On 2012/08/03 17:54:20, gab wrote:
> > > > > > I think this should be: {
> > > > > >   NOTREACHED();
> > > > > >   return DELETE_FAILED;
> > > > > > }
> > > > > 
> > > > > I added a DCHECK to GetChromeUserDataPaths() rather than sprinkling
them
> > > > around
> > > > > the code.  Do you think that's sufficient?
> > > > 
> > > > Yes, but I still think the return statement here should return
> > DELETE_FAILED.
> > > 
> > > hmm.  i disagree (although it's purely academic since the return value is
> > > ignored).  if the vector was empty, the function was told to delete
nothing
> at
> > > all, which is a noop.  i don't think this should imply that the uninstall
> > should
> > > be aborted.
> > 
> > Ah ok I see your point, given the returned value is not used though what
about
> > making this function return void?
> 
> After more thought, I like this as-is.  The fact that the (current) caller
> doesn't care about the result doesn't mean that this function shouldn't have a
> clean interface.

sgtm