Implement GetJobResponse and merge JobData into DataManager.

Implement GetJobResponse and merge JobData into DataManager.

This CL has two major changes. The first is the addition of the
GetJobResponse call on the DataManager. This Creates a new (and likely
temporary) BackgroundFetchJobResponseData which is owned by the JobInfo
and exists to collect the information needed to construct the Response
that will eventually be sent to Mojo.

The second major change is merging the JobData back into the DataManager.
The JobData was always intended as a cache layer to assist in fetches
involving large numbers of files in a batch, but given the desire to
reduce the memory footprint of Chrome and the relative rarity of these
operations, the cache layer is being removed in favor of storage accesses.

Minor changes:
* More attempts to isolate the eventual storage interface.
* Methods on the JobData which were moved to the DataManager are now virtual
  for testing.
* DataManager takes a BrowserContext* instead of BackgroundFetchContext*.
* Much of the batch management which was in JobData is now handled in
  BackgroundFetchJobInfo.

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

[harkness, 2017-03-23 03:39:14]

Currently very light on testing. I'll try to update with more tests tomorrow.
One open question is how we want to deal with lifetime of the response. Let's
talk about that in person tomorrow.

[harkness, 2017-03-23 09:44:34]

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

[commit-bot: I haz the power, 2017-03-23 09:44:50]

Dry run: 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-23 09:53:43]

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

[commit-bot: I haz the power, 2017-03-23 09:53:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[harkness, 2017-03-23 10:16:59]

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

[commit-bot: I haz the power, 2017-03-23 10:17:09]

Dry run: 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-23 12:09:25]

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

[commit-bot: I haz the power, 2017-03-23 12:09:27]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-03-24 14:43:31]

Description was changed from

==========
Implement GetJobResponse and merge JobData into DataManager.

This CL has two major changes. The first is the addition of the
GetJobResponse call on the DataManager. This Creates a new (and likely
temporary) BackgroundFetchJobResponseData which is owned by the JobInfo
and exists to collect the information needed to construct the Response
that will eventually be sent to Mojo.

The second major change is merging the JobData back into the DataManager.
The JobData was always intended as a cache layer to assist in fetches
involving large numbers of files in a batch, but given the desire to
reduce the memory footprint of Chrome and the relative rarity of these
operations, the cache layer is being removed in favor of storage accesses.

Minor changes:
* More attempts to isolate the eventual storage interface.
* Methods on the JobData which were moved to the DataManager are now virtual
  for testing.
* DataManager takes a BrowserContext* instead of BackgroundFetchContext*.
* Much of the batch management which was in JobData is now handled in
  BackgroundFetchJobInfo.

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

to

==========
Implement GetJobResponse and merge JobData into DataManager.

This CL has two major changes. The first is the addition of the
GetJobResponse call on the DataManager. This Creates a new (and likely
temporary) BackgroundFetchJobResponseData which is owned by the JobInfo
and exists to collect the information needed to construct the Response
that will eventually be sent to Mojo.

The second major change is merging the JobData back into the DataManager.
The JobData was always intended as a cache layer to assist in fetches
involving large numbers of files in a batch, but given the desire to
reduce the memory footprint of Chrome and the relative rarity of these
operations, the cache layer is being removed in favor of storage accesses.

Minor changes:
* More attempts to isolate the eventual storage interface.
* Methods on the JobData which were moved to the DataManager are now virtual
  for testing.
* DataManager takes a BrowserContext* instead of BackgroundFetchContext*.
* Much of the batch management which was in JobData is now handled in
  BackgroundFetchJobInfo.

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

[Peter Beverloo, 2017-03-24 14:43:31]

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

[Peter Beverloo, 2017-03-25 03:38:51]

Hi Jen,

