Implement the new polling system in the BackgroundFetchJobController

Implement the new polling system in the BackgroundFetchJobController

This CL changes how the BackgroundFetchJobController interacts with the
requests that have to be made. It'll be given a sequence of requests when
starting a fetch -currently set to a single request-, will fetch them
and will then get the next request from the BackgroundFetchDataManager
once completed.

This significantly simplifies the state we have to maintain, since it
only knows about the in-progress requests, deferring knowledge of which
requests are pending to the DataManager. The tests have been updated to
match this.

In effect, Background Fetch requests made by the developer will now
actually (sequentially) fetch the files. We still have to feed them
back to the developer once all fetches have been completed.

BUG=692540
Review-Url: https://codereview.chromium.org/2782553007
Cr-Commit-Position: refs/heads/master@{#460605}
Committed: https://chromium.googlesource.com/chromium/src/+/44302d356149f11edfd1e1708e4c171ee073f436

[Peter Beverloo, 2017-03-28 22:21:21]

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

[Peter Beverloo, 2017-03-28 22:21:21]

+harkness, wdyt?

This needs some more work, but I think it illustrates the direction. Would love
your opinion!

[harkness, 2017-03-29 07:41:28]

Mostly looks good. I particularly like that you were able to simplify a lot of
the tests, and the data manager tests will end up being simpler as well I think.

A few comments inline.

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

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:53:
std::queue<BackgroundFetchRequestInfo> requests_;
Doesn't this mean that we're going to be keeping all the
BackgroundFetchRequestInfos in memory at least at the start of a request? I
thought that was the entire point of the refactor away from JobData.

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:102: const
std::string& request_guid) {
Can you call this the download_guid? I know in the future there will only be
one, but it's still nice to remember that this one is defined by the download
system.

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

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:104: //
TODO(peter): Populate the responses' information in the |request|.
Are you going to do this as part of this review or a future one?

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:121: //
TODO(harkness): Consider how we want to handle cancelled downloads.
Is there a reason you moved the call to MarkRequestAsCompleteAndGetNextRequest
to the COMPLETE only state? I believe we want to call almost all the same logic
in the two cases.

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

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.h:36: bool /*
aborted_by_developer */)>;
Are you rethinking the state for the JobController? Or have you just not
integrated that code with this?

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.h:47: // Starts
fetching the |initial_fetches|. The controller will continue to
At this point, do we gain anything from having the Start() method compared to
just passing the initial requests into the constructor and starting immediately?

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

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:214:
{{"https://example.com/funny_cat.png", "GET"}}));
excellent example.

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:270:
download_manager_->set_post_completion_task_enabled(true);
I like this, it really cleans up the tests.

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_request_info.h (right):

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_request_info.h:36: const
std::string& guid() const { return guid_; }
After this change, is anything still using the old guid?

[Peter Beverloo, 2017-03-29 13:57:18]

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

[commit-bot: I haz the power, 2017-03-29 13:57:32]

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

[Peter Beverloo, 2017-03-29 13:57:35]

PTAL, this is ready for full review now

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

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:53:
std::queue<BackgroundFetchRequestInfo> requests_;
On 2017/03/29 07:41:28, harkness wrote:
> Doesn't this mean that we're going to be keeping all the
> BackgroundFetchRequestInfos in memory at least at the start of a request? I
> thought that was the entire point of the refactor away from JobData.

Well, until we have persistent storage we'll need to do that anyway. In the next
CL this class will even have to keep track of the finished
BackgroundFetchRequestInfos too.

(The whole RegistrationData class will be replaced by persistent storage, it
just exists to accommodate the API of the BackgroundFetchDataManager now.)

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:102: const
std::string& request_guid) {
On 2017/03/29 07:41:28, harkness wrote:
> Can you call this the download_guid? I know in the future there will only be
> one, but it's still nice to remember that this one is defined by the download
> system.

OK. Presumably we may even want to store it on the |request|. That object might
work better as a scoped_refptr.

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

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:104: //
TODO(peter): Populate the responses' information in the |request|.
On 2017/03/29 07:41:28, harkness wrote:
> Are you going to do this as part of this review or a future one?

In the next CL. I think I want two more CLs for e2e after this: (1) store
responses in the DataManager, presumably change the ownership model for
BGFetchRequestInfo a bit, (2) fire all the events.

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:121: //
TODO(harkness): Consider how we want to handle cancelled downloads.
On 2017/03/29 07:41:28, harkness wrote:
> Is there a reason you moved the call to MarkRequestAsCompleteAndGetNextRequest
> to the COMPLETE only state? I believe we want to call almost all the same
logic
> in the two cases.

Yeah, because I don't know what it means. I think in person discussion would
really help there. When does the DownloadManager "cancel" a download, and what
does it mean? Is cancellation final? Does it represent a failure? So many
questions that I hope you've got the right answers for :).

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

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.h:36: bool /*
aborted_by_developer */)>;
On 2017/03/29 07:41:28, harkness wrote:
> Are you rethinking the state for the JobController? Or have you just not
> integrated that code with this?

