//chrome/browser/chromeos: Change ScopedTempDir::path() to GetPath()

//chrome/browser/chromeos: 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/.

R=achuith@chromium.org
BUG=640599
Committed: https://crrev.com/d271a620617b6e77af1c0346eea87ca9f4e9d34b
Cr-Commit-Position: refs/heads/master@{#418533}

[vabr (Chromium), 2016-09-07 09:47:50]

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

[commit-bot: I haz the power, 2016-09-07 09:48:03]

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

[vabr (Chromium), 2016-09-07 10:05:37]

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

[commit-bot: I haz the power, 2016-09-07 10:06:03]

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-07 11:34:03]

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

[commit-bot: I haz the power, 2016-09-07 11:34:04]

Dry run: Exceeded global retry quota

[vabr (Chromium), 2016-09-07 13:42:23]

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

[commit-bot: I haz the power, 2016-09-07 13:42:31]

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

[vabr (Chromium), 2016-09-07 14:11:15]

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

[commit-bot: I haz the power, 2016-09-07 14:11:34]

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-07 14:26:57]

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

[commit-bot: I haz the power, 2016-09-07 14:26:58]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[vabr (Chromium), 2016-09-07 15:55:04]

vabr@chromium.org changed reviewers:
+ finnur@chromium.org, sorin@chromium.org

[vabr (Chromium), 2016-09-07 15:55:06]

Hello reviewers!

Please review the following:

@finnur: chrome/browser/extensions (just mechanical changes)

@sorin:
chrome/browser/component_updater/origin_trials_component_installer_unittest.cc
(mechanical as well)

@achuithb: the rest; it is mostly mechanical, I highlighted the only two
non-trivial changes by comments

Thanks!
Vaclav

https://codereview.chromium.org/2318023002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc
(right):

https://codereview.chromium.org/2318023002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:585:
ASSERT_TRUE(tmp_dir_.CreateUniqueTempDir());
I needed to move the CreateUniqueTempDir call from SetUpOnMainThread to here,
because the latter gets executed after the path of the directory is needed the
first time.

This was a real issue in the test before, because on line 606 of the original
patch, it would create a subdirectory in "" instead of the scoped temp dir.

Having the CreateUniqueTempDir matches what DriveFileSystemExtensionApiTest
above does.

https://codereview.chromium.org/2318023002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc
(right):

https://codereview.chromium.org/2318023002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc:187: //
Ensure that |path| is taken by |root_|. Cannot use |root_.GetPath()| to
I'm aware that this is a bit hacky.

I was tempted to leave out the check involving |current_root_path|, but I was
not sure whether CreateRootDirectory needs to handle the case when |root_|
already owns a different directory.

[Finnur, 2016-09-07 16:07:09]

extensions LGTM

[Sorin Jianu, 2016-09-07 16:46:11]

lgtm

Thank you! component updater lgtm.

[vabr (Chromium), 2016-09-08 10:29:15]

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

[commit-bot: I haz the power, 2016-09-08 10:29:36]

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-08 11:11:13]

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

[commit-bot: I haz the power, 2016-09-08 11:11:15]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-09-08 12:22:38]

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

[commit-bot: I haz the power, 2016-09-08 12:22: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-08 13:12:27]

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

[commit-bot: I haz the power, 2016-09-08 13:12:28]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-09-08 13:59:36]

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

[commit-bot: I haz the power, 2016-09-08 13:59: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-08 14:56:59]

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

[commit-bot: I haz the power, 2016-09-08 14:57:00]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-09-09 10:33:37]

Thanks Finnur and Sorin Jianu for the review!

achuith@ -- a gentle ping. Not to rush the review itself, but to check if this
is on your radar.

Thanks!
Vaclav

[achuithb, 2016-09-09 18:05:17]

On 2016/09/09 10:33:37, vabr (Chromium) wrote:
> Thanks Finnur and Sorin Jianu for the review!
> 
> achuith@ -- a gentle ping. Not to rush the review itself, but to check if this
> is on your radar.
> 
> Thanks!
> Vaclav

Thanks for the heads up - it had fallen off. I'll take a look later today

[vabr (Chromium), 2016-09-09 18:09:51]

On 2016/09/09 18:05:17, achuithb wrote:
> On 2016/09/09 10:33:37, vabr (Chromium) wrote:
> > Thanks Finnur and Sorin Jianu for the review!
> > 
> > achuith@ -- a gentle ping. Not to rush the review itself, but to check if
this
> > is on your radar.
> > 
> > Thanks!
> > Vaclav
> 
> Thanks for the heads up - it had fallen off. I'll take a look later today