Let me lead with a few higher level comments.

    - Conceptionally, my mental model of the DataManager is that it's
      a friendlier front-end to the actual storage model. It would,
      at most, give the caller a list of files available in storage.

      In response to that, another object, presumably the context,
      would create the blob entries on demand. This can actually be
      synchronous, see my comment in background_fetch_data_manager.cc

      That would create a flow like the following:

        BackgroundFetchContext::CreateJob()
        ~~~~ download happens ~~~~
        BackgroundFetchContext::DidCompleteJob()
        --> BackgroundFetchDataManager::GetFetchesForRegistration()
            (returns the Request/Response-info objects, plus the
             metadata we maintain like the download guid and path.)
        For each file...
            --> ChromeBlobStorageContext::CreateFileBackedBlob()
        --> BackgroundFetchEventDispatcher::Dispatch{..}Event()

      This would allow for much more focused unit testing by reducing
      the dependencies of the DataManager. It's also future proof -
      for MVP we only need to create blobs when a Background Fetch
      has completed (successfully or not), but for v2 we'll also need
      to be able to query for blobs for individual responses.

    - Now that we're going to work in the same files, we need to keep
      CLs smaller. There's a lot of conflict between this CL and code
      I wrote today to hook up end-to-end. That's OK, but it will
      happen more often, in both directions, and keeping up very
      quick iteration is going to be critical.

      A fair bit of this CL, notably GetJobResponse(), isn't yet used
      in the production code path, but will already have to change
      significantly in order to accommodate for integration. That's
      unfortunate and we need to improve our alignment.

      Is it feasible to take bits from this CL and land them
      separately? Since we're moving in directions regarding the
      DataManager, we need to align on that as soon as possible. I'll
      try to send a CL with the beginnings of my mental model
      tomorrow.

    - We really need to start working with the understanding that
      everything the DataManager does will be asynchronous. In order
      to avoid race conditions, we may in fact have to go one step
      further and actually start scheduling operations.

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_data_manager.cc (right):

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:90:
*(request_infos.begin() + request_index);
Why wouldn't request_infos[request_index] work?

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:104:
std::move(callback)));
This std::move() doesn't do anything because |callback| is a const ref.

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:115:
content::BrowserContext::CreateFileBackedBlob(
That method only exists to bridge UI thread calls to the IO thread. Instead, we
can get the ChromeBlobStorageContext in the BackgroundFetchContext and use that
directly on the IO thread. That way this class (or a class specific to blob
management?) doesn't have to know about all of |browser_context_|.

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:146:
DCHECK(job_info);
This is the sort of error condition that we'll have to be very careful with.
When this class becomes asynchronous, it'll become very hard to reason about
assumptions whether something is available or not - a task posted to a busy
message loop is all it takes to hit the DCHECK by accident.

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:214: const
std::string& download_guid) {
I'd love to simplify the data model away from GUIDs, because they're
very hard to reason about. What's your thoughts on something like:

  BackgroundFetchRegistrationId {origin, sw_registration_id, tag}
  --> int32_t request_index
      (targeted access to a request part of a BFRegistrationId)
  --- --> std::string request_download_guid
          (the GUID associated with the download)

We can give BackgroundFetchRegistrationId methods to serialize to and
from a string so that we can use those for the offline_item_collection
and co.

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

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:48: virtual
bool UpdateRequestState(const std::string& job_guid,
I'm not a huge fan of this approach. Ideally one would think of a class as an
API that defines a behaviour contract, with a (unit) test verifying that it
behaves according to that contract.

Looking at the JobController test, it would be much better if it would actually
create a new background fetch job, and then just operates on that data. It means
that the test would get some asynchronous aspects to it, but that'll have to
happen anyway.

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_info.h (right):

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_info.h:56: size_t
next_request_index() const { return next_request_index_; }
Instead of maintaining this here, have you considered having something like
DataManager::GetNextRequest() and just always querying that? This still feels
like a layer of caching to me.

Conceptionally a lot of what this class does feels like the JobController's
responsibility.

https://codereview.chromium.org/2767373002/diff/80001/content/public/test/fak...
File content/public/test/fake_download_item.cc (right):

https://codereview.chromium.org/2767373002/diff/80001/content/public/test/fak...
content/public/test/fake_download_item.cc:137: void
FakeDownloadItem::SetReceivedBytes(int64_t received_bytes) {
Should this trigger NotifyDownloadUpdated()?

[Peter Beverloo, 2017-03-25 03:54:29]

On 2017/03/25 03:38:51, Peter Beverloo wrote:
>       Is it feasible to take bits from this CL and land them
>       separately? Since we're moving in directions regarding the
>       DataManager, we need to align on that as soon as possible. I'll
>       try to send a CL with the beginnings of my mental model
>       tomorrow.

To be clear, it has my preference to land this, ideally with the Blob
interaction moved out of the DataManager, with the understanding that we'll be
refactoring most of it again.

[harkness, 2017-03-26 16:13:18]

At a high level, I was mostly thinking about the DataManager as a nice interface
to storage too, but I have been conceptually moving more data there as "less in
memory, more in storage" became the mantra. 

I'll definitely update the Blob query code to be synchronous, I didn't notice
that it was only async for the thread switch.

If you want me to, I can move the blob query to BackgroundFetchContext, but I
think the DataManager will need the BrowserContext visibility anyway for storage
so I'm not sure it's much of a win. It does seem odd to me to have the
DataManager build a temporary list of files and state to pass to the Context to
then have the Context build the blobs.

For some reason my internet connection is really flaky right now, but I'll try
to get the synchronous version up for review today. I wanted to go ahead and
upload comments though before the code is done.

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_data_manager.cc (right):

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:90:
*(request_infos.begin() + request_index);
On 2017/03/25 03:38:50, Peter Beverloo wrote:
> Why wouldn't request_infos[request_index] work?

I could do that, but then the method couldn't be const (although it wouldn't
change the class because of the DCHECK).

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:104:
std::move(callback)));
On 2017/03/25 03:38:50, Peter Beverloo wrote:
> This std::move() doesn't do anything because |callback| is a const ref.

