Downloads: replace BrowserThread::FILE with task scheduler.

Downloads: replace BrowserThread::FILE with task scheduler.

TBR=benjhayden@chromium.org
BUG=689520
Review-Url: https://codereview.chromium.org/2890853002
Cr-Commit-Position: refs/heads/master@{#488725}
Committed: https://chromium.googlesource.com/chromium/src/+/2b05ad10d4dd7d32f1501f6382f698285dbed01b

[Sigurður Ásgeirsson, 2017-05-19 20:23:10]

Patchset #2 (id:20001) has been deleted

[gab, 2017-06-09 18:25:27]

Any chance we can land this? Thanks!

[gab, 2017-06-13 20:12:58]

On 2017/06/09 18:25:27, gab wrote:
> Any chance we can land this? Thanks!

bueller :)?

[Sigurður Ásgeirsson, 2017-06-14 12:36:28]

Sorry, have been busy with other things, will circle back to this shortly.

On Tue, Jun 13, 2017 at 4:12 PM <gab@chromium.org> wrote:

> On 2017/06/09 18:25:27, gab wrote:
> > Any chance we can land this? Thanks!
>
> bueller :)?
>
> https://codereview.chromium.org/2890853002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sigurður Ásgeirsson, 2017-06-20 12:54:01]

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

[commit-bot: I haz the power, 2017-06-20 12:54:11]

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-20 13:04:07]

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

[commit-bot: I haz the power, 2017-06-20 13:04:08]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Sigurður Ásgeirsson, 2017-06-20 13:50:03]

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

[commit-bot: I haz the power, 2017-06-20 13:50:18]

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-20 14:02:50]

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

[commit-bot: I haz the power, 2017-06-20 14:02:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sigurður Ásgeirsson, 2017-06-20 14:08:29]

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

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

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-20 14:19:09]

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

[commit-bot: I haz the power, 2017-06-20 14:19:10]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sigurður Ásgeirsson, 2017-06-20 14:26:47]

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

[commit-bot: I haz the power, 2017-06-20 14:26: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-20 14:39:09]

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

[commit-bot: I haz the power, 2017-06-20 14:39:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sigurður Ásgeirsson, 2017-06-20 15:52:20]

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

[commit-bot: I haz the power, 2017-06-20 15:52:32]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-06-20 15:59:21]

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

[commit-bot: I haz the power, 2017-06-20 15:59: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, 2017-06-20 16:09:42]

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

[commit-bot: I haz the power, 2017-06-20 16:09:43]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Sigurður Ásgeirsson, 2017-06-20 16:51:27]

Hey Gab,

I'd like to sit down for a sec and talk through some of the "finer" points
of this, when you have a moment.

Siggi

On Fri, Jun 9, 2017 at 2:25 PM <gab@chromium.org> wrote:

> Any chance we can land this? Thanks!
>
> https://codereview.chromium.org/2890853002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sigurður Ásgeirsson, 2017-06-21 12:32:41]

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

[commit-bot: I haz the power, 2017-06-21 12:32:52]

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-21 13:06:05]

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

[commit-bot: I haz the power, 2017-06-21 13:06:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sigurður Ásgeirsson, 2017-06-21 15:16:35]

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

[commit-bot: I haz the power, 2017-06-21 15:16:48]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-06-21 15:48:26]

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

[commit-bot: I haz the power, 2017-06-21 15:48:39]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-06-21 15:49:03]

Description was changed from

==========
Downloads: replace BrowserThread::FILE with task scheduler.

BUG=689520
==========

to

==========
Downloads: replace BrowserThread::FILE with task scheduler.

BUG=689520
==========

[Sigurður Ásgeirsson, 2017-06-21 15:49:03]

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

[Sigurður Ásgeirsson, 2017-06-21 15:49:30]

K, Francois, Gab, I think this is ready for the first once-over.

[commit-bot: I haz the power, 2017-06-21 15:57:24]

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

[commit-bot: I haz the power, 2017-06-21 15:57:28]

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...)

[Sigurður Ásgeirsson, 2017-06-21 16:52:47]

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

[commit-bot: I haz the power, 2017-06-21 16:52:59]

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-21 17:23:35]

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

[commit-bot: I haz the power, 2017-06-21 17:23:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sigurður Ásgeirsson, 2017-06-21 18:31:49]

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

[commit-bot: I haz the power, 2017-06-21 18:32:11]

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-21 19:11:47]

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

[commit-bot: I haz the power, 2017-06-21 19:11:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sigurður Ásgeirsson, 2017-06-21 19:20:13]

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

[commit-bot: I haz the power, 2017-06-21 19:20:35]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-06-21 19:29:14]

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

[commit-bot: I haz the power, 2017-06-21 19:29:27]

Dry run: CQ is trying da patch.

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

[gab, 2017-06-21 19:42:24]

This is great, thanks!

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
File content/browser/download/base_file.h (right):

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/base_file.h:38: // destruction) must occur on the
download task runner.
"on the same sequence"

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
File content/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_browsertest.cc:1574: // factory, which is a
file thread object.
update reference to "file thread"

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
File content/browser/download/download_item_impl_unittest.cc (right):

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_item_impl_unittest.cc:397: void
RunAllPendingInMessageLoops() { RunAllBlockingPoolTasksUntilIdle(); }
inline below instead of having this indirection?

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_item_impl_unittest.cc:820:
.WillOnce(ScheduleCallbackWithParam(DOWNLOAD_INTERRUPT_REASON_NONE));
Why is this changing?

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
File content/browser/download/download_task_runner.cc (right):

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_task_runner.cc:17: {base::MayBlock(),
base::TaskPriority::BACKGROUND});
Isn't this USER_VISIBLE?

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_task_runner.cc:21: LAZY_INSTANCE_INITIALIZER;
Francois added a new API to do all of this in one line (instead of
struct+LazyInstance):

#include "base/task_scheduler/lazy_task_runner.h"

LazySingleThreadTaskRunner g_download_task_runner =
    LAZY_SINGLE_THREAD_TASK_RUNNER_INITIALIZER(
        base::TaskTraits(base::MayBlock(), 
                         base::TaskPriority::BACKGROUND),
    base::SingleThreadTaskRunnerThreadMode::SHARED);

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
File content/browser/download/download_task_runner.h (right):

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_task_runner.h:14: CONTENT_EXPORT
scoped_refptr<base::SingleThreadTaskRunner>
What's the reason this couldn't be a SequencedTaskRunner again? We fixed many of
those recently so maybe it's fine now? If not, add a TODO with a bug flagged as
a blocker for https://crbug.com/675631 which explains what prevents this today.

