Initial framework setup and skeleton for BackgroundFetchContext

Initial framework setup and skeleton for BackgroundFetchContext

The Background Fetch system will be a service worker based mechanism for
allowing persistent and reliable uploads and downloads.

This CL sets up some of the very basic framework, including the directory
and BackgroundFetchContext, which will be the main dispatcher for the
service. Once this CL is in place, future CLs will quickly iterate 
adding specific functionality.

BUG=679300
Review-Url: https://codereview.chromium.org/2678273003
Cr-Commit-Position: refs/heads/master@{#450785}
Committed: https://chromium.googlesource.com/chromium/src/+/7b36c43b8d390438422b8fcaaeaf97cb15f8e625

[harkness, 2017-02-07 18:23:30]

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

[Peter Beverloo, 2017-02-08 12:20:25]

Is there some basic piece of functionality that we can add in on this patch? It
does literally nothing but allocate a word of memory, so while iteration is
good, maybe we can take another small step?

https://codereview.chromium.org/2678273003/diff/1/content/browser/background_...
File content/browser/background_fetch/OWNERS (right):

https://codereview.chromium.org/2678273003/diff/1/content/browser/background_...
content/browser/background_fetch/OWNERS:1: beverloo@chromium.org
who is this person?

https://codereview.chromium.org/2678273003/diff/1/content/browser/background_...
content/browser/background_fetch/OWNERS:4: # TEAM:
background-fetch-dev@chromium.org
nit: platform-capabilities@chromium.org

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

https://codereview.chromium.org/2678273003/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.cc:17:
BackgroundFetchContext::~BackgroundFetchContext() = default;
Please DCHECK_CURRENTLY_ON here too (shutdown threading matters, we've seen
plenty of bugs there)

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

https://codereview.chromium.org/2678273003/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.h:23:
BackgroundFetchContext(BrowserContext* browser_context);
nit: explicit

[harkness, 2017-02-08 16:30:42]

Minor functionality added (service worker observation). I'm happy to add more
before committing, but the next piece is going to be adding in the metadata
management, which doesn't break into super small pieces.

https://codereview.chromium.org/2678273003/diff/1/content/browser/background_...
File content/browser/background_fetch/OWNERS (right):

https://codereview.chromium.org/2678273003/diff/1/content/browser/background_...
content/browser/background_fetch/OWNERS:1: beverloo@chromium.org
On 2017/02/08 12:20:25, Peter Beverloo wrote:
> who is this person?

Acknowledged.

https://codereview.chromium.org/2678273003/diff/1/content/browser/background_...
content/browser/background_fetch/OWNERS:4: # TEAM:
background-fetch-dev@chromium.org
On 2017/02/08 12:20:25, Peter Beverloo wrote:
> nit: mailto:platform-capabilities@chromium.org

Done.

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

https://codereview.chromium.org/2678273003/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.cc:17:
BackgroundFetchContext::~BackgroundFetchContext() = default;
On 2017/02/08 12:20:25, Peter Beverloo wrote:
> Please DCHECK_CURRENTLY_ON here too (shutdown threading matters, we've seen
> plenty of bugs there)

Done.

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

https://codereview.chromium.org/2678273003/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.h:23:
BackgroundFetchContext(BrowserContext* browser_context);
On 2017/02/08 12:20:25, Peter Beverloo wrote:
> nit: explicit

Done.

[Peter Beverloo, 2017-02-09 15:48:55]

A few nits. Let's include the metadata management bit in here too then. This is
trivial enough, so there's little to no overhead.

https://codereview.chromium.org/2678273003/diff/40001/content/browser/backgro...
File content/browser/background_fetch/OWNERS (right):

https://codereview.chromium.org/2678273003/diff/40001/content/browser/backgro...
content/browser/background_fetch/OWNERS:5: # COMPONENT Blink>BackgroundFetch
nit: missing :

https://codereview.chromium.org/2678273003/diff/40001/content/browser/backgro...
File content/browser/background_fetch/PRESUBMIT.py (right):

https://codereview.chromium.org/2678273003/diff/40001/content/browser/backgro...
content/browser/background_fetch/PRESUBMIT.py:12: return
input_api.canned_checks.CheckPatchFormatted(input_api, output_api)
Please delete this file - it's now enforced for all our code:

    https://codereview.chromium.org/2680413002/

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

https://codereview.chromium.org/2678273003/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:19:
service_worker_context_->AddObserver(this);
The Service Worker Context's observer list is thread-hostile, which here means
that we need to add and remove the observer on the thread that we want to
receive the methods on.

It seems to me like we'd want to receive OnRegistrationDeleted() on the IO
thread. That means we already need InitOnIO and ShutdownOnIO methods to make
sure we can register ourselves as the observer on the right thread.

However, perhaps we should punt that until a time where we'll start to use the
observer? It should be unit-testable.

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

https://codereview.chromium.org/2678273003/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.h:44:
~BackgroundFetchContext() override;
Lines 43/44 can be private too, so no need for the protected block.

[harkness, 2017-02-13 17:20:05]

None of this is really final, and a lot is unused at the moment, so if you want
to just continue glancing at the code as I iterate, I don't care whether this is
committed step by step. Up to you.

https://codereview.chromium.org/2678273003/diff/40001/content/browser/backgro...
File content/browser/background_fetch/OWNERS (right):

https://codereview.chromium.org/2678273003/diff/40001/content/browser/backgro...
content/browser/background_fetch/OWNERS:5: # COMPONENT Blink>BackgroundFetch
On 2017/02/09 15:48:54, Peter Beverloo wrote:
> nit: missing :

Done.

https://codereview.chromium.org/2678273003/diff/40001/content/browser/backgro...
File content/browser/background_fetch/PRESUBMIT.py (right):

https://codereview.chromium.org/2678273003/diff/40001/content/browser/backgro...
content/browser/background_fetch/PRESUBMIT.py:12: return
input_api.canned_checks.CheckPatchFormatted(input_api, output_api)
On 2017/02/09 15:48:55, Peter Beverloo wrote:
> Please delete this file - it's now enforced for all our code:
> 
>     https://codereview.chromium.org/2680413002/

Done.

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

https://codereview.chromium.org/2678273003/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:19:
service_worker_context_->AddObserver(this);
On 2017/02/09 15:48:55, Peter Beverloo wrote:
> The Service Worker Context's observer list is thread-hostile, which here means
> that we need to add and remove the observer on the thread that we want to
> receive the methods on.
> 
> It seems to me like we'd want to receive OnRegistrationDeleted() on the IO
> thread. That means we already need InitOnIO and ShutdownOnIO methods to make
> sure we can register ourselves as the observer on the right thread.
> 
> However, perhaps we should punt that until a time where we'll start to use the
> observer? It should be unit-testable.

I'll definitely look into this when I start cleaning up for those methods, but
that's a few steps down my road right now.

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

https://codereview.chromium.org/2678273003/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.h:44:
~BackgroundFetchContext() override;
On 2017/02/09 15:48:55, Peter Beverloo wrote:
> Lines 43/44 can be private too, so no need for the protected block.

Done.

[Peter Beverloo, 2017-02-13 18:35:06]

I'm OK with iterating in-tree, modulo the following comments:

  - Let's not *add* our Service Worker observer on a thread where we wouldn't
use it anyway. I'm fine with continuing to accept the parameter to the context.
  - Ownership for raw pointers needs to be defined as comments in the code.
(Your unit test is leaking memory.)
  - The FetchRequest class cannot work right now because one cannot download a
url::Origin.

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

https://codereview.chromium.org/2678273003/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:19:
service_worker_context_->AddObserver(this);
On 2017/02/13 17:20:05, harkness wrote:
> On 2017/02/09 15:48:55, Peter Beverloo wrote:
> > The Service Worker Context's observer list is thread-hostile, which here
means
> > that we need to add and remove the observer on the thread that we want to
> > receive the methods on.
> > 
> > It seems to me like we'd want to receive OnRegistrationDeleted() on the IO
> > thread. That means we already need InitOnIO and ShutdownOnIO methods to make
> > sure we can register ourselves as the observer on the right thread.
> > 
> > However, perhaps we should punt that until a time where we'll start to use
the
> > observer? It should be unit-testable.
> 
> I'll definitely look into this when I start cleaning up for those methods, but
> that's a few steps down my road right now.

Let's then have a TODO for registering an observer on the
|service_worker_context| as opposed to registering one we won't be able to use.

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

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:37: //
TODO(harkness) Create the Data manager.
nit: delete (see line 21)

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:60:
content::StoragePartition* storage_partition =
drop `content::` everywhere - you are in that namespace

Taking a step back, all of lines 58-67 should probably not be in this patch.
We're getting a storage partition, then allocating a DownloadUrlParameters
object with its request context, only to delete it immediately after.

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

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.h:30: explicit
BackgroundFetchContext(
nit: `explicit` is only necessary for constructors that take a single argument

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

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:23:
FetchIdentifier id = {fetch_request.origin(), fetch_request.tag()};
nit: I think you can drop the " = " and call the constructor instead.

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

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:25:
BackgroundFetchDataManager(BackgroundFetchContext* background_fetch_context);
nit: explicit

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:28: void
CreateRequest(const FetchRequest& fetch_request);
nit: This is a public method, please document it

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:31:
BackgroundFetchContext* background_fetch_context_;
Raw pointers need a line of documentation about why it's safe to have a raw
pointer. (Your unit test is now leaking memory.)

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:35:
std::map<FetchIdentifier, FetchRequest> fetch_map_;
We know this to be functionally wrong - fetches will be scoped by Service Worker
Registration ID, not by origin. Can we skip this intermediate step and just use
an int64_t?

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:36: };
DISALLOW_COPY_AND_ASSIGN()

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

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager_unittest.cc:27:
new ServiceWorkerContextWrapper(browser_context_.get())))) {}
I don't think this is OK. See EmbeddedWorkerTestHelper and co.

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager_unittest.cc:36:
std::unique_ptr<TestBrowserContext> browser_context_;
nit: no need to use an std::unique_ptr<>, just have a TestBrowserContext as a
member

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

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/fetch_request.cc:13: : origin_(), url_(),
tag_(), download_item_(nullptr) {}
nit: members initialize themselves. you only need to list them in the
constructor if you need to assign them values, or call them if they're a parent
class.