Done.

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:115:
content::BrowserContext::CreateFileBackedBlob(
On 2017/03/25 03:38:50, Peter Beverloo wrote:
> That method only exists to bridge UI thread calls to the IO thread. Instead,
we
> can get the ChromeBlobStorageContext in the BackgroundFetchContext and use
that
> directly on the IO thread. That way this class (or a class specific to blob
> management?) doesn't have to know about all of |browser_context_|.

Won't the DataManager need access to the browser context to get the service
worker storage though?

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:146:
DCHECK(job_info);
On 2017/03/25 03:38:50, Peter Beverloo wrote:
> This is the sort of error condition that we'll have to be very careful with.
> When this class becomes asynchronous, it'll become very hard to reason about
> assumptions whether something is available or not - a task posted to a busy
> message loop is all it takes to hit the DCHECK by accident.

The only way I could see this being hit would be if abort was called and it
cleaned up the local state in the DataManager before it gave the JobController a
chance to abort, which I don't think we should allow.

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:214: const
std::string& download_guid) {
On 2017/03/25 03:38:50, Peter Beverloo wrote:
> I'd love to simplify the data model away from GUIDs, because they're
> very hard to reason about. What's your thoughts on something like:
> 
>   BackgroundFetchRegistrationId {origin, sw_registration_id, tag}
>   --> int32_t request_index
>       (targeted access to a request part of a BFRegistrationId)
>   --- --> std::string request_download_guid
>           (the GUID associated with the download)
> 
> We can give BackgroundFetchRegistrationId methods to serialize to and
> from a string so that we can use those for the offline_item_collection
> and co.

I do hate that we have to have the download_guid and the request_guid and the
job_guid, so I'd be happy to get away from having all of those. Unfortunately,
we can't avoid the download_guid right now. But originally I was designing
everything to have maps in memory, so guids made sense performance wise, but now
that we're not going to hold those in memory, we can definitely try to remove
them.

It looks like the BackgroundFetchRegistrationId would serve the same purpose as
the JobIdentifier, but I'm a bit unclear on that. Are you thinking that the
BackgroundFetchRegistrationId would replace the JobInfo?

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

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:48: virtual
bool UpdateRequestState(const std::string& job_guid,
On 2017/03/25 03:38:50, Peter Beverloo wrote:
> I'm not a huge fan of this approach. Ideally one would think of a class as an
> API that defines a behaviour contract, with a (unit) test verifying that it
> behaves according to that contract.
> 
> Looking at the JobController test, it would be much better if it would
actually
> create a new background fetch job, and then just operates on that data. It
means
> that the test would get some asynchronous aspects to it, but that'll have to
> happen anyway.

Well, when I had the JobData, it had a very narrowly defined role, and it was
easy to create one for the JobController unittest to work with. Now that it's
the DataManager handling that role, it's a much more complicated object. This
particular code is the same, just moved from the JobData to the DataManager.

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_info.h (right):

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_info.h:56: size_t
next_request_index() const { return next_request_index_; }
On 2017/03/25 03:38:50, Peter Beverloo wrote:
> Instead of maintaining this here, have you considered having something like
> DataManager::GetNextRequest() and just always querying that? This still feels
> like a layer of caching to me.
> 
> Conceptionally a lot of what this class does feels like the JobController's
> responsibility.

Fundamentally, the class that says "ok, I just finished request n, now I want to
do request n+1" needs to know about all the requests. If we're not going to keep
that information in memory, then it makes sense to me to have that on the
DataManager, which is where the JobInfo lives, so that the DataManager can get
the next request from storage. 

wrt having a GetNextRequest() on the DataManager, that's what I have, and that
calls this to get the value for "n+1" so that it knows what to look for in
storage.

https://codereview.chromium.org/2767373002/diff/80001/content/public/test/fak...
File content/public/test/fake_download_item.cc (right):

https://codereview.chromium.org/2767373002/diff/80001/content/public/test/fak...
content/public/test/fake_download_item.cc:137: void
FakeDownloadItem::SetReceivedBytes(int64_t received_bytes) {
On 2017/03/25 03:38:50, Peter Beverloo wrote:
> Should this trigger NotifyDownloadUpdated()?

It could, but that would break the current model (see SetState()) that the test
author has to make 2 calls: 1 to update the DownloadItem and then 1 to
explicitly invoke the method on the Observer.

I wouldn't mind changing it to use that approach, but we'd need to change it
everywhere to be consistent.

[Peter Beverloo, 2017-03-26 22:32:39]

Let's chat first thing tomorrow.

> At a high level, I was mostly thinking about the DataManager as a nice
> interface to storage too, but I have been conceptually moving more data there
> as "less in memory, more in storage" became the mantra. 

That still allows us to distinguish between "get or manipulate the stored data"
and "do this operation with the stored data".

> If you want me to, I can move the blob query to BackgroundFetchContext, but I
> think the DataManager will need the BrowserContext visibility anyway for
> storage I'm not sure it's much of a win. It does seem odd to me to have the
> DataManager build a temporary list of files and state to pass to the Context
> to then have the Context build the blobs.

Our disagreement here boils down to whether the DataManager should just work on
the static data, or also manage the derived operations such as blob management.

Ideally, the DataManager would just get the ServiceWorkerContextWrapper which
we obtain from the StoragePartitionImpl. If at all possible I'd love us to drop
the dependency on the BrowserContext entirely.

(This reduces the DataManager's dependencies to the point where we never have to
mock anything out at all, we just give it a SWCW.)

If we could design the API in line with how we'd use it for the two JavaScript
events, so that it would return a vector of BackgroundFetchSettledFetch objects,
I'd be open to keeping blob management in there because means we can keep
things easier. That does mean that we need to figure out lifetime management.
Maybe just keep the BlobHandles alive for the duration of the event?

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_data_manager.cc (right):

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:90:
*(request_infos.begin() + request_index);
On 2017/03/26 16:13:18, harkness wrote:
> On 2017/03/25 03:38:50, Peter Beverloo wrote:
> > Why wouldn't request_infos[request_index] work?
> 
> I could do that, but then the method couldn't be const (although it wouldn't
> change the class because of the DCHECK).

This can't be const anyway - conceptually it could cause the database to be
initialized, or destroyed on corruption, etcetera.

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:115:
content::BrowserContext::CreateFileBackedBlob(
On 2017/03/26 16:13:18, harkness wrote:
> On 2017/03/25 03:38:50, Peter Beverloo wrote:
> > That method only exists to bridge UI thread calls to the IO thread. Instead,
> we
> > can get the ChromeBlobStorageContext in the BackgroundFetchContext and use
> that
> > directly on the IO thread. That way this class (or a class specific to blob
> > management?) doesn't have to know about all of |browser_context_|.
> 
> Won't the DataManager need access to the browser context to get the service
> worker storage though?

We'd give it the ServiceWorkerContextWrapper instead. Law of Demeter.

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:214: const
std::string& download_guid) {
On 2017/03/26 16:13:18, harkness wrote:
> On 2017/03/25 03:38:50, Peter Beverloo wrote:
> > I'd love to simplify the data model away from GUIDs, because they're
> > very hard to reason about. What's your thoughts on something like:
> > 
> >   BackgroundFetchRegistrationId {origin, sw_registration_id, tag}
> >   --> int32_t request_index
> >       (targeted access to a request part of a BFRegistrationId)
> >   --- --> std::string request_download_guid
> >           (the GUID associated with the download)
> > 
> > We can give BackgroundFetchRegistrationId methods to serialize to and
> > from a string so that we can use those for the offline_item_collection
> > and co.
> 
> I do hate that we have to have the download_guid and the request_guid and the
> job_guid, so I'd be happy to get away from having all of those. Unfortunately,
> we can't avoid the download_guid right now. But originally I was designing
> everything to have maps in memory, so guids made sense performance wise, but
now
> that we're not going to hold those in memory, we can definitely try to remove
> them.

Good!

> It looks like the BackgroundFetchRegistrationId would serve the same purpose
as
> the JobIdentifier, but I'm a bit unclear on that. Are you thinking that the
> BackgroundFetchRegistrationId would replace the JobInfo? 

You're adding a ton of responsibilities to the JobInfo in this CL. The scope of
the BackgroundFetchRegistrationId is defined per my CL -- it won't hold any data
beyond what it already has.

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_info.h (right):

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_info.h:56: size_t
next_request_index() const { return next_request_index_; }
On 2017/03/26 16:13:18, harkness wrote:
> On 2017/03/25 03:38:50, Peter Beverloo wrote:
> > Instead of maintaining this here, have you considered having something like
> > DataManager::GetNextRequest() and just always querying that? This still
feels
> > like a layer of caching to me.
> > 
> > Conceptionally a lot of what this class does feels like the JobController's
> > responsibility.
> 
> Fundamentally, the class that says "ok, I just finished request n, now I want
to
> do request n+1" needs to know about all the requests. If we're not going to
keep
> that information in memory, then it makes sense to me to have that on the
> DataManager, which is where the JobInfo lives, so that the DataManager can get
> the next request from storage. 

Yes, but why does the JobController need to know *which* requests are done? We
need the {total, finished} counts for purposes of the notification, other than
that it seems to me like we can limit that knowledge to the DataManager and
below.

https://codereview.chromium.org/2767373002/diff/100001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2767373002/diff/100001/content/browser/backgr...
content/browser/background_fetch/background_fetch_job_controller.cc:92:
item->GetReceivedBytes());
break;

[harkness, 2017-03-27 07:32:38]

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_data_manager.cc (right):

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:90:
*(request_infos.begin() + request_index);
On 2017/03/26 22:32:39, Peter Beverloo wrote:
> On 2017/03/26 16:13:18, harkness wrote:
> > On 2017/03/25 03:38:50, Peter Beverloo wrote:
> > > Why wouldn't request_infos[request_index] work?
> > 
> > I could do that, but then the method couldn't be const (although it wouldn't
> > change the class because of the DCHECK).
> 
> This can't be const anyway - conceptually it could cause the database to be
> initialized, or destroyed on corruption, etcetera.

Changed.

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:115:
content::BrowserContext::CreateFileBackedBlob(
On 2017/03/26 22:32:39, Peter Beverloo wrote:
> On 2017/03/26 16:13:18, harkness wrote:
> > On 2017/03/25 03:38:50, Peter Beverloo wrote:
> > > That method only exists to bridge UI thread calls to the IO thread.
Instead,
> > we
> > > can get the ChromeBlobStorageContext in the BackgroundFetchContext and use
> > that
> > > directly on the IO thread. That way this class (or a class specific to
blob
> > > management?) doesn't have to know about all of |browser_context_|.
> > 
> > Won't the DataManager need access to the browser context to get the service
> > worker storage though?
> 
> We'd give it the ServiceWorkerContextWrapper instead. Law of Demeter.

Fair enough.

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:214: const
std::string& download_guid) {
On 2017/03/26 22:32:39, Peter Beverloo wrote:
> On 2017/03/26 16:13:18, harkness wrote:
> > On 2017/03/25 03:38:50, Peter Beverloo wrote:
> > > I'd love to simplify the data model away from GUIDs, because they're
> > > very hard to reason about. What's your thoughts on something like:
> > > 
> > >   BackgroundFetchRegistrationId {origin, sw_registration_id, tag}
> > >   --> int32_t request_index
> > >       (targeted access to a request part of a BFRegistrationId)
> > >   --- --> std::string request_download_guid
> > >           (the GUID associated with the download)
> > > 
> > > We can give BackgroundFetchRegistrationId methods to serialize to and
> > > from a string so that we can use those for the offline_item_collection
> > > and co.
> > 
> > I do hate that we have to have the download_guid and the request_guid and
the
> > job_guid, so I'd be happy to get away from having all of those.
Unfortunately,
> > we can't avoid the download_guid right now. But originally I was designing
> > everything to have maps in memory, so guids made sense performance wise, but
> now
> > that we're not going to hold those in memory, we can definitely try to
remove
> > them.
> 
> Good!
> 
> > It looks like the BackgroundFetchRegistrationId would serve the same purpose
> as
> > the JobIdentifier, but I'm a bit unclear on that. Are you thinking that the
> > BackgroundFetchRegistrationId would replace the JobInfo? 
> 
> You're adding a ton of responsibilities to the JobInfo in this CL. The scope
of
> the BackgroundFetchRegistrationId is defined per my CL -- it won't hold any
data
> beyond what it already has.

All the knowledge now on the JobInfo are things that used to be on the JobData,
and it felt better to put them there than to put them directly on the
DataManager. However, from your comments it seems like you think they should
live on the JobController. Let's definitely talk today, because I think you and
I still have very different ideas of how everything fits together.

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_info.h (right):

https://codereview.chromium.org/2767373002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_info.h:56: size_t
next_request_index() const { return next_request_index_; }
On 2017/03/26 22:32:39, Peter Beverloo wrote:
> On 2017/03/26 16:13:18, harkness wrote:
> > On 2017/03/25 03:38:50, Peter Beverloo wrote:
> > > Instead of maintaining this here, have you considered having something
like
> > > DataManager::GetNextRequest() and just always querying that? This still
> feels
> > > like a layer of caching to me.
> > > 
> > > Conceptionally a lot of what this class does feels like the
JobController's
> > > responsibility.
> > 
> > Fundamentally, the class that says "ok, I just finished request n, now I
want
> to
> > do request n+1" needs to know about all the requests. If we're not going to
> keep
> > that information in memory, then it makes sense to me to have that on the
> > DataManager, which is where the JobInfo lives, so that the DataManager can
get
> > the next request from storage. 
> 
> Yes, but why does the JobController need to know *which* requests are done? We
> need the {total, finished} counts for purposes of the notification, other than
> that it seems to me like we can limit that knowledge to the DataManager and
> below.

I don't think the JobController should know anything about past or future
requests. All it knows about right now is the request it's processing and I
think that's the way it should stay.

https://codereview.chromium.org/2767373002/diff/100001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2767373002/diff/100001/content/browser/backgr...
content/browser/background_fetch/background_fetch_job_controller.cc:92:
item->GetReceivedBytes());
On 2017/03/26 22:32:39, Peter Beverloo wrote:
> break;

The following code (in the CANCELLED case) needs to be called for both CANCELLED
and COMPLETE, since that's what starts the next request or marks the job as
done.

[harkness, 2017-03-27 08:19:08]

https://codereview.chromium.org/2767373002/diff/100001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2767373002/diff/100001/content/browser/backgr...
content/browser/background_fetch/background_fetch_job_controller.cc:92:
item->GetReceivedBytes());
On 2017/03/27 07:32:38, harkness wrote:
> On 2017/03/26 22:32:39, Peter Beverloo wrote:
> > break;
> 
> The following code (in the CANCELLED case) needs to be called for both
CANCELLED
> and COMPLETE, since that's what starts the next request or marks the job as
> done.