[commit-bot: I haz the power, 2017-06-21 20:04:24]

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

[commit-bot: I haz the power, 2017-06-21 20:04:25]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sigurður Ásgeirsson, 2017-06-22 12:36:15]

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

[commit-bot: I haz the power, 2017-06-22 12:36:25]

Dry run: CQ is trying da patch.

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

[fdoray, 2017-06-22 13:01:35]

https://codereview.chromium.org/2890853002/diff/290001/content/browser/downlo...
File content/browser/download/download_browsertest.cc (left):

https://codereview.chromium.org/2890853002/diff/290001/content/browser/downlo...
content/browser/download/download_browsertest.cc:1794:
RunAllPendingInMessageLoop(BrowserThread::IO);
RunAllBlockingPoolTasksUntilIdle() doesn't flush the IO thread, so you need to
keep this line.

https://codereview.chromium.org/2890853002/diff/290001/content/browser/downlo...
File content/browser/download/download_file_impl.h (right):

https://codereview.chromium.org/2890853002/diff/290001/content/browser/downlo...
content/browser/download/download_file_impl.h:41: // (including destruction)
must occur on the download task runner.
s/download task runner/same sequence/ ?
Since you're using a SequenceChecker.

https://codereview.chromium.org/2890853002/diff/290001/content/browser/downlo...
File content/browser/download/download_item_impl_unittest.cc (right):

https://codereview.chromium.org/2890853002/diff/290001/content/browser/downlo...
content/browser/download/download_item_impl_unittest.cc:270: ~DownloadItemTest()
{ RunAllBlockingPoolTasksUntilIdle(); }
Redundant. TestBrowserThreadBundle does this in its destructor
https://cs.chromium.org/chromium/src/content/public/test/test_browser_thread_...

[commit-bot: I haz the power, 2017-06-22 13:06:18]

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

[commit-bot: I haz the power, 2017-06-22 13:06:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sigurður Ásgeirsson, 2017-06-22 14:42:23]

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

[commit-bot: I haz the power, 2017-06-22 14:42:40]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-06-22 14:50:14]

Ooof, what a slog!

Please take another look?

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
File content/browser/download/base_file.h (right):

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/base_file.h:38: // destruction) must occur on the
download task runner.
On 2017/06/21 19:42:24, gab wrote:
> "on the same sequence"

Done.

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
File content/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_browsertest.cc:1574: // factory, which is a
file thread object.
On 2017/06/21 19:42:24, gab wrote:
> update reference to "file thread"

Done.

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
File content/browser/download/download_item_impl_unittest.cc (right):

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_item_impl_unittest.cc:397: void
RunAllPendingInMessageLoops() { RunAllBlockingPoolTasksUntilIdle(); }
On 2017/06/21 19:42:24, gab wrote:
> inline below instead of having this indirection?

Done.

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_item_impl_unittest.cc:820:
.WillOnce(ScheduleCallbackWithParam(DOWNLOAD_INTERRUPT_REASON_NONE));
On 2017/06/21 19:42:24, gab wrote:
> Why is this changing?

As-is, the default mock setup fires the callback on-thread. The other places
where this a mock is used, it goes through a factory function that has this, but
that's apparently not applicable here.

... time passes ...

Actually the default behavior in the mock has other fallout, fixing at source.

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
File content/browser/download/download_task_runner.cc (right):

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_task_runner.cc:17: {base::MayBlock(),
base::TaskPriority::BACKGROUND});
On 2017/06/21 19:42:24, gab wrote:
> Isn't this USER_VISIBLE?

I'm not sure either way, the activity on this sequence is mostly about writing
files, though notifications go back to the user. Downloads aren't ever terribly
interactive though.

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_task_runner.cc:21: LAZY_INSTANCE_INITIALIZER;
On 2017/06/21 19:42:24, gab wrote:
> Francois added a new API to do all of this in one line (instead of
> struct+LazyInstance):
> 
> #include "base/task_scheduler/lazy_task_runner.h"
> 
> LazySingleThreadTaskRunner g_download_task_runner =
>     LAZY_SINGLE_THREAD_TASK_RUNNER_INITIALIZER(
>         base::TaskTraits(base::MayBlock(), 
>                          base::TaskPriority::BACKGROUND),
>     base::SingleThreadTaskRunnerThreadMode::SHARED);

Done.

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
File content/browser/download/download_task_runner.h (right):

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_task_runner.h:14: CONTENT_EXPORT
scoped_refptr<base::SingleThreadTaskRunner>
On 2017/06/21 19:42:24, gab wrote:
> What's the reason this couldn't be a SequencedTaskRunner again? We fixed many
of
> those recently so maybe it's fine now? If not, add a TODO with a bug flagged
as
> a blocker for https://crbug.com/675631 which explains what prevents this
today.

There was something that barfed on base::DelayTimer, as I recall. Is that fixed
now?

https://codereview.chromium.org/2890853002/diff/290001/content/browser/downlo...
File content/browser/download/download_browsertest.cc (left):

https://codereview.chromium.org/2890853002/diff/290001/content/browser/downlo...
content/browser/download/download_browsertest.cc:1794:
RunAllPendingInMessageLoop(BrowserThread::IO);
On 2017/06/22 13:01:35, fdoray wrote:
> RunAllBlockingPoolTasksUntilIdle() doesn't flush the IO thread, so you need to
> keep this line.

Done.

https://codereview.chromium.org/2890853002/diff/290001/content/browser/downlo...
File content/browser/download/download_file_impl.h (right):

https://codereview.chromium.org/2890853002/diff/290001/content/browser/downlo...
content/browser/download/download_file_impl.h:41: // (including destruction)
must occur on the download task runner.
On 2017/06/22 13:01:35, fdoray wrote:
> s/download task runner/same sequence/ ?
> Since you're using a SequenceChecker.

Done.

https://codereview.chromium.org/2890853002/diff/290001/content/browser/downlo...
File content/browser/download/download_item_impl_unittest.cc (right):

https://codereview.chromium.org/2890853002/diff/290001/content/browser/downlo...
content/browser/download/download_item_impl_unittest.cc:270: ~DownloadItemTest()
{ RunAllBlockingPoolTasksUntilIdle(); }
On 2017/06/22 13:01:35, fdoray wrote:
> Redundant. TestBrowserThreadBundle does this in its destructor
>
https://cs.chromium.org/chromium/src/content/public/test/test_browser_thread_...