I'm trying to keep my branch tree flat, so I hadn't rebased this on top of the
state CL yet. There's TODOs for state elsewhere - I just put that CL on the CQ.

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.h:47: // Starts
fetching the |initial_fetches|. The controller will continue to
On 2017/03/29 07:41:28, harkness wrote:
> At this point, do we gain anything from having the Start() method compared to
> just passing the initial requests into the constructor and starting
immediately?

Only when you subscribe to the idea that constructors shouldn't actually do
anything, which I do.

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

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:214:
{{"https://example.com/funny_cat.png", "GET"}}));
On 2017/03/29 07:41:28, harkness wrote:
> excellent example.

Acknowledged.

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:270:
download_manager_->set_post_completion_task_enabled(true);
On 2017/03/29 07:41:28, harkness wrote:
> I like this, it really cleans up the tests.

Acknowledged.

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_request_info.h (right):

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_request_info.h:36: const
std::string& guid() const { return guid_; }
On 2017/03/29 07:41:28, harkness wrote:
> After this change, is anything still using the old guid?

There's bits of unused code around now that I'd like to clean up in the
subsequent CL. Some tests may still depend on it.

[Peter Beverloo, 2017-03-29 14:01:27]

Description was changed from

==========
Implement the new polling system in the BFJobController

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

to

==========
Implement the new polling system in the BackgroundFetchJobController

This CL changes how the BackgroundFetchJobController interacts with the
requests that have to be made. It'll be given a sequence of requests when
starting a fetch -currently set to a single request-, will fetch them
and will then get the next request from the BackgroundFetchDataManager
once completed.

This significantly simplifies the state we have to maintain, since it
only knows about the in-progress requests, deferring knowledge of which
requests are pending to the DataManager. The tests have been updated to
match this.

In effect, Background Fetch requests made by the developer will now
actually (sequentially) fetch the files. We still have to feed them
back to the developer once all fetches have been completed.

BUG=692621, 692544, 692540
==========

[harkness, 2017-03-29 15:08:40]

lgtm

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

https://codereview.chromium.org/2782553007/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:121: //
TODO(harkness): Consider how we want to handle cancelled downloads.
On 2017/03/29 13:57:35, Peter Beverloo wrote:
> On 2017/03/29 07:41:28, harkness wrote:
> > Is there a reason you moved the call to
MarkRequestAsCompleteAndGetNextRequest
> > to the COMPLETE only state? I believe we want to call almost all the same
> logic
> > in the two cases.
> 
> Yeah, because I don't know what it means. I think in person discussion would
> really help there. When does the DownloadManager "cancel" a download, and what
> does it mean? Is cancellation final? Does it represent a failure? So many
> questions that I hope you've got the right answers for :).

My understanding is that CANCELLED is final. Anything non-final would report as
INTERRUPTED. But you can land it like this and we can update it later.

[commit-bot: I haz the power, 2017-03-29 15:29:08]

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

[commit-bot: I haz the power, 2017-03-29 15:29:09]

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

[Peter Beverloo, 2017-03-29 16:01:06]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2017-03-29 16:01:45]

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

[Peter Beverloo, 2017-03-29 17:11:42]

The CQ bit was unchecked by peter@chromium.org

[Peter Beverloo, 2017-03-29 17:50:21]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-03-29 17:50:22]

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/2782553007/#ps40001
(title: "Implement the new polling system in the BFJobController")

[commit-bot: I haz the power, 2017-03-29 17:50:51]

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-29 18:22:17]

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

[commit-bot: I haz the power, 2017-03-29 18:22:18]

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

[Peter Beverloo, 2017-03-29 18:44:05]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2017-03-29 18:44:54]

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-29 19:18:24]

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

[commit-bot: I haz the power, 2017-03-29 19:18:25]

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

