Add the JobComplete callback and error/interrupt information

Add the JobComplete callback and error/interrupt information

This adds a way for the JobController to communicate the completion of
a background fetch job. It also expands the state tracking for the
individual download requests so that the system can tell if requests
succeeded or were cancelled.

BUG=692621
Review-Url: https://codereview.chromium.org/2753583002
Cr-Commit-Position: refs/heads/master@{#458000}
Committed: https://chromium.googlesource.com/chromium/src/+/b56e7169125edf091bf4f5b6a4e830218653f880

[harkness, 2017-03-14 18:41:06]

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

[Peter Beverloo, 2017-03-15 16:53:39]

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:73:
job_info.guid()));
We need to clear job_map_ in ShutdownOnIO. This creates a reference cycle
that'll make it impossible to delete BackgroundFetchContext if there are active
jobs, which in turn violates all sorts of assumptions (e.g. availability of the
other Profile-bound objects).

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:81: // caller.
micro nit: same re: the TODO

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.h:63: // the Mojo
response.
micro nit: I'd drop this TODO, it's not really settleable by itself.

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.h:64: void
JobComplete(const std::string& job_guid);
nit: We tend to name callbacks as DidCompleteJob or OnJobCompleted.

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager_unittest.cc:76:
DownloadItem::DownloadState::MAX_DOWNLOAD_STATE);
What does this mean? What sort of state is "MAX_DOWNLOAD"? Do they mean
"Uninitialized" or similar with that?

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.h:31: using
JobCompleteCallback = base::Callback<void()>;
base::OnceClosure. Can also drop the typedef that way.

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.h:37: const
JobCompleteCallback& callback);
nit: drop const& if you can use OnceClosure and std::move() instead

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.h:37: const
JobCompleteCallback& callback);
nit: completed_callback?

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:101:
void JobComplete() { job_complete_called_ = true; }
nit: same re: naming.

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_data.cc:34:
request_info_index_.erase(index_iter);
Are we certain that this will continue to be synchronous in the future?

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_data.cc:48:
DCHECK(next_request.state() ==
DCHECK_EQ

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_data.h:31: bool
UpdateBackgroundFetchRequestState(
A method name like "UpdateFooBar" tells me that calling it will somehow update
FooBar. In this case I think saying that "FooBar *did* update" is more
appropriate. What about DidUpdateFooBar?

https://codereview.chromium.org/2753583002/diff/20001/content/public/test/fak...
File content/public/test/fake_download_item.h (right):

https://codereview.chromium.org/2753583002/diff/20001/content/public/test/fak...
content/public/test/fake_download_item.h:144: DownloadInterruptReason
last_reason_;
This is a scalar, we need to initialize it. |id_| and
|is_file_externally_removed_| and |download_state_| too...

[harkness, 2017-03-16 11:41:10]

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:73:
job_info.guid()));
On 2017/03/15 16:53:38, Peter Beverloo wrote:
> We need to clear job_map_ in ShutdownOnIO. This creates a reference cycle
> that'll make it impossible to delete BackgroundFetchContext if there are
active
> jobs, which in turn violates all sorts of assumptions (e.g. availability of
the
> other Profile-bound objects).

Done.

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:81: // caller.
On 2017/03/15 16:53:38, Peter Beverloo wrote:
> micro nit: same re: the TODO

Done.

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.h:63: // the Mojo
response.
On 2017/03/15 16:53:38, Peter Beverloo wrote:
> micro nit: I'd drop this TODO, it's not really settleable by itself.

Done.

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.h:64: void
JobComplete(const std::string& job_guid);
On 2017/03/15 16:53:38, Peter Beverloo wrote:
> nit: We tend to name callbacks as DidCompleteJob or OnJobCompleted.

Done.

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager_unittest.cc:76:
DownloadItem::DownloadState::MAX_DOWNLOAD_STATE);
On 2017/03/15 16:53:38, Peter Beverloo wrote:
> What does this mean? What sort of state is "MAX_DOWNLOAD"? Do they mean
> "Uninitialized" or similar with that?

I actually meant to send a mail to David to see if we can add a "NOT_STARTED" or
"UNINITIALIZED" state. Right now, the DownloadItem doesn't have any concept of
that. If a DownloadItem has been created at all, it is considerd to be
IN_PROGRESS. So I am using MAX_DOWNLOAD_STATE to represent a request that hasn't
been sent to the download system.

The alternative would be to also have a bool |started| on the RequestInfo on our
side and use the combination of the two to explain the state of the request.

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.h:31: using
JobCompleteCallback = base::Callback<void()>;
On 2017/03/15 16:53:38, Peter Beverloo wrote:
> base::OnceClosure. Can also drop the typedef that way.

I had hesitated on this based on the discussions around whether the events would
be combined, but I can always change it back if needed.

Done.

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.h:37: const
JobCompleteCallback& callback);
On 2017/03/15 16:53:38, Peter Beverloo wrote:
> nit: completed_callback?

Done.

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.h:37: const
JobCompleteCallback& callback);
On 2017/03/15 16:53:38, Peter Beverloo wrote:
> nit: completed_callback?