I added a "Fall through" comment to avoid future confusion.

[harkness, 2017-03-27 08:23:44]

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

[commit-bot: I haz the power, 2017-03-27 08:23:53]

Dry run: 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-27 09:31:11]

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

[commit-bot: I haz the power, 2017-03-27 09:31:12]

Dry run: This issue passed the CQ dry run.

[harkness, 2017-03-27 09:44:44]

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

[harkness, 2017-03-27 09:44:45]

+avi for file deletion and changes in content/public/test/fake_download_item.*

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

lgtm

https://codereview.chromium.org/2767373002/diff/140001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_data_manager.h (right):

https://codereview.chromium.org/2767373002/diff/140001/content/browser/backgr...
content/browser/background_fetch/background_fetch_data_manager.h:38:
BackgroundFetchRequestInfos request_infos);
It's rather confusing that we use the typedef in some places, and the raw type
in others. Can we always use the raw type?

https://codereview.chromium.org/2767373002/diff/140001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_job_response_data.h
(right):

https://codereview.chromium.org/2767373002/diff/140001/content/browser/backgr...
content/browser/background_fetch/background_fetch_job_response_data.h:39: };
DISALLOW_COPY_AND_ASSIGN

[harkness, 2017-03-27 11:47:49]

https://codereview.chromium.org/2767373002/diff/140001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_data_manager.h (right):