Done.

[Sigurður Ásgeirsson, 2017-06-22 14:51:17]

NB: this is blocked on https://chromium-review.googlesource.com/c/545095/, which
causes occasional NULL pointer crashes.

[gab, 2017-06-22 16:03:59]

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
File content/browser/download/download_task_runner.h (right):

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_task_runner.h:14: CONTENT_EXPORT
scoped_refptr<base::SingleThreadTaskRunner>
On 2017/06/22 14:50:09, Sigurður Ásgeirsson wrote:
> On 2017/06/21 19:42:24, gab wrote:
> > What's the reason this couldn't be a SequencedTaskRunner again? We fixed
many
> of
> > those recently so maybe it's fine now? If not, add a TODO with a bug flagged
> as
> > a blocker for https://crbug.com/675631 which explains what prevents this
> today.
> 
> There was something that barfed on base::DelayTimer, as I recall. Is that
fixed
> now?

Yes base::Timer is now sequence-friendly

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
content/browser/download/download_file_impl.cc:22: #include
"content/browser/download/download_task_runner.h"
rm (see below)

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
content/browser/download/download_file_impl.cc:475:
GetDownloadTaskRunner()->PostTask(
Use SequencedTaskRunnerHandle::Get() here instead of GetDownloadTaskRunner()
since this code is now merely affine to a sequence (this is required since
|weak_factory_|'s WeakPtrs below are not thread-safe so this would be an invalid
use if SequencedTaskRunnerHandle::Get() != GetDownloadTaskRunner(). And
SequencedTaskRunnerHandle::Get() also semantically matches "post a task so we
come back" better :).

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
File content/browser/download/download_task_runner.cc (right):

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
content/browser/download/download_task_runner.cc:15:
(base::TaskTraits(base::MayBlock(), base::TaskPriority::BACKGROUND)));
Can remove outer set of () here I think.

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
File content/browser/download/download_task_runner.h (right):

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
content/browser/download/download_task_runner.h:14: CONTENT_EXPORT
scoped_refptr<base::SequencedTaskRunner> GetDownloadTaskRunner();
None of the callers require a refptr (some are even forced to .get()), I'm going
to update the LazyTaskRunner API to return a raw ptr by default instead to avoid
unnecessary ref churn. In the meantime if you're so inclined this could return a
SequencedTaskRunner* and the impl can .Get().get() with a TODO assigned to me +
issue 730170.

[gab, 2017-06-22 16:10:18]

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
File content/browser/download/download_task_runner.h (right):

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
content/browser/download/download_task_runner.h:14: CONTENT_EXPORT
scoped_refptr<base::SequencedTaskRunner> GetDownloadTaskRunner();
On 2017/06/22 16:03:59, gab wrote:
> None of the callers require a refptr (some are even forced to .get()), I'm
going
> to update the LazyTaskRunner API to return a raw ptr by default instead to
avoid
> unnecessary ref churn. In the meantime if you're so inclined this could return
a
> SequencedTaskRunner* and the impl can .Get().get() with a TODO assigned to me
+
> issue 730170.

Nvm, looks like you do need the refptr in one place for CreateByteStream().

[gab, 2017-06-22 17:10:08]

Forgot to reply to that one..

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
File content/browser/download/download_task_runner.cc (right):

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_task_runner.cc:17: {base::MayBlock(),
base::TaskPriority::BACKGROUND});
On 2017/06/22 14:50:09, Sigurður Ásgeirsson wrote:
> On 2017/06/21 19:42:24, gab wrote:
> > Isn't this USER_VISIBLE?
> 
> I'm not sure either way, the activity on this sequence is mostly about writing
> files, though notifications go back to the user. Downloads aren't ever
terribly
> interactive though.

Right not interactive but user visible, i.e. not USER_BLOCKING but USER_VISIBLE

[Sigurður Ásgeirsson, 2017-06-22 18:19:16]

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

[commit-bot: I haz the power, 2017-06-22 18:19:45]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-06-22 18:47:29]

Thanks, the failures are down to a pretty few tests now, but sadly I'm leaving
on 3 weeks of vacation. If someone wants to pick up from here in the meantime,
just leave a comment on the CL so that I know to leave it.

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
File content/browser/download/download_task_runner.cc (right):

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_task_runner.cc:17: {base::MayBlock(),
base::TaskPriority::BACKGROUND});
On 2017/06/22 17:10:08, gab (OOO Friday) wrote:
> On 2017/06/22 14:50:09, Sigurður Ásgeirsson wrote:
> > On 2017/06/21 19:42:24, gab wrote:
> > > Isn't this USER_VISIBLE?
> > 
> > I'm not sure either way, the activity on this sequence is mostly about
writing
> > files, though notifications go back to the user. Downloads aren't ever
> terribly
> > interactive though.
> 
> Right not interactive but user visible, i.e. not USER_BLOCKING but
USER_VISIBLE

Done.

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
File content/browser/download/download_task_runner.h (right):

https://codereview.chromium.org/2890853002/diff/260001/content/browser/downlo...
content/browser/download/download_task_runner.h:14: CONTENT_EXPORT
scoped_refptr<base::SingleThreadTaskRunner>
On 2017/06/22 16:03:59, gab (OOO Friday) wrote:
> On 2017/06/22 14:50:09, Sigurður Ásgeirsson wrote:
> > On 2017/06/21 19:42:24, gab wrote:
> > > What's the reason this couldn't be a SequencedTaskRunner again? We fixed
> many
> > of
> > > those recently so maybe it's fine now? If not, add a TODO with a bug
flagged
> > as
> > > a blocker for https://crbug.com/675631 which explains what prevents this
> > today.
> > 
> > There was something that barfed on base::DelayTimer, as I recall. Is that
> fixed
> > now?
> 
> Yes base::Timer is now sequence-friendly

Acknowledged.

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
content/browser/download/download_file_impl.cc:22: #include
"content/browser/download/download_task_runner.h"
On 2017/06/22 16:03:59, gab (OOO Friday) wrote:
> rm (see below)