As for |download_item_|, you could assign `nullptr` in the header file's
declaration to avoid having to do it here.

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/fetch_request.cc:13: : origin_(), url_(),
tag_(), download_item_(nullptr) {}
nit: {} -> default, everywhere else in this file too

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

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/fetch_request.h:7: 
nit: <string>

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/fetch_request.h:16: class CONTENT_EXPORT
FetchRequest {
We're really going to have to think about this class. Logically we'll have a
background fetch "registration" that has metadata, which has one or more
background fetch requests, each of which has a ServiceWorkerFetchRequest object,
as well as data associating it with the in-progress download (GUID and
download_item). Part of that has to persist on disk, part of that doesn't.

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/fetch_request.h:20: const url::Origin& url,
Why is |url| a url::Origin? At the very least it needs to be a GURL.

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/fetch_request.h:23: FetchRequest(const
FetchRequest& request);
q: where would we copy instances of this object?

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/fetch_request.h:37: DownloadItem*
download_item_;
nit: document what the ownership model is

[harkness, 2017-02-14 13:52:47]

I removed the Add/Remove observer calls. Ideally we'd like to get those off the
UI thread, but that only makes sense if the DataManager calls it will make are
also not on the UI thread.

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

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:37: //
TODO(harkness) Create the Data manager.
On 2017/02/13 18:35:05, Peter Beverloo wrote:
> nit: delete (see line 21)

Done.

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.cc:60:
content::StoragePartition* storage_partition =
On 2017/02/13 18:35:05, Peter Beverloo wrote:
> drop `content::` everywhere - you are in that namespace
> 
> Taking a step back, all of lines 58-67 should probably not be in this patch.
> We're getting a storage partition, then allocating a DownloadUrlParameters
> object with its request context, only to delete it immediately after.

Done.

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

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.h:30: explicit
BackgroundFetchContext(
On 2017/02/13 18:35:05, Peter Beverloo wrote:
> nit: `explicit` is only necessary for constructors that take a single argument

Done.

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

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.cc:23:
FetchIdentifier id = {fetch_request.origin(), fetch_request.tag()};
On 2017/02/13 18:35:05, Peter Beverloo wrote:
> nit: I think you can drop the " = " and call the constructor instead.

Done.

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

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:25:
BackgroundFetchDataManager(BackgroundFetchContext* background_fetch_context);
On 2017/02/13 18:35:06, Peter Beverloo wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:28: void
CreateRequest(const FetchRequest& fetch_request);
On 2017/02/13 18:35:06, Peter Beverloo wrote:
> nit: This is a public method, please document it

Done.

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:31:
BackgroundFetchContext* background_fetch_context_;
On 2017/02/13 18:35:05, Peter Beverloo wrote:
> Raw pointers need a line of documentation about why it's safe to have a raw
> pointer. (Your unit test is now leaking memory.)

Done.

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:35:
std::map<FetchIdentifier, FetchRequest> fetch_map_;
On 2017/02/13 18:35:06, Peter Beverloo wrote:
> We know this to be functionally wrong - fetches will be scoped by Service
Worker
> Registration ID, not by origin. Can we skip this intermediate step and just
use
> an int64_t?

Done.

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:36: };
On 2017/02/13 18:35:06, Peter Beverloo wrote:
> DISALLOW_COPY_AND_ASSIGN()

Done.

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

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager_unittest.cc:27:
new ServiceWorkerContextWrapper(browser_context_.get())))) {}
On 2017/02/13 18:35:06, Peter Beverloo wrote:
> I don't think this is OK. See EmbeddedWorkerTestHelper and co.