[Peter Beverloo, 2017-03-29 19:22:20]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2017-03-29 19:23:34]

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-29 19:57:05]

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

[commit-bot: I haz the power, 2017-03-29 19:57:06]

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

[Peter Beverloo, 2017-03-29 20:47:30]

Description was changed from

==========
Implement the new polling system in the BackgroundFetchJobController

This CL changes how the BackgroundFetchJobController interacts with the
requests that have to be made. It'll be given a sequence of requests when
starting a fetch -currently set to a single request-, will fetch them
and will then get the next request from the BackgroundFetchDataManager
once completed.

This significantly simplifies the state we have to maintain, since it
only knows about the in-progress requests, deferring knowledge of which
requests are pending to the DataManager. The tests have been updated to
match this.

In effect, Background Fetch requests made by the developer will now
actually (sequentially) fetch the files. We still have to feed them
back to the developer once all fetches have been completed.

BUG=692621, 692544, 692540
==========

to

==========
Implement the new polling system in the BackgroundFetchJobController

This CL changes how the BackgroundFetchJobController interacts with the
requests that have to be made. It'll be given a sequence of requests when
starting a fetch -currently set to a single request-, will fetch them
and will then get the next request from the BackgroundFetchDataManager
once completed.

This significantly simplifies the state we have to maintain, since it
only knows about the in-progress requests, deferring knowledge of which
requests are pending to the DataManager. The tests have been updated to
match this.

In effect, Background Fetch requests made by the developer will now
actually (sequentially) fetch the files. We still have to feed them
back to the developer once all fetches have been completed.

BUG=692540
==========

[Peter Beverloo, 2017-03-29 20:47:36]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-03-29 20:47:37]

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/2782553007/#ps60001
(title: "Implement the new polling system in the BFJobController")

[commit-bot: I haz the power, 2017-03-29 20:48:21]

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

[Peter Beverloo, 2017-03-29 21:40:04]

The CQ bit was unchecked by peter@chromium.org

[Peter Beverloo, 2017-03-29 22:27:05]

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

[commit-bot: I haz the power, 2017-03-29 22:27:42]

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

[Peter Beverloo, 2017-03-30 00:20:04]

The CQ bit was unchecked by peter@chromium.org

[Peter Beverloo, 2017-03-30 00:20:09]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-03-30 00:20:09]

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/2782553007/#ps80001
(title: "Implement the new polling system in the BFJobController")

[commit-bot: I haz the power, 2017-03-30 00:20:41]

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 00:53:04]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490833209069090,
"parent_rev": "b93c729b78b6b59a5953215c2fbe6fcb551717e1", "commit_rev":
"44302d356149f11edfd1e1708e4c171ee073f436"}

[commit-bot: I haz the power, 2017-03-30 00:53:49]

Description was changed from

==========
Implement the new polling system in the BackgroundFetchJobController

This CL changes how the BackgroundFetchJobController interacts with the
requests that have to be made. It'll be given a sequence of requests when
starting a fetch -currently set to a single request-, will fetch them
and will then get the next request from the BackgroundFetchDataManager
once completed.

This significantly simplifies the state we have to maintain, since it
only knows about the in-progress requests, deferring knowledge of which
requests are pending to the DataManager. The tests have been updated to
match this.

In effect, Background Fetch requests made by the developer will now
actually (sequentially) fetch the files. We still have to feed them
back to the developer once all fetches have been completed.

BUG=692540
==========

to

==========
Implement the new polling system in the BackgroundFetchJobController

This CL changes how the BackgroundFetchJobController interacts with the
requests that have to be made. It'll be given a sequence of requests when
starting a fetch -currently set to a single request-, will fetch them
and will then get the next request from the BackgroundFetchDataManager
once completed.

This significantly simplifies the state we have to maintain, since it
only knows about the in-progress requests, deferring knowledge of which
requests are pending to the DataManager. The tests have been updated to
match this.

In effect, Background Fetch requests made by the developer will now
actually (sequentially) fetch the files. We still have to feed them
back to the developer once all fetches have been completed.

BUG=692540

Review-Url: https://codereview.chromium.org/2782553007
Cr-Commit-Position: refs/heads/master@{#460605}
Committed:
https://chromium.googlesource.com/chromium/src/+/44302d356149f11edfd1e1708e4c...
==========

[commit-bot: I haz the power, 2017-03-30 00:53:50]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/44302d356149f11edfd1e1708e4c...