Done.

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
content/browser/download/download_file_impl.cc:475:
GetDownloadTaskRunner()->PostTask(
On 2017/06/22 16:03:59, gab (OOO Friday) wrote:
> Use SequencedTaskRunnerHandle::Get() here instead of GetDownloadTaskRunner()
> since this code is now merely affine to a sequence (this is required since
> |weak_factory_|'s WeakPtrs below are not thread-safe so this would be an
invalid
> use if SequencedTaskRunnerHandle::Get() != GetDownloadTaskRunner(). And
> SequencedTaskRunnerHandle::Get() also semantically matches "post a task so we
> come back" better :).

Nice, this also cuts down on the sequence hopping in unittests.

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
File content/browser/download/download_task_runner.cc (right):

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
content/browser/download/download_task_runner.cc:15:
(base::TaskTraits(base::MayBlock(), base::TaskPriority::BACKGROUND)));
On 2017/06/22 16:03:59, gab (OOO Friday) wrote:
> Can remove outer set of () here I think.

Acknowledged.

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
File content/browser/download/download_task_runner.h (right):

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
content/browser/download/download_task_runner.h:14: CONTENT_EXPORT
scoped_refptr<base::SequencedTaskRunner> GetDownloadTaskRunner();
On 2017/06/22 16:03:59, gab (OOO Friday) wrote:
> None of the callers require a refptr (some are even forced to .get()), I'm
going
> to update the LazyTaskRunner API to return a raw ptr by default instead to
avoid
> unnecessary ref churn. In the meantime if you're so inclined this could return
a
> SequencedTaskRunner* and the impl can .Get().get() with a TODO assigned to me
+
> issue 730170.

Acknowledged.

https://codereview.chromium.org/2890853002/diff/330001/content/browser/downlo...
content/browser/download/download_task_runner.h:14: CONTENT_EXPORT
scoped_refptr<base::SequencedTaskRunner> GetDownloadTaskRunner();
On 2017/06/22 16:10:18, gab (OOO Friday) wrote:
> On 2017/06/22 16:03:59, gab wrote:
> > None of the callers require a refptr (some are even forced to .get()), I'm
> going
> > to update the LazyTaskRunner API to return a raw ptr by default instead to
> avoid
> > unnecessary ref churn. In the meantime if you're so inclined this could
return
> a
> > SequencedTaskRunner* and the impl can .Get().get() with a TODO assigned to
me
> +
> > issue 730170.
> 
> Nvm, looks like you do need the refptr in one place for CreateByteStream().

Acknowledged.

[gab, 2017-06-22 18:55:04]

lgtm once tests are happy, I suppose we may unfortunately need to introduce
SetDownloadTaskRunnerForTesting() in a few fixtures to be able to force the main
thread as the download task runner in tests that deeply baked in the old fact
that unit testing of the FILE thread happened on the main message loop.

[Sigurður Ásgeirsson, 2017-06-22 19:25:18]

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

[commit-bot: I haz the power, 2017-06-22 19:25:38]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-06-22 19:42:22]

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

[commit-bot: I haz the power, 2017-06-22 19:42:41]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-06-22 19:48:26]

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

[commit-bot: I haz the power, 2017-06-22 19:48:43]

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-22 20:45:28]

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

[commit-bot: I haz the power, 2017-06-22 20:45:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sigurður Ásgeirsson, 2017-06-27 00:12:34]

Looks like I came up one test short :(. Checking out for three weeks now
for realz. Realz!

On Thu, Jun 22, 2017, 18:55 <gab@chromium.org> wrote:

> lgtm once tests are happy, I suppose we may unfortunately need to introduce
> SetDownloadTaskRunnerForTesting() in a few fixtures to be able to force
> the main
> thread as the download task runner in tests that deeply baked in the old
> fact
> that unit testing of the FILE thread happened on the main message loop.
>
> https://codereview.chromium.org/2890853002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[qinmin, 2017-07-12 21:38:13]

On 2017/06/27 00:12:34, Sigurður Ásgeirsson wrote:
> Looks like I came up one test short :(. Checking out for three weeks now
> for realz. Realz!
> 
> On Thu, Jun 22, 2017, 18:55 <mailto:gab@chromium.org> wrote:
> 
> > lgtm once tests are happy, I suppose we may unfortunately need to introduce
> > SetDownloadTaskRunnerForTesting() in a few fixtures to be able to force
> > the main
> > thread as the download task runner in tests that deeply baked in the old
> > fact
> > that unit testing of the FILE thread happened on the main message loop.
> >
> > https://codereview.chromium.org/2890853002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

siggi@, you can also add download_prefs.cc and
chrome/browser/download/download_browsertest.cc to this CL as both of them
depends on the new task runner introduced here.

[Sigurður Ásgeirsson, 2017-07-17 13:28:46]

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

[commit-bot: I haz the power, 2017-07-17 13:28: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, 2017-07-17 14:36:46]

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

[commit-bot: I haz the power, 2017-07-17 14:36:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sigurður Ásgeirsson, 2017-07-17 19:32:44]

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

[commit-bot: I haz the power, 2017-07-17 19:32:54]

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-07-17 19:43:57]

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

[commit-bot: I haz the power, 2017-07-17 19:43:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sigurður Ásgeirsson, 2017-07-17 19:53:12]

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

[commit-bot: I haz the power, 2017-07-17 19:53:25]

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-07-17 22:12:53]

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

[commit-bot: I haz the power, 2017-07-17 22:12:55]

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_...)

[Sigurður Ásgeirsson, 2017-07-18 20:02:24]

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

[commit-bot: I haz the power, 2017-07-18 20:02:51]

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-07-18 20:08:07]

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

[commit-bot: I haz the power, 2017-07-18 20:08:09]

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...)

[Sigurður Ásgeirsson, 2017-07-19 13:16:43]

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

[commit-bot: I haz the power, 2017-07-19 13:16: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, 2017-07-19 13:37:11]

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

[commit-bot: I haz the power, 2017-07-19 13:37:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sigurður Ásgeirsson, 2017-07-19 14:24:29]

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

[commit-bot: I haz the power, 2017-07-19 14:24:49]

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-07-19 16:27:11]

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

[commit-bot: I haz the power, 2017-07-19 16:27:12]

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

[Sigurður Ásgeirsson, 2017-07-19 17:25:37]

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

[commit-bot: I haz the power, 2017-07-19 17:25:55]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-07-19 18:04:52]

siggi@chromium.org changed reviewers:
+ boliu@chromium.org, jochen@chromium.org, qinmin@chromium.org

[Sigurður Ásgeirsson, 2017-07-19 18:11:04]

Hey guys,

