[Background Fetch] Tidy up getting/activating pending requests

[Background Fetch] Tidy up getting/activating pending requests

- Stop BackgroundFetchDataManager::CreateRegistration from invoking its
  callback with an initial_requests vector that then has to be passed
  around everywhere; instead have BackgroundFetchJobController directly
  get the initial requests from BackgroundFetchDataManager using a
  PopNextRequest method as it already does for subsequent requests.

- Rename BackgroundFetchDataManager::RegistrationData::GetPendingRequest
  to PopNextPendingRequest since it also removes the request from the
  pending queue.

- Split up the MarkRequestAsCompleteAndGetNextRequest method of
  BackgroundFetchJobController so it re-uses PopNextRequest rather than
  duplicating it.

- Remove BackgroundFetchJobController's brittle tracking of
  pending_completed_file_acknowledgements_ since if/when multiple
  requests are run in parallel, it would incorrectly report the job to
  be complete as soon as the pending requests queue is exhausted, even
  if other requests are still being downloaded. Instead just get an
  accurate value for has_pending_or_active_requests from the
  BackgroundFetchDataManager when marking requests complete.


BUG=none

Review-Url: https://codereview.chromium.org/2978603003
Cr-Commit-Position: refs/heads/master@{#485969}
Committed: https://chromium.googlesource.com/chromium/src/+/dd0e9b921a04328ae16ccba31883c655cfe42456

[johnme, 2017-07-11 15:59:13]

johnme@chromium.org changed reviewers:
+ delphick@chromium.org, peter@chromium.org

[Peter Beverloo, 2017-07-11 16:10:55]

Nice clean-up! Thanks

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

https://codereview.chromium.org/2978603003/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.h:63:
NextRequestCallback callback);
I'm not a fan of "Activate". The JobController "starts" the request, the
DataManager "gets" it from storage -- it doesn't actually activate anything.
WDYT?

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

https://codereview.chromium.org/2978603003/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:270: //
and/or per origin rather than per fetch.
Please file a bug for this - scheduling will be a huge task by itself, since a
holistic solution requires insight in what other users of the download service
are doing as well.

https://codereview.chromium.org/2978603003/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:284: return;
 // No more pending requests to start.
Please document that this bail-out is only applicable for the initial set of
requests.

https://codereview.chromium.org/2978603003/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:306:
data_manager_->MarkRequestAsCompleteAndActivateNextRequest(
What about splitting up MarkRequestAsComplete so that {Activate,Get}NextRequest
can be used from both sources?

[johnme, 2017-07-12 13:12:44]

Description was changed from

==========
[Background Fetch] Tidy up getting/activating pending requests

- Stop BackgroundFetchDataManager::CreateRegistration from invoking its
  callback with an initial_requests vector that then has to be passed
  around everywhere; instead have BackgroundFetchJobController directly
  get the initial requests from BackgroundFetchDataManager as it already
  does for subsequent requests.

- Rename BackgroundFetchDataManager::RegistrationData::GetPendingRequest
  to ActivateNextPendingRequest since it moves the request between lists
  rather than merely getting it.

BUG=none
==========

to

==========
[Background Fetch] Tidy up getting/activating pending requests

- Stop BackgroundFetchDataManager::CreateRegistration from invoking its
  callback with an initial_requests vector that then has to be passed
  around everywhere; instead have BackgroundFetchJobController directly
  get the initial requests from BackgroundFetchDataManager using a
  PopNextRequest method as it already does for subsequent requests.

- Rename BackgroundFetchDataManager::RegistrationData::GetPendingRequest
  to PopNextPendingRequest since it also removes the request from the
  pending queue.

- Split up the MarkRequestAsCompleteAndGetNextRequest method of
  BackgroundFetchJobController so it re-uses PopNextRequest rather than
  duplicating it.

- Remove BackgroundFetchJobController's brittle tracking of
  pending_completed_file_acknowledgements_ since if/when multiple
  requests are run in parallel, it would incorrectly report the job to
  be complete as soon as the pending requests queue is exhausted, even
  if other requests are still being downloaded. Instead just get an
  accurate value for has_pending_or_active_requests from the
  BackgroundFetchDataManager when marking requests complete.


BUG=none
==========

[johnme, 2017-07-12 13:13:10]

Addressed review comments (and expanded CL description accordingly) - thanks!

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

https://codereview.chromium.org/2978603003/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.h:63:
NextRequestCallback callback);
On 2017/07/11 16:10:55, Peter Beverloo wrote:
> I'm not a fan of "Activate". The JobController "starts" the request, the
> DataManager "gets" it from storage -- it doesn't actually activate anything.
> WDYT?

I've renamed this to PopNextRequest, since the key point is that it's no longer
in the pending queue.

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

https://codereview.chromium.org/2978603003/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:270: //
and/or per origin rather than per fetch.
On 2017/07/11 16:10:55, Peter Beverloo wrote:
> Please file a bug for this - scheduling will be a huge task by itself, since a
> holistic solution requires insight in what other users of the download service
> are doing as well.

Done.

https://codereview.chromium.org/2978603003/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:284: return;
 // No more pending requests to start.
On 2017/07/11 16:10:55, Peter Beverloo wrote:
> Please document that this bail-out is only applicable for the initial set of
> requests.

Done.