Ah, that's a nifty helper class. Updated.

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager_unittest.cc:36:
std::unique_ptr<TestBrowserContext> browser_context_;
On 2017/02/13 18:35:06, Peter Beverloo wrote:
> nit: no need to use an std::unique_ptr<>, just have a TestBrowserContext as a
> member

Done.

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

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/fetch_request.cc:13: : origin_(), url_(),
tag_(), download_item_(nullptr) {}
On 2017/02/13 18:35:06, Peter Beverloo wrote:
> nit: members initialize themselves. you only need to list them in the
> constructor if you need to assign them values, or call them if they're a
parent
> class.
> 
> As for |download_item_|, you could assign `nullptr` in the header file's
> declaration to avoid having to do it here.

Done.

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/fetch_request.cc:13: : origin_(), url_(),
tag_(), download_item_(nullptr) {}
On 2017/02/13 18:35:06, Peter Beverloo wrote:
> nit: {} -> default, everywhere else in this file too

It seems that our compiler doesn't like = default after the member
inititalizations, but I changed it in the other two places.

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

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/fetch_request.h:7: 
On 2017/02/13 18:35:06, Peter Beverloo wrote:
> nit: <string>

Done.

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/fetch_request.h:16: class CONTENT_EXPORT
FetchRequest {
On 2017/02/13 18:35:06, Peter Beverloo wrote:
> We're really going to have to think about this class. Logically we'll have a
> background fetch "registration" that has metadata, which has one or more
> background fetch requests, each of which has a ServiceWorkerFetchRequest
object,
> as well as data associating it with the in-progress download (GUID and
> download_item). Part of that has to persist on disk, part of that doesn't.

Yes, how that registration is divided between the (to-be-written)BatchManager
and the DataManager is an open question as well.

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/fetch_request.h:20: const url::Origin& url,
On 2017/02/13 18:35:06, Peter Beverloo wrote:
> Why is |url| a url::Origin? At the very least it needs to be a GURL.

Good point.

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/fetch_request.h:23: FetchRequest(const
FetchRequest& request);
On 2017/02/13 18:35:06, Peter Beverloo wrote:
> q: where would we copy instances of this object?

map uses it

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/fetch_request.h:37: DownloadItem*
download_item_;
On 2017/02/13 18:35:06, Peter Beverloo wrote:
> nit: document what the ownership model is

I removed this, since I don't need DownloadItems until I have the download
observers.

[Peter Beverloo, 2017-02-14 15:05:53]

lgtm % comments

Please clarify in the CL description that your intention is to build upon this
to iterate with focused CLs. Let's also agree that each of those CLs should
implement a part of functionality that can be (unit) tested.

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

https://codereview.chromium.org/2678273003/diff/80001/content/browser/backgro...
content/browser/background_fetch/fetch_request.h:23: FetchRequest(const
FetchRequest& request);
On 2017/02/14 13:52:47, harkness wrote:
> On 2017/02/13 18:35:06, Peter Beverloo wrote:
> > q: where would we copy instances of this object?
> 
> map uses it

Let's add a TODO to get rid of it since the map is temporary. It'll end up
making copies of the std::strings too, including the url::Origin backings, so
that's eight allocations *per copy*.

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_context.cc (right):

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
content/browser/background_fetch/background_fetch_context.cc:51: //
TODO(harkness)ake the request to the download manager.
Please fix the grammar of this TODO

As a minor aside for future patches, please add a colon after the TODO. Only
6.4% of TODOs in //chrome and //content don't have them.

    // TODO(foo): bar

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_context.h (right):

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
content/browser/background_fetch/background_fetch_context.h:25: : public
ServiceWorkerContextObserver,
nit: let's remove this parent class, as well as the OnRegistrationDeleted()
method, since they're not needed yet. Keeping the argument sgtm w/ a TODO in the
constructor.

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

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
content/browser/background_fetch/background_fetch_data_manager.h:35: //
BackgroundFetchContext contains a BackgroundFetchDataManager, so the
contains a -> owns this

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_data_manager_unittest.cc
(right):

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
content/browser/background_fetch/background_fetch_data_manager_unittest.cc:21:
const int64_t kServiceWorkerRegistrationId = 9001;
constexpr

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
File content/browser/background_fetch/fetch_request.cc (right):

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
content/browser/background_fetch/fetch_request.cc:14:
FetchRequest::FetchRequest() = default;
You need to initialize sw_registration_id_ here and in the copy constructor.

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
content/browser/background_fetch/fetch_request.cc:18: int64_t
sw_registration_id,
note: try to avoid acronyms (sw -> service_worker_registration_id)

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
File content/browser/background_fetch/fetch_request.h (right):

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
content/browser/background_fetch/fetch_request.h:14: // Simple class to
encapsulate the components of a fetch request.
Please extend this in the future. "a fetch request" is highly ambiguous

[harkness, 2017-02-15 13:45:46]

Description was changed from

==========
Initial framework setup and skeleton for BackgroundFetchContext

The Background Fetch system will be a service worker based mechanism for
allowing persistent and reliable uploads and downloads.

This CL sets up some of the very basic framework, including the directory
and BackgroundFetchContext, which will be the main dispatcher for the
service.

BUG=679300
==========

to

==========
Initial framework setup and skeleton for BackgroundFetchContext

The Background Fetch system will be a service worker based mechanism for
allowing persistent and reliable uploads and downloads.

This CL sets up some of the very basic framework, including the directory
and BackgroundFetchContext, which will be the main dispatcher for the
service. Once this CL is in place, future CLs will quickly iterate adding
specific functionality.

BUG=679300
==========

[harkness, 2017-02-15 13:46:00]

Description was changed from

==========
Initial framework setup and skeleton for BackgroundFetchContext

The Background Fetch system will be a service worker based mechanism for
allowing persistent and reliable uploads and downloads.

This CL sets up some of the very basic framework, including the directory
and BackgroundFetchContext, which will be the main dispatcher for the
service. Once this CL is in place, future CLs will quickly iterate adding
specific functionality.

BUG=679300
==========

to

==========
Initial framework setup and skeleton for BackgroundFetchContext

The Background Fetch system will be a service worker based mechanism for
allowing persistent and reliable uploads and downloads.

This CL sets up some of the very basic framework, including the directory
and BackgroundFetchContext, which will be the main dispatcher for the
service. Once this CL is in place, future CLs will quickly iterate 
adding specific functionality.

BUG=679300
==========

[harkness, 2017-02-15 13:48:43]

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

[harkness, 2017-02-15 13:48:45]

@avi - could you take a look at this please? 

This is the first of many CLs over the next 6 weeks which will fill in the
BackgroundFetch functionality. This is currently not instantiated, but is a very
bare-bones framework. My intention is to add CLs over the next few weeks each of
which will add one (testable) piece of functionality.

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_context.cc (right):

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
content/browser/background_fetch/background_fetch_context.cc:51: //
TODO(harkness)ake the request to the download manager.
On 2017/02/14 15:05:52, Peter Beverloo wrote:
> Please fix the grammar of this TODO
> 
> As a minor aside for future patches, please add a colon after the TODO. Only
> 6.4% of TODOs in //chrome and //content don't have them.
> 
>     // TODO(foo): bar

Done.

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_context.h (right):

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
content/browser/background_fetch/background_fetch_context.h:25: : public
ServiceWorkerContextObserver,
On 2017/02/14 15:05:52, Peter Beverloo wrote:
> nit: let's remove this parent class, as well as the OnRegistrationDeleted()
> method, since they're not needed yet. Keeping the argument sgtm w/ a TODO in
the
> constructor.

Done.

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

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
content/browser/background_fetch/background_fetch_data_manager.h:35: //
BackgroundFetchContext contains a BackgroundFetchDataManager, so the
On 2017/02/14 15:05:52, Peter Beverloo wrote:
> contains a -> owns this

Done.

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_data_manager_unittest.cc
(right):

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
content/browser/background_fetch/background_fetch_data_manager_unittest.cc:21:
const int64_t kServiceWorkerRegistrationId = 9001;
On 2017/02/14 15:05:52, Peter Beverloo wrote:
> constexpr

Done.

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
File content/browser/background_fetch/fetch_request.cc (right):

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
content/browser/background_fetch/fetch_request.cc:14:
FetchRequest::FetchRequest() = default;
On 2017/02/14 15:05:52, Peter Beverloo wrote:
> You need to initialize sw_registration_id_ here and in the copy constructor.

I gave it a default value.

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
content/browser/background_fetch/fetch_request.cc:18: int64_t
sw_registration_id,
On 2017/02/14 15:05:52, Peter Beverloo wrote:
> note: try to avoid acronyms (sw -> service_worker_registration_id)

Done.

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
File content/browser/background_fetch/fetch_request.h (right):

https://codereview.chromium.org/2678273003/diff/120001/content/browser/backgr...
content/browser/background_fetch/fetch_request.h:14: // Simple class to
encapsulate the components of a fetch request.
On 2017/02/14 15:05:52, Peter Beverloo wrote:
> Please extend this in the future. "a fetch request" is highly ambiguous

Acknowledged.

[Avi (use Gerrit), 2017-02-15 15:15:33]

lgtm

Best of luck in building this.

[harkness, 2017-02-15 15:49:48]

The CQ bit was checked by harkness@chromium.org

[harkness, 2017-02-15 15:49:48]

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/2678273003/#ps130001
(title: "Removed service worker observation and other code review nits")

[commit-bot: I haz the power, 2017-02-15 15:50:10]

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-02-15 17:04:45]

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