Thanks for the update, much appreciated! :)
Vaclav

[vabr (Chromium), 2016-09-12 19:40:38]

Hello achuith@,

A gentle ping about this review. For your convenience I re-posted the two
comments highlighting the only two non-mechanical parts of the CL in the most
recent patch set.

Cheers,
Vaclav

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc
(right):

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:585:
ASSERT_TRUE(tmp_dir_.CreateUniqueTempDir());
I needed to move the CreateUniqueTempDir call from SetUpOnMainThread to here,
because the latter gets executed after the path of the directory is needed the
first time.

This was a real issue in the test before, because on line 606 of the original
patch, it would create a subdirectory in "" instead of the scoped temp dir.

Having the CreateUniqueTempDir matches what DriveFileSystemExtensionApiTest
above does.

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc
(right):

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc:187: //
Ensure that |path| is taken by |root_|. Cannot use |root_.GetPath()| to
I'm aware that this is a bit hacky.

I was tempted to leave out the check involving |current_root_path|, but I was
not sure whether CreateRootDirectory needs to handle the case when |root_|
already owns a different directory.

[vabr (Chromium), 2016-09-13 20:25:44]

Hello achuith@,

Just a friendly reminder about this CL. Thanks! :)

Vaclav

On 2016/09/12 19:40:38, vabr (Chromium) wrote:
> Hello achuith@,
> 
> A gentle ping about this review. For your convenience I re-posted the two
> comments highlighting the only two non-mechanical parts of the CL in the most
> recent patch set.
> 
> Cheers,
> Vaclav
> 
>
https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
> File chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc
> (right):
> 
>
https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
> chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:585:
> ASSERT_TRUE(tmp_dir_.CreateUniqueTempDir());
> I needed to move the CreateUniqueTempDir call from SetUpOnMainThread to here,
> because the latter gets executed after the path of the directory is needed the
> first time.
> 
> This was a real issue in the test before, because on line 606 of the original
> patch, it would create a subdirectory in "" instead of the scoped temp dir.
> 
> Having the CreateUniqueTempDir matches what DriveFileSystemExtensionApiTest
> above does.
> 
>
https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
> File chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc
> (right):
> 
>
https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
> chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc:187: //
> Ensure that |path| is taken by |root_|. Cannot use |root_.GetPath()| to
> I'm aware that this is a bit hacky.
> 
> I was tempted to leave out the check involving |current_root_path|, but I was
> not sure whether CreateRootDirectory needs to handle the case when |root_|
> already owns a different directory.

[achuithb, 2016-09-13 20:55:20]

I think it may be better to land the non-trivial changes separately to guard
against reverts.

lgtm otherwise.

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc
(right):

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:548: return
new drive::DriveIntegrationService(profile, NULL,
replace with nullptr since you're here

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:550:
test_cache_root_.GetPath(), NULL);
nullptr

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:585:
ASSERT_TRUE(tmp_dir_.CreateUniqueTempDir());
On 2016/09/12 19:40:37, vabr (Chromium) wrote:
> I needed to move the CreateUniqueTempDir call from SetUpOnMainThread to here,
> because the latter gets executed after the path of the directory is needed the
> first time.
> 
> This was a real issue in the test before, because on line 606 of the original
> patch, it would create a subdirectory in "" instead of the scoped temp dir.
> 
> Having the CreateUniqueTempDir matches what DriveFileSystemExtensionApiTest
> above does.