I think this CL is now complete, please take a look. The idea is to switch
BrowserThread::FILE for GetDownloadTaskRunner all round. Where possible/easy
I've bound the leaf classes to a single sequence, rather than a particular
sequence, and thread assertions have been replaced with blocking-allowed
assertions in a couple of places.

Min, Jochen for OWNERS please, and Bo, you were suggested for Android. This can
do with a bit of a critical look, I figure.

Siggi

[Sigurður Ásgeirsson, 2017-07-19 20:19:46]

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

[commit-bot: I haz the power, 2017-07-19 20:19: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, 2017-07-19 21:59:11]

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

[commit-bot: I haz the power, 2017-07-19 21:59:12]

Dry run: This issue passed the CQ dry run.

[Sigurður Ásgeirsson, 2017-07-20 12:36:34]

Gab, I'm not loving the ScopedCOMInitialization as-is, and knowing Chrome, there
may be other cases that aren't exercised by tests where this. The only reason I
noticed these is because in release browser_tests we have a patch that asserts
on CoCreate in threads that haven't CoInitialized.
WDTY - is it better to shunt all of downloads into a dedicated single-threaded
COM STA sequence? It's going to be safer for sure :/.

https://codereview.chromium.org/2890853002/diff/590001/content/common/quarant...
File content/common/quarantine/quarantine_win.cc (right):

https://codereview.chromium.org/2890853002/diff/590001/content/common/quarant...
content/common/quarantine/quarantine_win.cc:306: // TODO(siggi): As this is a
rare operation, this seems better than putting
Gab: there are two places where downloading needs COM on windows, the other one
is in BaseFile::MoveFileAndAdjustPermissions. I think ideally these would be
invoked on a common/global COM STA thread, but I feel that's outside the scope
of this change. WDYT?

[qinmin, 2017-07-20 16:30:16]

download/ lgtm

[gab, 2017-07-20 16:30:42]

gab@chromium.org changed reviewers:
+ robliao@chromium.org

[gab, 2017-07-20 16:33:36]

On 2017/07/20 12:36:34, Sigurður Ásgeirsson wrote:
> Gab, I'm not loving the ScopedCOMInitialization as-is, and knowing Chrome,
there
> may be other cases that aren't exercised by tests where this. The only reason
I
> noticed these is because in release browser_tests we have a patch that asserts
> on CoCreate in threads that haven't CoInitialized.
> WDTY - is it better to shunt all of downloads into a dedicated single-threaded
> COM STA sequence? It's going to be safer for sure :/.
> 
>
https://codereview.chromium.org/2890853002/diff/590001/content/common/quarant...
> File content/common/quarantine/quarantine_win.cc (right):
> 
>
https://codereview.chromium.org/2890853002/diff/590001/content/common/quarant...
> content/common/quarantine/quarantine_win.cc:306: // TODO(siggi): As this is a
> rare operation, this seems better than putting
> Gab: there are two places where downloading needs COM on windows, the other
one
> is in BaseFile::MoveFileAndAdjustPermissions. I think ideally these would be
> invoked on a common/global COM STA thread, but I feel that's outside the scope
> of this change. WDYT?

+robliao for his opinion on COM. I think we can go with everything on a
COMStaTaskRunner on Windows and SequencedTaskRunner on other platforms. I don't
assume the download manager is particularly critical to general performance
(when there aren't downloads) and mixing it with other USER_VISIBLE+MayBlock()
COM work on Windows doesn't sound like it will create havoc (it was already
previously mixed with much more on FILE thread).

(looking at rest of CL now, w00T for passing tests!)

[gab, 2017-07-20 16:54:31]

lgtm w/ nits % COM discussion with robliao

This is awesome! Thanks a lot!

https://codereview.chromium.org/2890853002/diff/590001/chrome/browser/downloa...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/2890853002/diff/590001/chrome/browser/downloa...
chrome/browser/download/download_browsertest.cc:920:
content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
Can remove this line too as RunAllBlockingPoolTasksUntilIdle() does both (I know
it's poorly named, we  have TODO to rename it but will wait until after
migration to avoid causing merge conflicts for everyone working on migration..)

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
File content/browser/download/base_file.cc (right):

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
content/browser/download/base_file.cc:21: #include
"content/public/browser/browser_thread.h"
We can remove browser_thread.h in many of these I assume (i.e. all of those that
don't still tie into BrowserThread::UI or IO).

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
File content/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
content/browser/download/download_browsertest.cc:1498: // factory, which is a
file thread object.
rm mention to "file thread"

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
content/browser/download/download_browsertest.cc:1540: // factory, which is a
file thread object.
ditto

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
File content/browser/download/download_item_impl_unittest.cc (left):

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
content/browser/download/download_item_impl_unittest.cc:23: #include
"base/run_loop.h"
Maybe can get rid of run_loop.h, message_loop.h and browser_thread.h?

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
File content/browser/download/download_item_impl_unittest.cc (right):

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
content/browser/download/download_item_impl_unittest.cc:419:
std::unique_ptr<TestBrowserContext> browser_context_;
Don't think you need the unique_ptr and explicit construction change anymore now
that we got rid of need for explicit destruction order in a precursor CL.

[Sigurður Ásgeirsson, 2017-07-20 17:22:40]

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

[commit-bot: I haz the power, 2017-07-20 17:22:52]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-07-20 17:23:21]

Thanks, comments addressed - Rob, what sayeth thou?

https://codereview.chromium.org/2890853002/diff/590001/chrome/browser/downloa...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/2890853002/diff/590001/chrome/browser/downloa...
chrome/browser/download/download_browsertest.cc:920:
content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
On 2017/07/20 16:54:31, gab wrote:
> Can remove this line too as RunAllBlockingPoolTasksUntilIdle() does both (I
know
> it's poorly named, we  have TODO to rename it but will wait until after
> migration to avoid causing merge conflicts for everyone working on
migration..)