[commit-bot: I haz the power, 2017-02-15 17:04:46]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[harkness, 2017-02-15 18:28:49]

The CQ bit was checked by harkness@chromium.org

[harkness, 2017-02-15 18:28:49]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2678273003/#ps150001
(title: "Removed unused browser_context_ stored member.")

[commit-bot: I haz the power, 2017-02-15 18:29:55]

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-02-15 20:05:42]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1487183329257120,
"parent_rev": "eed2187ba7e9acb812754997400e56ff9107d8a0", "commit_rev":
"7b36c43b8d390438422b8fcaaeaf97cb15f8e625"}

[commit-bot: I haz the power, 2017-02-15 20:07:33]

Description was changed from

==========
Initial framework setup and skeleton for BackgroundFetchContext

The Background Fetch system will be a service worker based mechanism for
allowing persistent and reliable uploads and downloads.

This CL sets up some of the very basic framework, including the directory
and BackgroundFetchContext, which will be the main dispatcher for the
service. Once this CL is in place, future CLs will quickly iterate 
adding specific functionality.

BUG=679300
==========

to

==========
Initial framework setup and skeleton for BackgroundFetchContext

The Background Fetch system will be a service worker based mechanism for
allowing persistent and reliable uploads and downloads.

This CL sets up some of the very basic framework, including the directory
and BackgroundFetchContext, which will be the main dispatcher for the
service. Once this CL is in place, future CLs will quickly iterate 
adding specific functionality.

BUG=679300

Review-Url: https://codereview.chromium.org/2678273003
Cr-Commit-Position: refs/heads/master@{#450785}
Committed:
https://chromium.googlesource.com/chromium/src/+/7b36c43b8d390438422b8fcaaeaf...
==========

[commit-bot: I haz the power, 2017-02-15 20:07:34]

Committed patchset #9 (id:150001) as
https://chromium.googlesource.com/chromium/src/+/7b36c43b8d390438422b8fcaaeaf...