Migrate part of the BackgroundFetchJobController to the UI thread

Migrate part of the BackgroundFetchJobController to the UI thread

One of its responsibilities is to dispatch the requests with the
download manager, which must only be used on the UI thread. We need to
meet that requirement. This also hooks up Start() in the context to
ensure layout tests also exercise this code path.

BUG=

Review-Url: https://codereview.chromium.org/2781623009
Cr-Commit-Position: refs/heads/master@{#460730}
Committed: https://chromium.googlesource.com/chromium/src/+/765cc1c8a14a1c6dfdfdb4ffb2e9b68d4766944a

[Peter Beverloo, 2017-03-29 19:35:51]

peter@chromium.org changed reviewers:
+ harkness@chromium.org

[Peter Beverloo, 2017-03-29 19:35:52]

+harkness for review

[harkness, 2017-03-29 22:31:10]

LGTM

https://codereview.chromium.org/2781623009/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2781623009/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:24: class
BackgroundFetchJobController::Core : public DownloadItem::Observer {
I don't have any objections to this approach, but I'm curious why you chose to
have the Core class which lives entirely on the UI thread rather than the
StartRequestOnUI/StartRequestOnIO approach that I see other places.

https://codereview.chromium.org/2781623009/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:40:
base::WeakPtr<Core> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); }
nit: Never called?

https://codereview.chromium.org/2781623009/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:168:
std::move(request_context)));
why not MakeUnique?

[Peter Beverloo, 2017-03-29 22:37:56]

Thanks!

https://codereview.chromium.org/2781623009/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2781623009/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:24: class
BackgroundFetchJobController::Core : public DownloadItem::Observer {
On 2017/03/29 22:31:10, harkness wrote:
> I don't have any objections to this approach, but I'm curious why you chose to
> have the Core class which lives entirely on the UI thread rather than the
> StartRequestOnUI/StartRequestOnIO approach that I see other places.

This is what we do with push. It creates a cleaner separation between the
threads - the methods and data are now clearly separated, and it's always clear
on which thread something should be called.

https://codereview.chromium.org/2781623009/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:40:
base::WeakPtr<Core> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); }
On 2017/03/29 22:31:10, harkness wrote:
> nit: Never called?

BackgroundFetchJobController::ctor calls it

https://codereview.chromium.org/2781623009/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:168:
std::move(request_context)));
On 2017/03/29 22:31:10, harkness wrote:
> why not MakeUnique?

Because our unique_ptr has deletion traits. (Core will be deleted on the UI
thread as opposed to the IO thread.)

[Peter Beverloo, 2017-03-30 11:08:24]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-03-30 11:08:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
harkness@chromium.org
Link to the patchset: https://codereview.chromium.org/2781623009/#ps20001
(title: "Migrate part of the BackgroundFetchJobController to the UI thread")

[commit-bot: I haz the power, 2017-03-30 11:08:47]

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-03-30 12:03:31]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490872104602450,
"parent_rev": "95a71800ad4b14f2a590f470a899da0cde7d5b27", "commit_rev":
"765cc1c8a14a1c6dfdfdb4ffb2e9b68d4766944a"}

[commit-bot: I haz the power, 2017-03-30 12:04:18]

Description was changed from

==========
Migrate part of the BackgroundFetchJobController to the UI thread

One of its responsibilities is to dispatch the requests with the
download manager, which must only be used on the UI thread. We need to
meet that requirement. This also hooks up Start() in the context to
ensure layout tests also exercise this code path.

BUG=
==========

to

==========
Migrate part of the BackgroundFetchJobController to the UI thread

One of its responsibilities is to dispatch the requests with the
download manager, which must only be used on the UI thread. We need to
meet that requirement. This also hooks up Start() in the context to
ensure layout tests also exercise this code path.

BUG=

Review-Url: https://codereview.chromium.org/2781623009
Cr-Commit-Position: refs/heads/master@{#460730}
Committed:
https://chromium.googlesource.com/chromium/src/+/765cc1c8a14a1c6dfdfdb4ffb2e9...
==========

[commit-bot: I haz the power, 2017-03-30 12:04:18]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/765cc1c8a14a1c6dfdfdb4ffb2e9...