Would've been nice to pull this into a separate CL, but ok.

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:711: return
new drive::DriveIntegrationService(profile, NULL,
nullptrs in this invocation

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc
(right):

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc:187: //
Ensure that |path| is taken by |root_|. Cannot use |root_.GetPath()| to
On 2016/09/12 19:40:37, vabr (Chromium) wrote:
> I'm aware that this is a bit hacky.
> 
> I was tempted to leave out the check involving |current_root_path|, but I was
> not sure whether CreateRootDirectory needs to handle the case when |root_|
> already owns a different directory.

I'm really not sure either. Is it possible to pull this out into a separate CL?

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc:190:
base::FilePath current_root_path = root_.Take();
const

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc:199: const
base::FilePath root_path() { return root_.GetPath(); }
Get rid of this const return type, but make the function const. Please also make
name() a const function.

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/fileapi/external_file_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/external_file_url_request_job_unittest.cc:221:
profile, NULL, drive_service, drive_mount_name,
make this nullptr

[vabr (Chromium), 2016-09-14 10:07:22]

Thanks, achuith@, for the review!

I agree and will separate the non-trivial changes into different CLs. The plan
is now:
* Keep the trivial changes here and land.
* Create a CL for
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc, address
your comments there, TBR you and land.
* Create a CL for chrome/browser/chromeos/file_manager/*, address your comments
there, TBR you and land.

Cheers,
Vaclav

[vabr (Chromium), 2016-09-14 10:12:39]

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

[commit-bot: I haz the power, 2016-09-14 10:12:55]

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-14 10:56:39]

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

[commit-bot: I haz the power, 2016-09-14 10:56:41]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-09-14 10:58:20]

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc
(right):

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:548: return
new drive::DriveIntegrationService(profile, NULL,
On 2016/09/13 20:55:19, achuithb wrote:
> replace with nullptr since you're here

Done in https://codereview.chromium.org/2338793004.

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:550:
test_cache_root_.GetPath(), NULL);
On 2016/09/13 20:55:19, achuithb wrote:
> nullptr

Done in https://codereview.chromium.org/2338793004.

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:585:
ASSERT_TRUE(tmp_dir_.CreateUniqueTempDir());
On 2016/09/13 20:55:19, achuithb wrote:
> On 2016/09/12 19:40:37, vabr (Chromium) wrote:
> > I needed to move the CreateUniqueTempDir call from SetUpOnMainThread to
here,
> > because the latter gets executed after the path of the directory is needed
the
> > first time.
> > 
> > This was a real issue in the test before, because on line 606 of the
original
> > patch, it would create a subdirectory in "" instead of the scoped temp dir.
> > 
> > Having the CreateUniqueTempDir matches what DriveFileSystemExtensionApiTest
> > above does.
> 
> Would've been nice to pull this into a separate CL, but ok.

Done: https://codereview.chromium.org/2338793004.

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:711: return
new drive::DriveIntegrationService(profile, NULL,
On 2016/09/13 20:55:19, achuithb wrote:
> nullptrs in this invocation

Done in https://codereview.chromium.org/2338793004.

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc
(right):

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc:187: //
Ensure that |path| is taken by |root_|. Cannot use |root_.GetPath()| to
On 2016/09/13 20:55:19, achuithb wrote:
> On 2016/09/12 19:40:37, vabr (Chromium) wrote:
> > I'm aware that this is a bit hacky.
> > 
> > I was tempted to leave out the check involving |current_root_path|, but I
was
> > not sure whether CreateRootDirectory needs to handle the case when |root_|
> > already owns a different directory.
> 
> I'm really not sure either. Is it possible to pull this out into a separate
CL?

Sure: https://codereview.chromium.org/2345473002
I realised that CreateRootDirectory is potentially called repeatedly from
FakeTestVolume, so I made it handle repeated calls. But there is no need to
assume that root_ was initialised from anywhere else, so I added a Boolean
member variable to track initialisation done by CreateRootDirectory, to make the
code clearer.

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc:190:
base::FilePath current_root_path = root_.Take();
On 2016/09/13 20:55:19, achuithb wrote:
> const

Now obsolete after the above described changes (made in
https://codereview.chromium.org/2345473002).

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc:199: const
base::FilePath root_path() { return root_.GetPath(); }
On 2016/09/13 20:55:19, achuithb wrote:
> Get rid of this const return type, but make the function const. Please also
make
> name() a const function.

Looking at the callsites, they never move away the result, and often require a
const reference, so returning by value (const or not) would do one more copy
than needed. So I made this a const ref instead in
https://codereview.chromium.org/2345473002.

(On the topic of returning const values: sometimes returning a const value has
been done on purpose to prevent assigning to it, see
http://crbug.com/393155#c4.)

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/fileapi/external_file_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/2318023002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/external_file_url_request_job_unittest.cc:221:
profile, NULL, drive_service, drive_mount_name,
On 2016/09/13 20:55:19, achuithb wrote:
> make this nullptr

Done.

[vabr (Chromium), 2016-09-14 10:58:33]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-09-14 10:58:33]

The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org,
finnur@chromium.org, achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2318023002/#ps180001
(title: "Address comment and also remove non-trivial cases")

[commit-bot: I haz the power, 2016-09-14 10:58:46]

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-14 11:04:29]

Committed patchset #10 (id:180001)

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

Description was changed from

==========
//chrome/browser/chromeos: 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/.

R=achuith@chromium.org
BUG=640599
==========

to

==========
//chrome/browser/chromeos: 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/.

R=achuith@chromium.org
BUG=640599

Committed: https://crrev.com/d271a620617b6e77af1c0346eea87ca9f4e9d34b
Cr-Commit-Position: refs/heads/master@{#418533}
==========

[commit-bot: I haz the power, 2016-09-14 11:07:16]

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