Componentize component_updater: Replace content::BrowserThread usage with task runners

Componentize component_updater: Replace content::BrowserThread usage with task runners

This patch strips out usage of specific threads defined in content/.

- Replaces FILE thread in BackgroundDownloaderWin with a SingleThreadTaskRunner backed by the FILE thread.
- Replaces a bunch of DCHECK(UI thread) with use of base::ThreadChecker (which just checks that it's all on the constructor thread).
- Replaces Posts to the UI thread with a post to the current message loop used in the constructor.

BUG=371463
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284960

[tommycli, 2014-07-14 23:45:54]

blundell, sorin:

PTAL. This patch contains threading changes discussed in our email. See the
description.

Thanks!

[blundell, 2014-07-15 08:45:20]

Thanks, Tommy!

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/background_downloader_win.cc (right):

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/background_downloader_win.cc:28: // while the
BITS specific code runs on a single thread task runner.
I would say something like "a separate thread passed in by the client".

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/background_downloader_win.cc:394:
DCHECK(thread_checker_.CalledOnValidThread());
this DCHECK is meaningless I think as thread_checker_ has just been attached to
this thread in its initialization.

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/background_downloader_win.h (right):

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/background_downloader_win.h:38:
scoped_refptr<base::SingleThreadTaskRunner> task_runner);
This could probably use a more descriptive name, e.g. |file_task_runner|. Maybe
Sorin has a better name.

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/background_downloader_win.h:87:
scoped_refptr<base::MessageLoopProxy> main_loop_;
Is there a reason this has to be a MLP rather than a SingleThreadTaskRunner?
MessageLoopProxy is deprecated at this point.

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/cld_component_installer.cc:112:
content::BrowserThread::GetBlockingPool()->GetSequenceToken(),
What logic is behind the choices on lines 112 and 113 (and elsewhere throughout
this CL)?

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_configurator.h (right):

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.h:101: // Gets a
task runner for worker jobs guaranteed to run on a single thread.
Does there need to be further constraints specified here, e.g., that the task
runner can do IO?

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.h:49: // with all the
unpacked CRX files. This method is called from a task runner.
"from a task runner" is not particularly informative here.

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/crx_downloader.h (right):

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/crx_downloader.h:91: // background downloader
be used, if the platform supports it.
The new params should be documented.

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/crx_downloader.h:95:
scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner,
It would be good if these params could be given names that reflect their
intended usage.

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/default_component_installer.h (right):

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/default_component_installer.h:114:
scoped_refptr<base::MessageLoopProxy> main_loop_;
Same comment re: MessageLoopProxy.

[tommycli, 2014-07-15 18:58:26]

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/background_downloader_win.cc (right):

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/background_downloader_win.cc:28: // while the
BITS specific code runs on a single thread task runner.
On 2014/07/15 08:45:20, blundell wrote:
> I would say something like "a separate thread passed in by the client".

Done.

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/background_downloader_win.cc:394:
DCHECK(thread_checker_.CalledOnValidThread());
On 2014/07/15 08:45:20, blundell wrote:
> this DCHECK is meaningless I think as thread_checker_ has just been attached
to
> this thread in its initialization.

Done.

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/background_downloader_win.h (right):

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/background_downloader_win.h:38:
scoped_refptr<base::SingleThreadTaskRunner> task_runner);
On 2014/07/15 08:45:20, blundell wrote:
> This could probably use a more descriptive name, e.g. |file_task_runner|.
Maybe
> Sorin has a better name.

Done. I tried |bits_task_runner|. I'll let Sorin see.

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/background_downloader_win.h:87:
scoped_refptr<base::MessageLoopProxy> main_loop_;
On 2014/07/15 08:45:20, blundell wrote:
> Is there a reason this has to be a MLP rather than a SingleThreadTaskRunner?
> MessageLoopProxy is deprecated at this point.

Done.

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/cld_component_installer.cc:112:
content::BrowserThread::GetBlockingPool()->GetSequenceToken(),
On 2014/07/15 08:45:20, blundell wrote:
> What logic is behind the choices on lines 112 and 113 (and elsewhere
throughout
> this CL)?

I replicated the choices made here: 

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/com...

In another version of this patch, which I didn't upload, I used:
  ComponentUpdateService::GetConfig()->GetSequencedTaskRunner(), but this
requires adding a GetConfig method to ComponentUpdateService.

