installer and some misc files: Change ScopedTempDir::path() to GetPath()

installer and some misc files: Change ScopedTempDir::path() to GetPath()

path() is being deprecated, GetPath() has better checking against wrong use.

For more context, see https://codereview.chromium.org/2275553005/.

BUG=640599
Committed: https://crrev.com/5d919d6e4866aa240c11d21b917f2ac8ea5d5eb8
Cr-Commit-Position: refs/heads/master@{#422068}

[vabr (Chromium), 2016-09-28 09:48:39]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 09:48:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 10:54:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 10:54:53]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[vabr (Chromium), 2016-09-28 11:14:35]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 11:14:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 11:44:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 11:44:23]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[vabr (Chromium), 2016-09-28 12:25:35]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 12:25:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 12:58:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 12:58:52]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-09-28 13:10:23]

vabr@chromium.org changed reviewers:
+ gab@chromium.org, siggi@chromium.org

[vabr (Chromium), 2016-09-28 13:10:25]

Hi gab@ and siggi@!

siggi@: Please review the mechanical change in
components/browser_watcher/postmortem_report_collector_unittest.cc

gab@: Please review chrome/installer/* and components/prefs/*. I highlighted the
three instances of the same non-trivial change in my comments below. The rest is
replacing path() with GetPath() and clang-format.

Thanks!
Vaclav

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/c...
File chrome/installer/util/copy_tree_work_item.h (right):

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/c...
chrome/installer/util/copy_tree_work_item.h:83: base::FilePath backup_path_;
Adding the path separately allows the CopyTreeWorkItem to keep track of whether
|backup_dir_| was initialised or not. It was possible to do so via the
deprecated path(), but by design it is not possible via the replacement
GetPath(). The point is to move the responsibility of not writing anything under
the empty path from ScopedTempDir to CopyTreeWorkItem.

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/d...
File chrome/installer/util/delete_tree_work_item.h (right):

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/d...
chrome/installer/util/delete_tree_work_item.h:54: base::FilePath backup_path_;
Another instance of the change done to copy_tree_work_item.h.

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/m...
File chrome/installer/util/move_tree_work_item.h (right):

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/m...
chrome/installer/util/move_tree_work_item.h:63: base::FilePath backup_path_;
Another instance of the change done to copy_tree_work_item.h.

[Sigurður Ásgeirsson, 2016-09-28 13:48:13]

On 2016/09/28 13:10:25, vabr (Chromium) wrote:
> Hi gab@ and siggi@!
> 
> siggi@: Please review the mechanical change in
> components/browser_watcher/postmortem_report_collector_unittest.cc
> 
> gab@: Please review chrome/installer/* and components/prefs/*. I highlighted
the
> three instances of the same non-trivial change in my comments below. The rest
is
> replacing path() with GetPath() and clang-format.
> 
> Thanks!
> Vaclav
> 
>
https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/c...
> File chrome/installer/util/copy_tree_work_item.h (right):
> 
>
https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/c...
> chrome/installer/util/copy_tree_work_item.h:83: base::FilePath backup_path_;
> Adding the path separately allows the CopyTreeWorkItem to keep track of
whether
> |backup_dir_| was initialised or not. It was possible to do so via the
> deprecated path(), but by design it is not possible via the replacement
> GetPath(). The point is to move the responsibility of not writing anything
under
> the empty path from ScopedTempDir to CopyTreeWorkItem.
> 
>
https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/d...
> File chrome/installer/util/delete_tree_work_item.h (right):
> 
>
https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/d...
> chrome/installer/util/delete_tree_work_item.h:54: base::FilePath backup_path_;
> Another instance of the change done to copy_tree_work_item.h.
> 
>
https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/m...
> File chrome/installer/util/move_tree_work_item.h (right):
> 
>
https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/m...
> chrome/installer/util/move_tree_work_item.h:63: base::FilePath backup_path_;
> Another instance of the change done to copy_tree_work_item.h.

Browser watcher LGTM.

[gab, 2016-09-28 16:31:59]

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/c...
File chrome/installer/util/copy_tree_work_item.cc (right):

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/c...
chrome/installer/util/copy_tree_work_item.cc:79: backup_path_ =
backup_dir_.GetPath();
This results in an extra copy though. I did read your comment in the header but
I don't understand why GetPath() doesn't work below, feels like both use cases
would be happy with a const FilePath& to Append() to?

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/d...
File chrome/installer/util/delete_tree_work_item.cc (right):

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/d...
chrome/installer/util/delete_tree_work_item.cc:48: backup_path_ =
backup_dir_.GetPath();
If we're going to have a local copy of this variable, then it might as well
contain the full path constructed by this method (i.e. do the Append() below
into it) and have this method return const base::FilePath& which its users will
be happy with (and will result in less copies overall, yay!)

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/m...
File chrome/installer/util/move_tree_work_item.cc (right):

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/m...
chrome/installer/util/move_tree_work_item.cc:51: backup_path_ =
backup_dir_.GetPath();
Just like copy_tree_work_item.cc, looks like the only use case in this class
would be happy with GetPath()

[vabr (Chromium), 2016-09-28 20:06:36]

Thanks for the review so far!

@gab -- I answered two of your comments, please have a look. I will answer the
third one and any further follow-up tomorrow, when I'm back in the office and
able to upload modifications to the code.

Cheers,
Vaclav

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/c...
File chrome/installer/util/copy_tree_work_item.cc (right):

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/c...
chrome/installer/util/copy_tree_work_item.cc:79: backup_path_ =
backup_dir_.GetPath();
On 2016/09/28 16:31:59, gab wrote:
> This results in an extra copy though. I did read your comment in the header
but
> I don't understand why GetPath() doesn't work below, feels like both use cases
> would be happy with a const FilePath& to Append() to?

Here yes, but one of the unit tests [1] uses backup_path_ to verify that the
backup directory does not, in fact, exist. If copying the path is a concern, I
can replace the FilePath with just a bool, which would indicate whether the
backup directory has been created or not.

[1]
https://cs.chromium.org/chromium/src/chrome/installer/util/copy_tree_work_ite...

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/m...
File chrome/installer/util/move_tree_work_item.cc (right):

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/m...
chrome/installer/util/move_tree_work_item.cc:51: backup_path_ =
backup_dir_.GetPath();
On 2016/09/28 16:31:59, gab wrote:
> Just like copy_tree_work_item.cc, looks like the only use case in this class
> would be happy with GetPath()

Looking at the test failures in the first patch set of this CL, it seems that
MoveTreeWorkItem::RollbackImpl can be called when the backup directory has not
been created (in MoveTreeWorkItemTest.MoveAFile and
MoveTreeWorkItemTest.MoveDirectory). Calling GetPath() would trigger a DCHECK
then, on purpose. And actually, thinking about it now, RollbackImpl trying to
call base::Move on a path which comes from a directory which has not been
created seems like an error. What do you think?

[gab, 2016-09-28 20:25:25]

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/c...
File chrome/installer/util/copy_tree_work_item.cc (right):

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/c...
chrome/installer/util/copy_tree_work_item.cc:79: backup_path_ =
backup_dir_.GetPath();
On 2016/09/28 20:06:36, vabr (Chromium) wrote:
> On 2016/09/28 16:31:59, gab wrote:
> > This results in an extra copy though. I did read your comment in the header
> but
> > I don't understand why GetPath() doesn't work below, feels like both use
cases
> > would be happy with a const FilePath& to Append() to?
> 
> Here yes, but one of the unit tests [1] uses backup_path_ to verify that the
> backup directory does not, in fact, exist. If copying the path is a concern, I
> can replace the FilePath with just a bool, which would indicate whether the
> backup directory has been created or not.
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/installer/util/copy_tree_work_ite...

So a bool would do? I also haven't looked at the tests in detail but testing for
empty() on a private variable feels like it's reaching into the guts more than a
test should (i.e. it's abusing being a friend).

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/m...
File chrome/installer/util/move_tree_work_item.cc (right):

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/m...
chrome/installer/util/move_tree_work_item.cc:51: backup_path_ =
backup_dir_.GetPath();
On 2016/09/28 20:06:36, vabr (Chromium) wrote:
> On 2016/09/28 16:31:59, gab wrote:
> > Just like copy_tree_work_item.cc, looks like the only use case in this class
> > would be happy with GetPath()
> 
> Looking at the test failures in the first patch set of this CL, it seems that
> MoveTreeWorkItem::RollbackImpl can be called when the backup directory has not
> been created (in MoveTreeWorkItemTest.MoveAFile and
> MoveTreeWorkItemTest.MoveDirectory). Calling GetPath() would trigger a DCHECK
> then, on purpose. And actually, thinking about it now, RollbackImpl trying to
> call base::Move on a path which comes from a directory which has not been
> created seems like an error. What do you think?

Right, I think obtaining the variable should be inside an

if (moved_to_backup_ || source_moved_to_backup_) {}

block.

[vabr (Chromium), 2016-09-28 20:33:11]

Thanks, gab@. I will update the code following your comments  tomorrow and ping
you at that point for a re-review.

Cheers,
Vaclav

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/c...
File chrome/installer/util/copy_tree_work_item.cc (right):

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/c...
chrome/installer/util/copy_tree_work_item.cc:79: backup_path_ =
backup_dir_.GetPath();
On 2016/09/28 20:25:25, gab wrote:
> On 2016/09/28 20:06:36, vabr (Chromium) wrote:
> > On 2016/09/28 16:31:59, gab wrote:
> > > This results in an extra copy though. I did read your comment in the
header
> > but
> > > I don't understand why GetPath() doesn't work below, feels like both use
> cases
> > > would be happy with a const FilePath& to Append() to?
> > 
> > Here yes, but one of the unit tests [1] uses backup_path_ to verify that the
> > backup directory does not, in fact, exist. If copying the path is a concern,
I
> > can replace the FilePath with just a bool, which would indicate whether the
> > backup directory has been created or not.
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/chrome/installer/util/copy_tree_work_ite...
> 
> So a bool would do? I also haven't looked at the tests in detail but testing
for
> empty() on a private variable feels like it's reaching into the guts more than
a
> test should (i.e. it's abusing being a friend).

Happy to switch to a bool.

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/m...
File chrome/installer/util/move_tree_work_item.cc (right):

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/m...
chrome/installer/util/move_tree_work_item.cc:51: backup_path_ =
backup_dir_.GetPath();
On 2016/09/28 20:25:25, gab wrote:
> On 2016/09/28 20:06:36, vabr (Chromium) wrote:
> > On 2016/09/28 16:31:59, gab wrote:
> > > Just like copy_tree_work_item.cc, looks like the only use case in this
class
> > > would be happy with GetPath()
> > 
> > Looking at the test failures in the first patch set of this CL, it seems
that
> > MoveTreeWorkItem::RollbackImpl can be called when the backup directory has
not
> > been created (in MoveTreeWorkItemTest.MoveAFile and
> > MoveTreeWorkItemTest.MoveDirectory). Calling GetPath() would trigger a
DCHECK
> > then, on purpose. And actually, thinking about it now, RollbackImpl trying
to
> > call base::Move on a path which comes from a directory which has not been
> > created seems like an error. What do you think?
> 
> Right, I think obtaining the variable should be inside an
> 
> if (moved_to_backup_ || source_moved_to_backup_) {}
> 
> block.

Good idea, I will do that.

[vabr (Chromium), 2016-09-29 07:30:22]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 07:30:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 08:04:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 08:05:00]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[vabr (Chromium), 2016-09-29 08:23:35]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 08:23:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 09:04:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 09:04:13]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-09-29 09:11:03]

Hi gab@, please re-review.
Thanks!
Vaclav

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/d...
File chrome/installer/util/delete_tree_work_item.cc (right):

https://codereview.chromium.org/2379543002/diff/40001/chrome/installer/util/d...
chrome/installer/util/delete_tree_work_item.cc:48: backup_path_ =
backup_dir_.GetPath();
On 2016/09/28 16:31:59, gab wrote:
> If we're going to have a local copy of this variable, then it might as well
> contain the full path constructed by this method (i.e. do the Append() below
> into it) and have this method return const base::FilePath& which its users
will
> be happy with (and will result in less copies overall, yay!)

Done.

[gab, 2016-09-29 13:36:00]

lgtm % comment

https://codereview.chromium.org/2379543002/diff/120001/chrome/installer/util/...
File chrome/installer/util/move_tree_work_item.cc (right):

https://codereview.chromium.org/2379543002/diff/120001/chrome/installer/util/...
chrome/installer/util/move_tree_work_item.cc:111: backup =
backup_path_.GetPath().Append(dest_path_.BaseName());
The two ifs below only make sense within this condition as well so what I meant
is to:

if (moved_to_backup_ || source_moved_to_backup_) {
  base::FilePath backup = 
      backup_path_.GetPath().Append(dest_path_.BaseName());

   if (moved_to_backup_ && !base::Move(backup, dest_path_)) {
     ...
   }

   if (source_moved_to_backup_ && !base::Move(backup, source_path_)) {
     ...
   }
}

[vabr (Chromium), 2016-09-30 07:40:10]

Thanks!
Comment addressed, sending to the CQ.

Cheers,
Vaclav

https://codereview.chromium.org/2379543002/diff/120001/chrome/installer/util/...
File chrome/installer/util/move_tree_work_item.cc (right):

https://codereview.chromium.org/2379543002/diff/120001/chrome/installer/util/...
chrome/installer/util/move_tree_work_item.cc:111: backup =
backup_path_.GetPath().Append(dest_path_.BaseName());
On 2016/09/29 13:36:00, gab wrote:
> The two ifs below only make sense within this condition as well so what I
meant
> is to:
> 
> if (moved_to_backup_ || source_moved_to_backup_) {
>   base::FilePath backup = 
>       backup_path_.GetPath().Append(dest_path_.BaseName());
> 
>    if (moved_to_backup_ && !base::Move(backup, dest_path_)) {
>      ...
>    }
> 
>    if (source_moved_to_backup_ && !base::Move(backup, source_path_)) {
>      ...
>    }
> }

Done.

[vabr (Chromium), 2016-09-30 07:40:15]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-09-30 07:40:16]

The patchset sent to the CQ was uploaded after l-g-t-m from siggi@chromium.org,
gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2379543002/#ps160001
(title: "regroup ifs")

[commit-bot: I haz the power, 2016-09-30 07:40:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 08:55:58]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-30 08:59:40]

Description was changed from

==========
installer and some misc files: Change ScopedTempDir::path() to GetPath()

path() is being deprecated, GetPath() has better checking against wrong use.

For more context, see https://codereview.chromium.org/2275553005/.

BUG=640599
==========

to

==========
installer and some misc files: Change ScopedTempDir::path() to GetPath()

path() is being deprecated, GetPath() has better checking against wrong use.

For more context, see https://codereview.chromium.org/2275553005/.

BUG=640599

Committed: https://crrev.com/5d919d6e4866aa240c11d21b917f2ac8ea5d5eb8
Cr-Commit-Position: refs/heads/master@{#422068}
==========

[commit-bot: I haz the power, 2016-09-30 08:59:42]

Patchset 9 (id:??) landed as
https://crrev.com/5d919d6e4866aa240c11d21b917f2ac8ea5d5eb8
Cr-Commit-Position: refs/heads/master@{#422068}