Done.

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
File content/browser/download/base_file.cc (right):

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
content/browser/download/base_file.cc:21: #include
"content/public/browser/browser_thread.h"
On 2017/07/20 16:54:31, gab wrote:
> We can remove browser_thread.h in many of these I assume (i.e. all of those
that
> don't still tie into BrowserThread::UI or IO).

Done for those where the shoe fits - not that many, alas.

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
File content/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
content/browser/download/download_browsertest.cc:1498: // factory, which is a
file thread object.
On 2017/07/20 16:54:31, gab wrote:
> rm mention to "file thread"

Done.

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
content/browser/download/download_browsertest.cc:1540: // factory, which is a
file thread object.
On 2017/07/20 16:54:31, gab wrote:
> ditto

Done.

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
File content/browser/download/download_item_impl_unittest.cc (left):

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
content/browser/download/download_item_impl_unittest.cc:23: #include
"base/run_loop.h"
On 2017/07/20 16:54:31, gab wrote:
> Maybe can get rid of run_loop.h, message_loop.h and browser_thread.h?

BrowserThread::UI still makes an appearance.

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
File content/browser/download/download_item_impl_unittest.cc (right):

https://codereview.chromium.org/2890853002/diff/590001/content/browser/downlo...
content/browser/download/download_item_impl_unittest.cc:419:
std::unique_ptr<TestBrowserContext> browser_context_;
On 2017/07/20 16:54:31, gab wrote:
> Don't think you need the unique_ptr and explicit construction change anymore
now
> that we got rid of need for explicit destruction order in a precursor CL.

Done.

[robliao, 2017-07-20 18:29:17]

https://codereview.chromium.org/2890853002/diff/590001/content/common/quarant...
File content/common/quarantine/quarantine_win.cc (right):

https://codereview.chromium.org/2890853002/diff/590001/content/common/quarant...
content/common/quarantine/quarantine_win.cc:306: // TODO(siggi): As this is a
rare operation, this seems better than putting
On 2017/07/20 12:36:34, Sigurður Ásgeirsson wrote:
> Gab: there are two places where downloading needs COM on windows, the other
one
> is in BaseFile::MoveFileAndAdjustPermissions. I think ideally these would be
> invoked on a common/global COM STA thread, but I feel that's outside the scope
> of this change. WDYT?

I was actually thinking about making ScopedCOMInitializer DCHECK for non COM STA
Task Runners. 

Is it difficult to move this call to a COM STA Task Runner?

[boliu, 2017-07-20 18:47:35]

minor comments inline

Pretty easy to check there are no thread locals. But I think the potential risk
is if this misses anything else that runs on FILE thread and needs to be
sequenced with downloads. I patched this in, and ran:

git grep -l "content/public/browser/download" | xargs grep "\<FILE\>"

and looking at the non-comment files. Both
chrome/browser/extensions/webstore_installer.cc and
chrome/browser/safe_browsing/download_protection_service.cc are fine. I don't
know if there's a better way, and I doubt anyone at this point has everything in
their head, so random grepping is probably best thing possible..

But command also shows a whole bunch of comments that still refer to FILE and
should be updated :)

https://codereview.chromium.org/2890853002/diff/610001/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2890853002/diff/610001/content/browser/downlo...
content/browser/download/download_item_impl.cc:414: PostTaskAndReplyWithResult(
nit: I didn't see a "using base::PostTaskAndReplyWithResult" in this file, so
did some header file inadvertently have that? that's against the style guide I
think

https://codereview.chromium.org/2890853002/diff/610001/content/browser/downlo...
File content/browser/download/download_task_runner.h (right):

https://codereview.chromium.org/2890853002/diff/610001/content/browser/downlo...
content/browser/download/download_task_runner.h:1: // Copyright (c) 2017 The
Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/2890853002/diff/610001/content/browser/downlo...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/2890853002/diff/610001/content/browser/downlo...
content/browser/download/save_package.cc:1287:
base::ThreadRestrictions::AssertIOAllowed();
I suppose should replace these with both this and
DCHECK(DownloadRunner()->RunsInCurrentSequence())?

[commit-bot: I haz the power, 2017-07-20 18:52:14]

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

[commit-bot: I haz the power, 2017-07-20 18:52:16]

Dry run: This issue passed the CQ dry run.

[gab, 2017-07-20 19:07:47]

@boliu: right I also worry about forgetting something that depends on downloads
(and in fact I have this worry in general for the whole migration). I hope this
is mitigated by having everything get off the FILE threads at once with the
ongoing mass migration such that no caller is left unsurveyed (the two
chrome/browser changes in this CL were brought to our attention by another
migration owner which correctly said they had to go hand in hand with rest of
download sequence).

Hopefully TSAN catches anything we miss..

https://codereview.chromium.org/2890853002/diff/610001/content/browser/downlo...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/2890853002/diff/610001/content/browser/downlo...
content/browser/download/save_package.cc:1287:
base::ThreadRestrictions::AssertIOAllowed();
On 2017/07/20 18:47:35, boliu wrote:
> I suppose should replace these with both this and
> DCHECK(DownloadRunner()->RunsInCurrentSequence())?

I recommended going this way because 

1) They have no sequencing requirements per being static.

2) these are unit tested directly and having them
DCHECK(DownloadRunner()->RunsInCurrentSequence()) requires that the unit test
hops to test its code which is unfortunate when there isn't a real sequencing
requirement.

[boliu, 2017-07-20 19:21:54]

sure. although not sure if there are tsan bots on all platforms (thinking of
android in particular..). fingers crossed I guess

[Sigurður Ásgeirsson, 2017-07-20 19:39:48]

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

[commit-bot: I haz the power, 2017-07-20 19:40:10]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-07-20 19:45:53]

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

[commit-bot: I haz the power, 2017-07-20 19:46:14]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-07-20 19:48:16]