https://codereview.chromium.org/2767373002/diff/140001/content/browser/backgr...
content/browser/background_fetch/background_fetch_data_manager.h:38:
BackgroundFetchRequestInfos request_infos);
On 2017/03/27 10:01:14, Peter Beverloo wrote:
> It's rather confusing that we use the typedef in some places, and the raw type
> in others. Can we always use the raw type?

Done.

https://codereview.chromium.org/2767373002/diff/140001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_job_response_data.h
(right):

https://codereview.chromium.org/2767373002/diff/140001/content/browser/backgr...
content/browser/background_fetch/background_fetch_job_response_data.h:39: };
On 2017/03/27 10:01:14, Peter Beverloo wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Avi (use Gerrit), 2017-03-27 14:57:36]

content/public lgtm

[Peter Beverloo, 2017-03-27 15:00:32]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-03-27 15:00:33]

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/2767373002/#ps180001
(title: "Removed typedef and added DISALLOW_COPY_AND_ASSIGN")

[commit-bot: I haz the power, 2017-03-27 15:00:56]

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-27 16:15:14]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490626832811830,
"parent_rev": "97eaf60663ebf818732a387b27151150404cdfac", "commit_rev":
"4a4b593243e2acdedd593b84db51de418a801663"}

[commit-bot: I haz the power, 2017-03-27 16:16:09]