Do you prefer that?

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_configurator.h (right):

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.h:101: // Gets a
task runner for worker jobs guaranteed to run on a single thread.
On 2014/07/15 08:45:20, blundell wrote:
> Does there need to be further constraints specified here, e.g., that the task
> runner can do IO?

Done.

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.h:49: // with all the
unpacked CRX files. This method is called from a task runner.
On 2014/07/15 08:45:20, blundell wrote:
> "from a task runner" is not particularly informative here.

Done.

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/crx_downloader.h (right):

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/crx_downloader.h:91: // background downloader
be used, if the platform supports it.
On 2014/07/15 08:45:20, blundell wrote:
> The new params should be documented.

Done.

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/crx_downloader.h:95:
scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner,
On 2014/07/15 08:45:20, blundell wrote:
> It would be good if these params could be given names that reflect their
> intended usage.

Done.

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/default_component_installer.h (right):

https://codereview.chromium.org/385013002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/default_component_installer.h:114:
scoped_refptr<base::MessageLoopProxy> main_loop_;
On 2014/07/15 08:45:20, blundell wrote:
> Same comment re: MessageLoopProxy.

Done.

[blundell, 2014-07-21 14:20:35]

Sorin: friendly ping :)

[Sorin Jianu, 2014-07-21 21:42:14]

lgtm

+cpu