Bo, I only found one mention of FILE in content/public/browser/*downloads* and I
fixed that, is there other concrete action you're asking for here?

Rob, I'm not going to spin those stragglers to other threads, at least not in
this CL. Shall I leave as-is, or drop all downloads on the COM STA?
I'm easy either way.

https://codereview.chromium.org/2890853002/diff/590001/content/common/quarant...
File content/common/quarantine/quarantine_win.cc (right):

https://codereview.chromium.org/2890853002/diff/590001/content/common/quarant...
content/common/quarantine/quarantine_win.cc:306: // TODO(siggi): As this is a
rare operation, this seems better than putting
On 2017/07/20 18:29:17, robliao wrote:
> On 2017/07/20 12:36:34, Sigurður Ásgeirsson wrote:
> > Gab: there are two places where downloading needs COM on windows, the other
> one
> > is in BaseFile::MoveFileAndAdjustPermissions. I think ideally these would be
> > invoked on a common/global COM STA thread, but I feel that's outside the
scope
> > of this change. WDYT?
> 
> I was actually thinking about making ScopedCOMInitializer DCHECK for non COM
STA
> Task Runners. 
> 
> Is it difficult to move this call to a COM STA Task Runner?

I'm not sure, but I'm pretty sure it's out of scope for this CL, which is
already a 6 week drag-on :). I'm more worried that there are untested cases in
downloads that stray into COM or the shell, that I haven't found.

The safe thing to do is to drop downloads on the COM STA sequence on Windows?

https://codereview.chromium.org/2890853002/diff/610001/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2890853002/diff/610001/content/browser/downlo...
content/browser/download/download_item_impl.cc:414: PostTaskAndReplyWithResult(
On 2017/07/20 18:47:35, boliu wrote:
> nit: I didn't see a "using base::PostTaskAndReplyWithResult" in this file, so
> did some header file inadvertently have that? that's against the style guide I
> think

Yeah - that's weird. I've no idea where the declaration is coming from - I
didn't find anything in code search. Fixed here and elsewhere where somehow I
got away with this :/.

https://codereview.chromium.org/2890853002/diff/610001/content/browser/downlo...
File content/browser/download/download_task_runner.h (right):

https://codereview.chromium.org/2890853002/diff/610001/content/browser/downlo...
content/browser/download/download_task_runner.h:1: // Copyright (c) 2017 The
Chromium Authors. All rights reserved.
On 2017/07/20 18:47:35, boliu wrote:
> no (c)

Done.

[robliao, 2017-07-20 20:15:53]

https://codereview.chromium.org/2890853002/diff/590001/content/common/quarant...
File content/common/quarantine/quarantine_win.cc (right):

https://codereview.chromium.org/2890853002/diff/590001/content/common/quarant...
content/common/quarantine/quarantine_win.cc:306: // TODO(siggi): As this is a
rare operation, this seems better than putting
On 2017/07/20 19:48:15, Sigurður Ásgeirsson wrote:
> On 2017/07/20 18:29:17, robliao wrote:
> > On 2017/07/20 12:36:34, Sigurður Ásgeirsson wrote:
> > > Gab: there are two places where downloading needs COM on windows, the
other
> > one
> > > is in BaseFile::MoveFileAndAdjustPermissions. I think ideally these would
be
> > > invoked on a common/global COM STA thread, but I feel that's outside the
> scope
> > > of this change. WDYT?
> > 
> > I was actually thinking about making ScopedCOMInitializer DCHECK for non COM
> STA
> > Task Runners. 
> > 
> > Is it difficult to move this call to a COM STA Task Runner?
> 
> I'm not sure, but I'm pretty sure it's out of scope for this CL, which is
> already a 6 week drag-on :). I'm more worried that there are untested cases in
> downloads that stray into COM or the shell, that I haven't found.
> 
> The safe thing to do is to drop downloads on the COM STA sequence on Windows?

Dropping all of downloads in a COM STA sounds fine to me.

[boliu, 2017-07-20 20:29:07]

On 2017/07/20 19:48:16, Sigurður Ásgeirsson wrote:
> Bo, I only found one mention of FILE in content/public/browser/*downloads* and
I
> fixed that, is there other concrete action you're asking for here?

Oh, my command showed most comments are in content/browser/download. Can just do
a grep "<FILE>" for files in that directory

[commit-bot: I haz the power, 2017-07-20 21:11:43]

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

[commit-bot: I haz the power, 2017-07-20 21:11:45]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2017-07-20 22:00:17]

On 2017/07/20 20:29:07, boliu wrote:
> On 2017/07/20 19:48:16, Sigurður Ásgeirsson wrote:
> > Bo, I only found one mention of FILE in content/public/browser/*downloads*
and
> I
> > fixed that, is there other concrete action you're asking for here?
> 
> Oh, my command showed most comments are in content/browser/download. Can just
do
> a grep "<FILE>" for files in that directory

Hey is it possible to also migrate download_api_browsertest.cc over to the
download task runner?  It has a call to
"content::RunAllPendingInMessageLoop(BrowserThread::FILE);" that it looks like
this will break/make possible to fix? :)

[David Trainor- moved to gerrit, 2017-07-20 23:41:05]

On 2017/07/20 22:00:17, David Trainor-ping after 12h wrote:
> On 2017/07/20 20:29:07, boliu wrote:
> > On 2017/07/20 19:48:16, Sigurður Ásgeirsson wrote:
> > > Bo, I only found one mention of FILE in content/public/browser/*downloads*
> and
> > I
> > > fixed that, is there other concrete action you're asking for here?
> > 
> > Oh, my command showed most comments are in content/browser/download. Can
just
> do
> > a grep "<FILE>" for files in that directory
> 
> Hey is it possible to also migrate download_api_browsertest.cc over to the
> download task runner?  It has a call to
> "content::RunAllPendingInMessageLoop(BrowserThread::FILE);" that it looks like
> this will break/make possible to fix? :)

Ah maybe this won't break it as all the tests passed!

[chromium-reviews, 2017-07-21 04:43:02]

Le jeu. 20 juil. 2017 19:41, <dtrainor@chromium.org> a écrit :

> On 2017/07/20 22:00:17, David Trainor-ping after 12h wrote:
> > On 2017/07/20 20:29:07, boliu wrote:
> > > On 2017/07/20 19:48:16, Sigurður Ásgeirsson wrote:
> > > > Bo, I only found one mention of FILE in
> content/public/browser/*downloads*
> > and
> > > I
> > > > fixed that, is there other concrete action you're asking for here?
> > >
> > > Oh, my command showed most comments are in content/browser/download.
> Can
> just
> > do
> > > a grep "<FILE>" for files in that directory
> >
> > Hey is it possible to also migrate download_api_browsertest.cc over to
> the
> > download task runner? It has a call to
> > "content::RunAllPendingInMessageLoop(BrowserThread::FILE);" that it
> looks like
> > this will break/make possible to fix? :)
>
> Ah maybe this won't break it as all the tests passed!
>

I agree that the call should either be migrated or removed if it's
considered useless (but safe thing to do is to migrate it to
RunAllBlockingPoolTasksUntilIdle() for now just to be safe as this CL is
pretty massive and it would suck to have it reverted for making a test
flaky..


> https://codereview.chromium.org/2890853002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sigurður Ásgeirsson, 2017-07-21 13:29:06]

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

[commit-bot: I haz the power, 2017-07-21 13:29:11]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-07-21 13:35:42]

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

[commit-bot: I haz the power, 2017-07-21 13:35:51]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-07-21 13:37:02]

K, I've updated all FILE-mentioning comments I could find in /downloads, added
the downloads_api_browsertest to the lot, and moved to the COM STA.
Good to go yet?

[boliu, 2017-07-21 15:01:02]

On 2017/07/21 13:37:02, Sigurður Ásgeirsson wrote:
> K, I've updated all FILE-mentioning comments I could find in /downloads, added
> the downloads_api_browsertest to the lot, and moved to the COM STA.
> Good to go yet?

lgtm, thanks!

[commit-bot: I haz the power, 2017-07-21 15:58:04]

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

[commit-bot: I haz the power, 2017-07-21 15:58:07]

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

[Sigurður Ásgeirsson, 2017-07-21 16:43:24]

The CQ bit was checked by siggi@chromium.org

[Sigurður Ásgeirsson, 2017-07-21 16:43:25]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/2890853002/#ps690001
(title: "Revert ScopedCOMInitializer invocations.")

[commit-bot: I haz the power, 2017-07-21 16:43:43]

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-07-21 16:52:57]

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

[commit-bot: I haz the power, 2017-07-21 16:52:59]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Sigurður Ásgeirsson, 2017-07-21 17:07:59]

siggi@chromium.org changed reviewers:
+ avi@chromium.org

[Sigurður Ásgeirsson, 2017-07-21 17:08:01]

Avi, can you give this one a quick OWNERS once-over?

[Avi (use Gerrit), 2017-07-21 17:30:09]

This LGTM mod a few comment nits here and there.

https://codereview.chromium.org/2890853002/diff/690001/content/browser/downlo...
File content/browser/download/mhtml_generation_browsertest.cc (right):

https://codereview.chromium.org/2890853002/diff/690001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:323: // - From (X) post
TaskY to downlod sequence.  Because this posting is
typo: download

https://codereview.chromium.org/2890853002/diff/690001/content/browser/downlo...
File content/browser/download/save_file.h (right):

https://codereview.chromium.org/2890853002/diff/690001/content/browser/downlo...
content/browser/download/save_file.h:24: // duration that  the saving job is 'in
progress': once the saving job has been
fix double-space between "that" and "the"

https://codereview.chromium.org/2890853002/diff/690001/content/browser/downlo...
content/browser/download/save_file.h:25: // completed or  canceled, the SaveFile
is destroyed. One SaveFile object
fix double-space bedtween "or" and "canceled"

https://codereview.chromium.org/2890853002/diff/690001/content/browser/downlo...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/2890853002/diff/690001/content/browser/downlo...
content/browser/download/save_package.cc:607: // sequence to delete all saved 
files.
may as well fix the double space between "saved" and "files" while you're here.

[Sigurður Ásgeirsson, 2017-07-21 17:38:20]

Thanks!

https://codereview.chromium.org/2890853002/diff/690001/content/browser/downlo...
File content/browser/download/mhtml_generation_browsertest.cc (right):

https://codereview.chromium.org/2890853002/diff/690001/content/browser/downlo...
content/browser/download/mhtml_generation_browsertest.cc:323: // - From (X) post
TaskY to downlod sequence.  Because this posting is
On 2017/07/21 17:30:08, Avi (ping after 24h) wrote:
> typo: download

Done.

https://codereview.chromium.org/2890853002/diff/690001/content/browser/downlo...
File content/browser/download/save_file.h (right):

https://codereview.chromium.org/2890853002/diff/690001/content/browser/downlo...
content/browser/download/save_file.h:24: // duration that  the saving job is 'in
progress': once the saving job has been
On 2017/07/21 17:30:08, Avi (ping after 24h) wrote:
> fix double-space between "that" and "the"

Done.

https://codereview.chromium.org/2890853002/diff/690001/content/browser/downlo...
content/browser/download/save_file.h:25: // completed or  canceled, the SaveFile
is destroyed. One SaveFile object
On 2017/07/21 17:30:08, Avi (ping after 24h) wrote:
> fix double-space bedtween "or" and "canceled"

Done.

https://codereview.chromium.org/2890853002/diff/690001/content/browser/downlo...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/2890853002/diff/690001/content/browser/downlo...
content/browser/download/save_package.cc:607: // sequence to delete all saved 
files.
On 2017/07/21 17:30:09, Avi (ping after 24h) wrote:
> may as well fix the double space between "saved" and "files" while you're
here.

Done.

[Sigurður Ásgeirsson, 2017-07-21 17:38:29]

The CQ bit was checked by siggi@chromium.org

[Sigurður Ásgeirsson, 2017-07-21 17:38:30]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
boliu@chromium.org, qinmin@chromium.org, gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2890853002/#ps710001
(title: "Address Avi's comments.")

[commit-bot: I haz the power, 2017-07-21 17:38:44]

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-07-21 17:51:52]

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

[commit-bot: I haz the power, 2017-07-21 17:51:55]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Sigurður Ásgeirsson, 2017-07-21 17:56:44]

Description was changed from

==========
Downloads: replace BrowserThread::FILE with task scheduler.

BUG=689520
==========

to

==========
Downloads: replace BrowserThread::FILE with task scheduler.

TBR=benjhayden@chromium.org
BUG=689520
==========

[Sigurður Ásgeirsson, 2017-07-21 17:56:56]

The CQ bit was checked by siggi@chromium.org

[commit-bot: I haz the power, 2017-07-21 17:57:08]

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-07-21 18:58:08]

CQ is committing da patch.

Bot data: {"patchset_id": 710001, "attempt_start_ts": 1500659816041890,
"parent_rev": "756d947a35e25732ad9c58f47683dafbddfe32fd", "commit_rev":
"2b05ad10d4dd7d32f1501f6382f698285dbed01b"}

[commit-bot: I haz the power, 2017-07-21 18:58:25]

Description was changed from

==========
Downloads: replace BrowserThread::FILE with task scheduler.

TBR=benjhayden@chromium.org
BUG=689520
==========

to

==========
Downloads: replace BrowserThread::FILE with task scheduler.

TBR=benjhayden@chromium.org
BUG=689520

Review-Url: https://codereview.chromium.org/2890853002
Cr-Commit-Position: refs/heads/master@{#488725}
Committed:
https://chromium.googlesource.com/chromium/src/+/2b05ad10d4dd7d32f1501f6382f6...
==========

[commit-bot: I haz the power, 2017-07-21 18:58:27]

Committed patchset #36 (id:710001) as
https://chromium.googlesource.com/chromium/src/+/2b05ad10d4dd7d32f1501f6382f6...