https://codereview.chromium.org/2978603003/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:306:
data_manager_->MarkRequestAsCompleteAndActivateNextRequest(
On 2017/07/11 16:10:55, Peter Beverloo wrote:
> What about splitting up MarkRequestAsComplete so that
{Activate,Get}NextRequest
> can be used from both sources?

Done (yeah, I'd been considering that; initially I was reluctant because it made
the pending_completed_file_acknowledgements_ logic more complicated, but I've
since realised that the pending_completed_file_acknowledgements_ logic was
brittle and incorrect anyway, so I've gotten rid of it and now just check if any
requests are yet to complete).

[Peter Beverloo, 2017-07-12 13:30:50]

lgtm

https://codereview.chromium.org/2978603003/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_data_manager.h (right):

https://codereview.chromium.org/2978603003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:63: // started,
and invokes the |callback| with that request, else a null request.
nit: update comment ("activate")

https://codereview.chromium.org/2978603003/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2978603003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:270: //
and/or per origin rather than per fetch (crbug.com/741609).
note: TODO(crbug.com/123456) is allowed syntax, and probably more appropriate in
this case

https://codereview.chromium.org/2978603003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:344:
DCHECK_EQ(state_, State::FETCHING);
Hmm. Might we end up with a scheduling solution where we won't immediately start
background fetch jobs, making it possible (and valid!) to call Abort() prior to
Start()?

https://codereview.chromium.org/2978603003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:351:
completed_callback_.Reset();
delete, that's what std::move() will conceptually do. on line 333 too.

[johnme, 2017-07-12 14:13:58]

Addressed review comments - thanks :)

https://codereview.chromium.org/2978603003/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_data_manager.h (right):

https://codereview.chromium.org/2978603003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:63: // started,
and invokes the |callback| with that request, else a null request.
On 2017/07/12 13:30:50, Peter Beverloo wrote:
> nit: update comment ("activate")

Done.

https://codereview.chromium.org/2978603003/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2978603003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:270: //
and/or per origin rather than per fetch (crbug.com/741609).
On 2017/07/12 13:30:50, Peter Beverloo wrote:
> note: TODO(crbug.com/123456) is allowed syntax, and probably more appropriate
in
> this case

Done.

https://codereview.chromium.org/2978603003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:344:
DCHECK_EQ(state_, State::FETCHING);
On 2017/07/12 13:30:50, Peter Beverloo wrote:
> Hmm. Might we end up with a scheduling solution where we won't immediately
start
> background fetch jobs, making it possible (and valid!) to call Abort() prior
to
> Start()?

Good point. Changed this to just early out if state is completed.

https://codereview.chromium.org/2978603003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:351:
completed_callback_.Reset();
On 2017/07/12 13:30:50, Peter Beverloo wrote:
> delete, that's what std::move() will conceptually do. on line 333 too.

Done.

[johnme, 2017-07-12 14:14:03]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-07-12 14:14:04]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2978603003/#ps40001
(title: "Address 2nd round review comments")

[commit-bot: I haz the power, 2017-07-12 14:14:15]

CQ is trying da patch.

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

[Dan Elphick, 2017-07-12 14:18:51]

lgtm

[commit-bot: I haz the power, 2017-07-12 15:39:34]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1499868843693930,
"parent_rev": "3c9b1cb60a0f85032fb67540f0c584e401c72479", "commit_rev":
"609624b419ed2a18ed9b0efed8ed9662644d81ee"}

[commit-bot: I haz the power, 2017-07-12 15:39:46]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1499868843693930,
"parent_rev": "15617f0ba9ba58672ce22b2269017c7b12ec124f", "commit_rev":
"dd0e9b921a04328ae16ccba31883c655cfe42456"}

[commit-bot: I haz the power, 2017-07-12 15:39:56]

Description was changed from

==========
[Background Fetch] Tidy up getting/activating pending requests

- Stop BackgroundFetchDataManager::CreateRegistration from invoking its
  callback with an initial_requests vector that then has to be passed
  around everywhere; instead have BackgroundFetchJobController directly
  get the initial requests from BackgroundFetchDataManager using a
  PopNextRequest method as it already does for subsequent requests.

- Rename BackgroundFetchDataManager::RegistrationData::GetPendingRequest
  to PopNextPendingRequest since it also removes the request from the
  pending queue.

- Split up the MarkRequestAsCompleteAndGetNextRequest method of
  BackgroundFetchJobController so it re-uses PopNextRequest rather than
  duplicating it.

- Remove BackgroundFetchJobController's brittle tracking of
  pending_completed_file_acknowledgements_ since if/when multiple
  requests are run in parallel, it would incorrectly report the job to
  be complete as soon as the pending requests queue is exhausted, even
  if other requests are still being downloaded. Instead just get an
  accurate value for has_pending_or_active_requests from the
  BackgroundFetchDataManager when marking requests complete.


BUG=none
==========

to

==========
[Background Fetch] Tidy up getting/activating pending requests

- Stop BackgroundFetchDataManager::CreateRegistration from invoking its
  callback with an initial_requests vector that then has to be passed
  around everywhere; instead have BackgroundFetchJobController directly
  get the initial requests from BackgroundFetchDataManager using a
  PopNextRequest method as it already does for subsequent requests.

- Rename BackgroundFetchDataManager::RegistrationData::GetPendingRequest
  to PopNextPendingRequest since it also removes the request from the
  pending queue.

- Split up the MarkRequestAsCompleteAndGetNextRequest method of
  BackgroundFetchJobController so it re-uses PopNextRequest rather than
  duplicating it.

- Remove BackgroundFetchJobController's brittle tracking of
  pending_completed_file_acknowledgements_ since if/when multiple
  requests are run in parallel, it would incorrectly report the job to
  be complete as soon as the pending requests queue is exhausted, even
  if other requests are still being downloaded. Instead just get an
  accurate value for has_pending_or_active_requests from the
  BackgroundFetchDataManager when marking requests complete.


BUG=none

Review-Url: https://codereview.chromium.org/2978603003
Cr-Commit-Position: refs/heads/master@{#485969}
Committed:
https://chromium.googlesource.com/chromium/src/+/dd0e9b921a04328ae16ccba31883...
==========

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

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/dd0e9b921a04328ae16ccba31883...