Description was changed from

==========
Implement GetJobResponse and merge JobData into DataManager.

This CL has two major changes. The first is the addition of the
GetJobResponse call on the DataManager. This Creates a new (and likely
temporary) BackgroundFetchJobResponseData which is owned by the JobInfo
and exists to collect the information needed to construct the Response
that will eventually be sent to Mojo.

The second major change is merging the JobData back into the DataManager.
The JobData was always intended as a cache layer to assist in fetches
involving large numbers of files in a batch, but given the desire to
reduce the memory footprint of Chrome and the relative rarity of these
operations, the cache layer is being removed in favor of storage accesses.

Minor changes:
* More attempts to isolate the eventual storage interface.
* Methods on the JobData which were moved to the DataManager are now virtual
  for testing.
* DataManager takes a BrowserContext* instead of BackgroundFetchContext*.
* Much of the batch management which was in JobData is now handled in
  BackgroundFetchJobInfo.

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

to

==========
Implement GetJobResponse and merge JobData into DataManager.

This CL has two major changes. The first is the addition of the
GetJobResponse call on the DataManager. This Creates a new (and likely
temporary) BackgroundFetchJobResponseData which is owned by the JobInfo
and exists to collect the information needed to construct the Response
that will eventually be sent to Mojo.

The second major change is merging the JobData back into the DataManager.
The JobData was always intended as a cache layer to assist in fetches
involving large numbers of files in a batch, but given the desire to
reduce the memory footprint of Chrome and the relative rarity of these
operations, the cache layer is being removed in favor of storage accesses.

Minor changes:
* More attempts to isolate the eventual storage interface.
* Methods on the JobData which were moved to the DataManager are now virtual
  for testing.
* DataManager takes a BrowserContext* instead of BackgroundFetchContext*.
* Much of the batch management which was in JobData is now handled in
  BackgroundFetchJobInfo.

BUG=692621

Review-Url: https://codereview.chromium.org/2767373002
Cr-Commit-Position: refs/heads/master@{#459793}
Committed:
https://chromium.googlesource.com/chromium/src/+/4a4b593243e2acdedd593b84db51...
==========

[commit-bot: I haz the power, 2017-03-27 16:16:10]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/4a4b593243e2acdedd593b84db51...