Chrome Cleaner: download the Chrome Cleaner executable in an empty directory.

Chrome Cleaner: download the Chrome Cleaner executable in an empty directory.

BUG=730073
Review-Url: https://codereview.chromium.org/2929483002
Cr-Commit-Position: refs/heads/master@{#478741}
Committed: https://chromium.googlesource.com/chromium/src/+/39fbf000fd660ce059a62d68b02f51cc0384ca0c

[alito, 2017-06-06 17:35:52]

alito@chromium.org changed reviewers:
+ csharp@chromium.org, robertshield@chromium.org

[alito, 2017-06-06 17:35:52]

PTAL!

[csharp, 2017-06-06 19:01:57]

https://codereview.chromium.org/2929483002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc
(right):

https://codereview.chromium.org/2929483002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc:52: //
Will be called back on the same sequence as this object was created on.
nit: sequence -> task runner sequence? or maybe SequencedTaskRunner (I just
haven't seen sequence in a comment like this before, not sure how clear it is.)

Same idea for line 70

https://codereview.chromium.org/2929483002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc:100:
return scoped_temp_dir_->CreateUniqueTempDir() &&
AS discussed offline, you should create the path name manually using
GetTempDirPrefix so you can add a common string to all the temp dirs, so they
can be easily grabbed

https://codereview.chromium.org/2929483002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc:102:
&temp_file_);
Does a file need to be created here, or is this just for the temp file name? If
just for the name, why not just create it yourself? Something like
ASCIIToUTF16(base::GenerateGUID())

https://codereview.chromium.org/2929483002/diff/20001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win_unittest.cc
(right):

https://codereview.chromium.org/2929483002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win_unittest.cc:113:
// Set up the fetcher to return a not-found failure.
nit: not-found -> HTTP_NOT_FOUND (same for line 125)

https://codereview.chromium.org/2929483002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win_unittest.cc:125:
// Set up the fetcher to return failure other than not-found.
nit: not-found -> not found (?)

[alito, 2017-06-06 19:32:45]

Chris's comments addressed. PTAnL.

https://codereview.chromium.org/2929483002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc
(right):

https://codereview.chromium.org/2929483002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc:52: //
Will be called back on the same sequence as this object was created on.
On 2017/06/06 19:01:53, csharp wrote:
> nit: sequence -> task runner sequence? or maybe SequencedTaskRunner (I just
> haven't seen sequence in a comment like this before, not sure how clear it
is.)
> 
> Same idea for line 70

Sequence seems to be fairly standard lingo now. Taken from the comments in
post_task.h:

"Posts |task| to the TaskScheduler and posts |reply| on the caller's execution
context (i.e. same sequence or thread and same TaskTraits if applicable) when
|task| completes."

https://codereview.chromium.org/2929483002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc:100:
return scoped_temp_dir_->CreateUniqueTempDir() &&
On 2017/06/06 19:01:53, csharp wrote:
> AS discussed offline, you should create the path name manually using
> GetTempDirPrefix so you can add a common string to all the temp dirs, so they
> can be easily grabbed

Done.

https://codereview.chromium.org/2929483002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc:102:
&temp_file_);
On 2017/06/06 19:01:54, csharp wrote:
> Does a file need to be created here, or is this just for the temp file name?
If
> just for the name, why not just create it yourself? Something like
> ASCIIToUTF16(base::GenerateGUID())

Done.

https://codereview.chromium.org/2929483002/diff/20001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win_unittest.cc
(right):

https://codereview.chromium.org/2929483002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win_unittest.cc:113:
// Set up the fetcher to return a not-found failure.
On 2017/06/06 19:01:55, csharp wrote:
> nit: not-found -> HTTP_NOT_FOUND (same for line 125)

Done.

https://codereview.chromium.org/2929483002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win_unittest.cc:125:
// Set up the fetcher to return failure other than not-found.
On 2017/06/06 19:01:54, csharp wrote:
> nit: not-found -> not found (?)

Done.

[csharp, 2017-06-06 19:43:24]

lgtm

https://codereview.chromium.org/2929483002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc
(right):

https://codereview.chromium.org/2929483002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc:119:
base::ASCIIToUTF16(base::GenerateGUID()) + L".tmp");
Would it make sense to make the file type exe, since it is an executable?

[alito, 2017-06-06 19:55:53]

https://codereview.chromium.org/2929483002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc
(right):

https://codereview.chromium.org/2929483002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc:119:
base::ASCIIToUTF16(base::GenerateGUID()) + L".tmp");
On 2017/06/06 19:43:24, csharp wrote:
> Would it make sense to make the file type exe, since it is an executable?

As discussed, it might be slightly preferable to add an executable extension
once we know that the file has been downloaded correctly.

[alito, 2017-06-08 02:53:40]

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

[commit-bot: I haz the power, 2017-06-08 02:53:57]

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, 2017-06-08 02:55:53]

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

[commit-bot: I haz the power, 2017-06-08 02:55:54]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alito, 2017-06-08 03:09:46]

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

[commit-bot: I haz the power, 2017-06-08 03:09:58]

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, 2017-06-08 03:37:07]

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

[commit-bot: I haz the power, 2017-06-08 03:37:08]

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

[alito, 2017-06-08 03:59:09]

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

[commit-bot: I haz the power, 2017-06-08 03:59:24]

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, 2017-06-08 04:54:52]

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

[commit-bot: I haz the power, 2017-06-08 04:54:53]

Dry run: This issue passed the CQ dry run.

[robertshield, 2017-06-12 14:36:42]

lgtm

https://codereview.chromium.org/2929483002/diff/140001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win_unittest.cc
(right):

https://codereview.chromium.org/2929483002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win_unittest.cc:64:
// any created temp files.
How does this intercept and change the file path? Looks like it just stores it
for future cleanup?

[alito, 2017-06-12 19:05:36]

All done. Will try to land now.

https://codereview.chromium.org/2929483002/diff/140001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win_unittest.cc
(right):

https://codereview.chromium.org/2929483002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win_unittest.cc:64:
// any created temp files.
On 2017/06/12 14:36:42, robertshield wrote:
> How does this intercept and change the file path? Looks like it just stores it
> for future cleanup?

Forgot to update the comment after some changes. Updated now. Thx.

[alito, 2017-06-12 19:05:44]

The CQ bit was checked by alito@chromium.org

[alito, 2017-06-12 19:05:44]

The patchset sent to the CQ was uploaded after l-g-t-m from csharp@chromium.org,
robertshield@chromium.org
Link to the patchset: https://codereview.chromium.org/2929483002/#ps160001
(title: "Nits")

[commit-bot: I haz the power, 2017-06-12 19:06: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, 2017-06-12 20:21:09]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1497294344276490,
"parent_rev": "68a502104896fc2bb14fc3903afe31d275e2ffda", "commit_rev":
"39fbf000fd660ce059a62d68b02f51cc0384ca0c"}

[commit-bot: I haz the power, 2017-06-12 20:21:26]

Description was changed from

==========
Chrome Cleaner: download the Chrome Cleaner executable in an empty directory.

BUG=730073
==========

to

==========
Chrome Cleaner: download the Chrome Cleaner executable in an empty directory.

BUG=730073

Review-Url: https://codereview.chromium.org/2929483002
Cr-Commit-Position: refs/heads/master@{#478741}
Committed:
https://chromium.googlesource.com/chromium/src/+/39fbf000fd660ce059a62d68b02f...
==========

[commit-bot: I haz the power, 2017-06-12 20:21:28]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/39fbf000fd660ce059a62d68b02f...