Thank you! I apologize for the delayed turn over.

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/background_downloader_win.cc (right):

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/background_downloader_win.cc:394:
is_completed_(false) {
Keep the assert?

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/background_downloader_win.h (right):

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/background_downloader_win.h:47: // Called
asynchronously on the |task_runner_| at different stages during
This should be |bits_task_runner_|, shouldn't it?

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/background_downloader_win.h:92:
scoped_refptr<base::SingleThreadTaskRunner> bits_task_runner_;
We could keep the original |task_runner_| name so that this code is similar to
the code in the other classes with similar changes.

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/crx_downloader.cc (right):

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/crx_downloader.cc:7: #include
"chrome/browser/component_updater/crx_downloader.h"
crx_downloader.h must be the first header to include.

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/crx_downloader.cc:51: :
successor_(successor.Pass()) {
Maybe keep the thread checker assert.

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/update_checker.cc (right):

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/update_checker.cc:105: : config_(config),
update_check_callback_(update_check_callback) {
Maybe keep the assert?

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/url_fetcher_downloader.cc (right):

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/url_fetcher_downloader.cc:24: total_bytes_(-1)
{
Maybe keep the assert?

[tommycli, 2014-07-21 22:00:08]

sorin: Thanks!

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/background_downloader_win.cc (right):

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/background_downloader_win.cc:394:
is_completed_(false) {
On 2014/07/21 21:42:13, Sorin Jianu wrote:
> Keep the assert?

thread_checker_ is bound to the current thread upon construction, so an
assertion in the constructor would always pass.

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/background_downloader_win.h (right):

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/background_downloader_win.h:47: // Called
asynchronously on the |task_runner_| at different stages during
On 2014/07/21 21:42:13, Sorin Jianu wrote:
> This should be |bits_task_runner_|, shouldn't it?

Kept it as task_runner_.

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/background_downloader_win.h:92:
scoped_refptr<base::SingleThreadTaskRunner> bits_task_runner_;
On 2014/07/21 21:42:13, Sorin Jianu wrote:
> We could keep the original |task_runner_| name so that this code is similar to
> the code in the other classes with similar changes.

Done.

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/crx_downloader.cc (right):

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/crx_downloader.cc:7: #include
"chrome/browser/component_updater/crx_downloader.h"
On 2014/07/21 21:42:14, Sorin Jianu wrote:
> crx_downloader.h must be the first header to include.

Done.

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/crx_downloader.cc:51: :
successor_(successor.Pass()) {
On 2014/07/21 21:42:13, Sorin Jianu wrote:
> Maybe keep the thread checker assert.

Would always pass.

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/update_checker.cc (right):

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/update_checker.cc:105: : config_(config),
update_check_callback_(update_check_callback) {
On 2014/07/21 21:42:14, Sorin Jianu wrote:
> Maybe keep the assert?

Would always pass.

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/url_fetcher_downloader.cc (right):

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/url_fetcher_downloader.cc:24: total_bytes_(-1)
{
On 2014/07/21 21:42:14, Sorin Jianu wrote:
> Maybe keep the assert?

Would always pass.

[Sorin Jianu, 2014-07-21 22:11:32]

Thank you Tommy!

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/background_downloader_win.cc (right):

https://codereview.chromium.org/385013002/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/background_downloader_win.cc:394:
is_completed_(false) {
On 2014/07/21 22:00:07, tommycli wrote:
> On 2014/07/21 21:42:13, Sorin Jianu wrote:
> > Keep the assert?
> 
> thread_checker_ is bound to the current thread upon construction, so an
> assertion in the constructor would always pass.

Of course, I was suggesting to keep it so that it is easier to see that all code
runs on the same thread. I see your point of removing the assert that always
passes. Sound good.

[tommycli, 2014-07-21 22:18:15]

waffles: Ready for your review. Thanks!

[waffles, 2014-07-21 23:41:54]

I'm not sure about the duplication on each call of "new
DefaultComponentInstaller". It would be nice if we could reuse the configurator
for this; Perhaps DefaultComponentInstaller could pick it up from cus_?
Thoughts?

[Sorin Jianu, 2014-07-22 17:53:32]

On 2014/07/21 23:41:54, waffles wrote:
> I'm not sure about the duplication on each call of "new
> DefaultComponentInstaller". It would be nice if we could reuse the
configurator
> for this; Perhaps DefaultComponentInstaller could pick it up from cus_?
> Thoughts?

I did not like that code duplication either, I did not comment on it but thought
that would be something I want to refactor in the future. I am ok if this change
is taken as part of this CL.

[tommycli, 2014-07-22 18:22:26]

waffles/sorin: I added a GetConfig method to CUS to remove the code duplication
on the task runner creation.

[waffles, 2014-07-22 19:24:18]

On 2014/07/22 18:22:26, tommycli wrote:
> waffles/sorin: I added a GetConfig method to CUS to remove the code
duplication
> on the task runner creation.

Thanks Tommy, this is what I had in mind.
However, now that I see it, I think it would be better to expose only the task
runner from cus, not the whole configurator.
Other than that issue things look pretty good.

[tommycli, 2014-07-22 21:14:36]

waffles: No problem, see if this is more to your liking.

[waffles, 2014-07-22 21:21:00]

lgtm % some #includes.

https://codereview.chromium.org/385013002/diff/180001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/385013002/diff/180001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:16: #include
"base/threading/sequenced_worker_pool.h"
Is this still needed?

https://codereview.chromium.org/385013002/diff/180001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/385013002/diff/180001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:25: #include
"base/threading/sequenced_worker_pool.h"
Is this still needed?

https://codereview.chromium.org/385013002/diff/180001/chrome/browser/componen...
File chrome/browser/component_updater/widevine_cdm_component_installer.cc
(right):

https://codereview.chromium.org/385013002/diff/180001/chrome/browser/componen...
chrome/browser/component_updater/widevine_cdm_component_installer.cc:23:
#include "base/threading/sequenced_worker_pool.h"
Is this still needed?

[blundell, 2014-07-22 21:27:52]

lgtm

Thanks, Tommy!

[tommycli, 2014-07-22 21:28:36]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2014-07-22 21:28:43]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2014-07-22 21:47:44]

thanks

https://codereview.chromium.org/385013002/diff/180001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/385013002/diff/180001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:16: #include
"base/threading/sequenced_worker_pool.h"
On 2014/07/22 21:20:59, waffles wrote:
> Is this still needed?

Done.

https://codereview.chromium.org/385013002/diff/180001/chrome/browser/componen...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/385013002/diff/180001/chrome/browser/componen...
chrome/browser/component_updater/sw_reporter_installer_win.cc:25: #include
"base/threading/sequenced_worker_pool.h"
On 2014/07/22 21:21:00, waffles wrote:
> Is this still needed?

Done.

https://codereview.chromium.org/385013002/diff/180001/chrome/browser/componen...
File chrome/browser/component_updater/widevine_cdm_component_installer.cc
(right):

https://codereview.chromium.org/385013002/diff/180001/chrome/browser/componen...
chrome/browser/component_updater/widevine_cdm_component_installer.cc:23:
#include "base/threading/sequenced_worker_pool.h"
On 2014/07/22 21:21:00, waffles wrote:
> Is this still needed?

Done.

[tommycli, 2014-07-22 21:47:50]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-07-22 21:50:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/385013002/200001

[commit-bot: I haz the power, 2014-07-23 02:26:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-23 04:22:47]

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

[commit-bot: I haz the power, 2014-07-23 04:22:49]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[tommycli, 2014-07-23 16:50:54]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-07-23 16:54:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/385013002/200001

[commit-bot: I haz the power, 2014-07-23 16:56:57]

Change committed as 284960