Done.

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:101:
void JobComplete() { job_complete_called_ = true; }
On 2017/03/15 16:53:38, Peter Beverloo wrote:
> nit: same re: naming.

Done.

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_data.cc:34:
request_info_index_.erase(index_iter);
On 2017/03/15 16:53:38, Peter Beverloo wrote:
> Are we certain that this will continue to be synchronous in the future?

In the future, the JobData might fire off an async update to the DataManager to
persist the state update, but it would be a best effort persist. The in-memory
data structures will always be a snapshot of the actual state, and if Chrome is
restarted, we will get the snapshot from the DownloadManager.

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_data.cc:48:
DCHECK(next_request.state() ==
On 2017/03/15 16:53:38, Peter Beverloo wrote:
> DCHECK_EQ

Done.

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_data.h:31: bool
UpdateBackgroundFetchRequestState(
On 2017/03/15 16:53:38, Peter Beverloo wrote:
> A method name like "UpdateFooBar" tells me that calling it will somehow update
> FooBar. In this case I think saying that "FooBar *did* update" is more
> appropriate. What about DidUpdateFooBar?

This is telling the RequestInfo that the attribute "state" on the DownloadItem
has changed, and that RequestInfo should update its local copy to match the
change. Technically UpdateBackgroundFetchRequestInfoState would fit the naming,
but I think shortening out the "Info" still makes sense.

https://codereview.chromium.org/2753583002/diff/20001/content/public/test/fak...
File content/public/test/fake_download_item.h (right):

https://codereview.chromium.org/2753583002/diff/20001/content/public/test/fak...
content/public/test/fake_download_item.h:144: DownloadInterruptReason
last_reason_;
On 2017/03/15 16:53:38, Peter Beverloo wrote:
> This is a scalar, we need to initialize it. |id_| and
> |is_file_externally_removed_| and |download_state_| too...

Done.

[Peter Beverloo, 2017-03-16 12:37:10]

lgtm

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager_unittest.cc:76:
DownloadItem::DownloadState::MAX_DOWNLOAD_STATE);
On 2017/03/16 11:41:09, harkness wrote:
> On 2017/03/15 16:53:38, Peter Beverloo wrote:
> > What does this mean? What sort of state is "MAX_DOWNLOAD"? Do they mean
> > "Uninitialized" or similar with that?
> 
> I actually meant to send a mail to David to see if we can add a "NOT_STARTED"
or
> "UNINITIALIZED" state. Right now, the DownloadItem doesn't have any concept of
> that. If a DownloadItem has been created at all, it is considerd to be
> IN_PROGRESS. So I am using MAX_DOWNLOAD_STATE to represent a request that
hasn't
> been sent to the download system.
> 
> The alternative would be to also have a bool |started| on the RequestInfo on
our
> side and use the combination of the two to explain the state of the request.

The download item notification also uses this as an uninitialized state value.
This is the sort of thing that I wouldn't send an e-mail about, but instead just
send a CL to add the value and see what they say :).

I won't block you on this since there's precedent, but please do consider
following up.

https://codereview.chromium.org/2753583002/diff/40001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_context.cc (right):

https://codereview.chromium.org/2753583002/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:54:
job_map_.clear();
nit (can be a follow up): since we execute both Shutdown() and the destructor on
the same thread at pretty much the same time, you can just fold the Shutdown()
function into the destructor.

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

https://codereview.chromium.org/2753583002/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:34:
std::move(completed_closure_).Run();
nit: it's safe to delete this for now.

What's happening now is:

  BackgroundFetchContext::ShutdownOnIO()
  --> BackgroundFetchJobController::Shutdown()
  --> BackgroundFetchJobController::~BackgroundFetchJobController()
      --> BackgroundFetchContext::DidCompleteJob()

The latter will try to remove the |job_guid| from the |job_map_| that *is being
deleted* by the call to |job_map_.clear()| in
BackgroundFetchContext::ShutdownOnIO().

I don't know enough about the internals of STL to assess what this means - are
we deleting the object twice? Do we risk hitting the DCHECK in
BackgroundFetchContext::DidCompleteJob()?

So deleting this seems like the wise thing to do.

https://codereview.chromium.org/2753583002/diff/40001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_data.cc (right):

https://codereview.chromium.org/2753583002/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_data.cc:33: (state ==
DownloadItem::DownloadState::CANCELLED)) {
Could we have a switch() w/o a default: state to protect ourselves from
forgetting to update code if a new state is added? :)

https://codereview.chromium.org/2753583002/diff/40001/content/public/test/fak...
File content/public/test/fake_download_item.h (right):

https://codereview.chromium.org/2753583002/diff/40001/content/public/test/fak...
content/public/test/fake_download_item.h:142:
DownloadItem::DownloadState::MAX_DOWNLOAD_STATE;
nit: annotate that this is used as an uninitialized state

[harkness, 2017-03-17 11:14:11]

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

https://codereview.chromium.org/2753583002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager_unittest.cc:76:
DownloadItem::DownloadState::MAX_DOWNLOAD_STATE);
On 2017/03/16 12:37:09, Peter Beverloo wrote:
> On 2017/03/16 11:41:09, harkness wrote:
> > On 2017/03/15 16:53:38, Peter Beverloo wrote:
> > > What does this mean? What sort of state is "MAX_DOWNLOAD"? Do they mean
> > > "Uninitialized" or similar with that?
> > 
> > I actually meant to send a mail to David to see if we can add a
"NOT_STARTED"
> or
> > "UNINITIALIZED" state. Right now, the DownloadItem doesn't have any concept
of
> > that. If a DownloadItem has been created at all, it is considerd to be
> > IN_PROGRESS. So I am using MAX_DOWNLOAD_STATE to represent a request that
> hasn't
> > been sent to the download system.
> > 
> > The alternative would be to also have a bool |started| on the RequestInfo on
> our
> > side and use the combination of the two to explain the state of the request.
> 
> The download item notification also uses this as an uninitialized state value.
> This is the sort of thing that I wouldn't send an e-mail about, but instead
just
> send a CL to add the value and see what they say :).
> 
> I won't block you on this since there's precedent, but please do consider
> following up.

ok, I'll follow up on that.

https://codereview.chromium.org/2753583002/diff/40001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_context.cc (right):

https://codereview.chromium.org/2753583002/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:54:
job_map_.clear();
On 2017/03/16 12:37:09, Peter Beverloo wrote:
> nit (can be a follow up): since we execute both Shutdown() and the destructor
on
> the same thread at pretty much the same time, you can just fold the Shutdown()
> function into the destructor.

Acknowledged.

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

https://codereview.chromium.org/2753583002/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:34:
std::move(completed_closure_).Run();
On 2017/03/16 12:37:09, Peter Beverloo wrote:
> nit: it's safe to delete this for now.
> 
> What's happening now is:
> 
>   BackgroundFetchContext::ShutdownOnIO()
>   --> BackgroundFetchJobController::Shutdown()
>   --> BackgroundFetchJobController::~BackgroundFetchJobController()
>       --> BackgroundFetchContext::DidCompleteJob()
> 
> The latter will try to remove the |job_guid| from the |job_map_| that *is
being
> deleted* by the call to |job_map_.clear()| in
> BackgroundFetchContext::ShutdownOnIO().
> 
> I don't know enough about the internals of STL to assess what this means - are
> we deleting the object twice? Do we risk hitting the DCHECK in
> BackgroundFetchContext::DidCompleteJob()?
> 
> So deleting this seems like the wise thing to do.

Done.

https://codereview.chromium.org/2753583002/diff/40001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_data.cc (right):

https://codereview.chromium.org/2753583002/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_data.cc:33: (state ==
DownloadItem::DownloadState::CANCELLED)) {
On 2017/03/16 12:37:10, Peter Beverloo wrote:
> Could we have a switch() w/o a default: state to protect ourselves from
> forgetting to update code if a new state is added? :)

Done.

https://codereview.chromium.org/2753583002/diff/40001/content/public/test/fak...
File content/public/test/fake_download_item.h (right):

https://codereview.chromium.org/2753583002/diff/40001/content/public/test/fak...
content/public/test/fake_download_item.h:142:
DownloadItem::DownloadState::MAX_DOWNLOAD_STATE;
On 2017/03/16 12:37:10, Peter Beverloo wrote:
> nit: annotate that this is used as an uninitialized state

Done.

[harkness, 2017-03-17 11:14:19]

The CQ bit was checked by harkness@chromium.org

[harkness, 2017-03-17 11:14:20]

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/2753583002/#ps60001
(title: "Code review changes")

[commit-bot: I haz the power, 2017-03-17 11:14: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-03-17 11:20:44]

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

[commit-bot: I haz the power, 2017-03-17 11:20:45]

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

[harkness, 2017-03-17 11:40:28]

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

[harkness, 2017-03-17 11:40:29]

avi@chromium.org: Please review changes in
content/public/test/fake_download_item.*

[Avi (use Gerrit), 2017-03-17 16:38:54]

lgtm

[harkness, 2017-03-19 17:59:28]

The CQ bit was checked by harkness@chromium.org

[harkness, 2017-03-19 17:59:29]

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/2753583002/#ps80001
(title: "added argument name")

[commit-bot: I haz the power, 2017-03-19 17:59: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-19 19:01:29]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489946368847940,
"parent_rev": "5cc894ac2fde8be17931ccd7a7d19f0f89e67050", "commit_rev":
"b56e7169125edf091bf4f5b6a4e830218653f880"}

[commit-bot: I haz the power, 2017-03-19 19:02:13]

Description was changed from

==========
Add the JobComplete callback and error/interrupt information

This adds a way for the JobController to communicate the completion of
a background fetch job. It also expands the state tracking for the
individual download requests so that the system can tell if requests
succeeded or were cancelled.

BUG=692621
==========

to

==========
Add the JobComplete callback and error/interrupt information

This adds a way for the JobController to communicate the completion of
a background fetch job. It also expands the state tracking for the
individual download requests so that the system can tell if requests
succeeded or were cancelled.

BUG=692621

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

[commit-bot: I haz the power, 2017-03-19 19:02:15]

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