Implement BackgroundFetchManager.fetch() and struct traits

Implement BackgroundFetchManager.fetch() and struct traits

This implements the BackgroundFetchManager.fetch() method and sends it
to the browser process over IPC. Struct traits have been added for three
C++ types that are analogous to the Web exposed types:

  - BackgroundFetchOptions
    The options provided by the developer when starting the fetch.

  - BackgroundFetchRegistration
    The data associated with an in-progress background fetch.

  - IconDefinition
    The data associated with an individual icon definition. This will
    eventually move to a more generalized location.

What's still missing in the call is the sequence of Request objects.

BUG=692534, 692535
Review-Url: https://codereview.chromium.org/2762573003
Cr-Commit-Position: refs/heads/master@{#459412}
Committed: https://chromium.googlesource.com/chromium/src/+/a1ab5a9bd84f0a1cf5ebbd322124152bb4cd1323

[Peter Beverloo, 2017-03-20 17:49:45]

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

[commit-bot: I haz the power, 2017-03-20 17:50:05]

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

[Peter Beverloo, 2017-03-20 17:53:31]

Description was changed from

==========
Implement BackgroundFetchManager.fetch() and struct traits

This implements the BackgroundFetchManager.fetch() method and sends it
to the browser process over IPC. Struct traits have been added for three
C++ types that are analogous to the Web exposed types:

  - BackgroundFetchOptions
    The options provided by the developer when starting the fetch.

  - BackgroundFetchRegistration
    The data associated with an in-progress background fetch.

  - IconDefinition
    The data associated with an individual icon definition. This will
    eventually move to a more generalized location.

What's still missing is the sequence of Request objects. Although we now
convert the developer-provided requests to a sequence of
WebServiceWorkerRequest objects, they're not yet being send over Mojo.

BUG=692534, 692535
==========

to

==========
Implement BackgroundFetchManager.fetch() and struct traits

This implements the BackgroundFetchManager.fetch() method and sends it
to the browser process over IPC. Struct traits have been added for three
C++ types that are analogous to the Web exposed types:

  - BackgroundFetchOptions
    The options provided by the developer when starting the fetch.

  - BackgroundFetchRegistration
    The data associated with an in-progress background fetch.

  - IconDefinition
    The data associated with an individual icon definition. This will
    eventually move to a more generalized location.

What's still missing in the call is the sequence of Request objects.

BUG=692534, 692535
==========

[Peter Beverloo, 2017-03-20 17:55:26]

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

[commit-bot: I haz the power, 2017-03-20 17:55:32]

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

[Peter Beverloo, 2017-03-20 18:03:36]

Patchset #2 (id:20001) has been deleted

[Peter Beverloo, 2017-03-20 18:04:04]

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

[commit-bot: I haz the power, 2017-03-20 18:04:33]

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

[Peter Beverloo, 2017-03-20 18:04:44]

peter@chromium.org changed reviewers:
+ avi@chromium.org, dcheng@chromium.org, harkness@chromium.org,
tsepez@chromium.org

[Peter Beverloo, 2017-03-20 18:04:44]

+harkness for background fetch
+dcheng for Mojo (also +tsepez, although this is somewhat Blink-heavy)
+avi for //content/common/ additions and BUILD.gn files.

[Avi (use Gerrit), 2017-03-20 18:08:42]

lgtm

stampity stamp

[commit-bot: I haz the power, 2017-03-20 19:22:14]

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

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

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dcheng, 2017-03-21 07:03:20]

https://codereview.chromium.org/2762573003/diff/60001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_service_impl.cc (right):

https://codereview.chromium.org/2762573003/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_service_impl.cc:58:
std::move(registration));
FWIW, I believe that BackgroundFetchRegistration is actually non-movable (so
this std::move is a no-op). Also #include <utility> if we do make it movable.

https://codereview.chromium.org/2762573003/diff/60001/content/common/backgrou...
File content/common/background_fetch/background_fetch_struct_traits_unittest.cc
(right):

https://codereview.chromium.org/2762573003/diff/60001/content/common/backgrou...
content/common/background_fetch/background_fetch_struct_traits_unittest.cc:15:
IconDefinition CreateIconDefinition(std::string src,
FWIW, it's still more common to pass by ref and just copy. While it can be more
efficient, there's a binary size tradeoff: since the arguments are passed by
value, the compiler often has to emit code to clean up the temporaries (even if
the value is moved out).

https://codereview.chromium.org/2762573003/diff/60001/content/common/backgrou...
content/common/background_fetch/background_fetch_struct_traits_unittest.cc:19:
definition.src = std::move(src);
Nit: #include <utility>

https://codereview.chromium.org/2762573003/diff/60001/content/common/backgrou...
File content/common/background_fetch/background_fetch_types.h (right):

https://codereview.chromium.org/2762573003/diff/60001/content/common/backgrou...
content/common/background_fetch/background_fetch_types.h:21:
IconDefinition(const IconDefinition& other);
http://en.cppreference.com/w/cpp/language/move_constructor#Implicitly-declare...
are the rules for an implicitly declared move constructor, but since this class
has a user-defined copy constructor, I think we won't have any move semantics.

https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp
(right):

https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:10:
#include "modules/background_fetch/BackgroundFetchTypeConverters.h"
Let's use typemapping on the Blink side. It'll save some temporaries and be a
bit more efficient.

For a relatively simple example,
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mojo/...
should provide some useful pointers. I'm happy to provide detailed help as well
if there's issues getting this to work.

https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/background_fetch/background_fetch.mojom
(right):

https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/background_fetch/background_fetch.mojom:12:
struct IconDefinition {
It would be nice to point at the spec definition for these as appropriate, so
it's clear what the contents of the different fields can be. In particular, it
seems like these fields might need some sort of parsing, which always warrants
extra scrutiny (is it possible / does it make sense to parse it in Blink?)

[harkness, 2017-03-21 11:30:00]

lgtm BackgroundFetch

https://codereview.chromium.org/2762573003/diff/60001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_service_impl.cc (right):

https://codereview.chromium.org/2762573003/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_service_impl.cc:12: #include
"content/common/service_worker/service_worker_types.h"
#include background_fetch_types instead?

https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp
(right):

https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:83:
void BackgroundFetchBridge::didGetRegistration(
optional: I'm glad that we're reusing the code for both fetch and
getRegistration, but consider renaming didGetRegistration to something more
generic so it doesn't look like a 1:1 mapping to getRegistration.

[Peter Beverloo, 2017-03-21 13:46:27]

Thanks! Let me reply to the Mojo suggestion separately...

https://codereview.chromium.org/2762573003/diff/60001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_service_impl.cc (right):

https://codereview.chromium.org/2762573003/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_service_impl.cc:12: #include
"content/common/service_worker/service_worker_types.h"
On 2017/03/21 11:30:00, harkness wrote:
> #include background_fetch_types instead?

Done.

https://codereview.chromium.org/2762573003/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_service_impl.cc:58:
std::move(registration));
On 2017/03/21 07:03:20, dcheng wrote:
> FWIW, I believe that BackgroundFetchRegistration is actually non-movable (so
> this std::move is a no-op). Also #include <utility> if we do make it movable.

Uch, sloppy. Sorry, done. I had been going back and forth on this code a bit.
I've kept it as a copy for now since we may not need move semantics.

https://codereview.chromium.org/2762573003/diff/60001/content/common/backgrou...
File content/common/background_fetch/background_fetch_struct_traits_unittest.cc
(right):

https://codereview.chromium.org/2762573003/diff/60001/content/common/backgrou...
content/common/background_fetch/background_fetch_struct_traits_unittest.cc:15:
IconDefinition CreateIconDefinition(std::string src,
On 2017/03/21 07:03:20, dcheng wrote:
> FWIW, it's still more common to pass by ref and just copy. While it can be
more
> efficient, there's a binary size tradeoff: since the arguments are passed by
> value, the compiler often has to emit code to clean up the temporaries (even
if
> the value is moved out).

That's good to know, thanks. I'll look at some disassembly later in the week to
grow a better understanding of this.

The arguments given to this function are string literals, so we end up creating
the temporaries anyway. I've left this call as-is for that reason.

https://codereview.chromium.org/2762573003/diff/60001/content/common/backgrou...
content/common/background_fetch/background_fetch_struct_traits_unittest.cc:19:
definition.src = std::move(src);
On 2017/03/21 07:03:20, dcheng wrote:
> Nit: #include <utility>

Done.

https://codereview.chromium.org/2762573003/diff/60001/content/common/backgrou...
File content/common/background_fetch/background_fetch_types.h (right):

https://codereview.chromium.org/2762573003/diff/60001/content/common/backgrou...
content/common/background_fetch/background_fetch_types.h:21:
IconDefinition(const IconDefinition& other);
On 2017/03/21 07:03:20, dcheng wrote:
>
http://en.cppreference.com/w/cpp/language/move_constructor#Implicitly-declare...
> are the rules for an implicitly declared move constructor, but since this
class
> has a user-defined copy constructor, I think we won't have any move semantics.

Acknowledged.

https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp
(right):

https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:83:
void BackgroundFetchBridge::didGetRegistration(
On 2017/03/21 11:30:00, harkness wrote:
> optional: I'm glad that we're reusing the code for both fetch and
> getRegistration, but consider renaming didGetRegistration to something more
> generic so it doesn't look like a 1:1 mapping to getRegistration.

Can you suggest a name that you think would be clearer? While I agree that the
overlap with the getRegistration() method is unfortunate, in both cases we did
get a registration.

https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/background_fetch/background_fetch.mojom
(right):

https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/background_fetch/background_fetch.mojom:12:
struct IconDefinition {
On 2017/03/21 07:03:20, dcheng wrote:
> It would be nice to point at the spec definition for these as appropriate, so
> it's clear what the contents of the different fields can be. In particular, it
> seems like these fields might need some sort of parsing, which always warrants
> extra scrutiny (is it possible / does it make sense to parse it in Blink?)

I've added links to their specifications. The links now occur in the code three
times for each type, but it should help people create a mental model of the
code. In the mid-term this'll move elsewhere, we're just not quite sure yet
where. (Other specs will use it too.)

We need to do two things with these values:
  (1) Fetch an appropriate image to use in the notification associated with the
background fetch. This will happen through an image loader in Blink, where we'll
also parse its values.
  (2) Store them and give them back to the developer when they get a
BackgroundFetchRegistration.

As such, we don't actually use or parse the values themselves in the browser, we
merely store them. I've extended the comment in background_fetch_types.h to
explain this intent.

[Peter Beverloo, 2017-03-21 14:05:19]

https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp
(right):

https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:10:
#include "modules/background_fetch/BackgroundFetchTypeConverters.h"
On 2017/03/21 07:03:20, dcheng wrote:
> Let's use typemapping on the Blink side. It'll save some temporaries and be a
> bit more efficient.
> 
> For a relatively simple example,
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mojo/...
> should provide some useful pointers. I'm happy to provide detailed help as
well
> if there's issues getting this to work.

These types live in //Source/modules/, whereas the mojom lives in
//public/platform/ and are, on the Blink side, linked in as part of
//Source/platform/. The issue is that platform cannot depend on modules.

One approach would be to define another layer of wrapper types in platform that
we provide a typemap for, which the module then uses to create the actual types.
That seems unfortunate as it's more duplication.

Another approach would be to separate out the background fetch mojom (and
potentially other affected mojom files, e.g. NFC) to a new target that still
lives in //public/platform/, but whose Blink bindings will actually be linked in
with modules. That feels a bit icky as it requires the Blink embedder to know
about this distinction by depending on another target.

I'm not sure what the best way forward here is. What's your take?

[dcheng, 2017-03-22 09:30:38]

On 2017/03/21 14:05:19, Peter Beverloo wrote:
>
https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/Sour...
> File
> third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp
> (right):
> 
>
https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:10:
> #include "modules/background_fetch/BackgroundFetchTypeConverters.h"
> On 2017/03/21 07:03:20, dcheng wrote:
> > Let's use typemapping on the Blink side. It'll save some temporaries and be
a
> > bit more efficient.
> > 
> > For a relatively simple example,
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mojo/...
> > should provide some useful pointers. I'm happy to provide detailed help as
> well
> > if there's issues getting this to work.
> 
> These types live in //Source/modules/, whereas the mojom lives in
> //public/platform/ and are, on the Blink side, linked in as part of
> //Source/platform/. The issue is that platform cannot depend on modules.
> 
> One approach would be to define another layer of wrapper types in platform
that
> we provide a typemap for, which the module then uses to create the actual
types.
> That seems unfortunate as it's more duplication.
> 
> Another approach would be to separate out the background fetch mojom (and
> potentially other affected mojom files, e.g. NFC) to a new target that still
> lives in //public/platform/, but whose Blink bindings will actually be linked
in
> with modules. That feels a bit icky as it requires the Blink embedder to know
> about this distinction by depending on another target.
> 
> I'm not sure what the best way forward here is. What's your take?

Sorry I ran out of time to research this today =/

I'm going to poke around and see if i can find anything similar; you're
definitely right that we shouldn't have to define another layer of wrapper types
though. One question I have is: would it be possible to define the types in
platform, or does that not make any sense here?

[Peter Beverloo, 2017-03-22 14:13:14]

On 2017/03/22 09:30:38, dcheng wrote:
> On 2017/03/21 14:05:19, Peter Beverloo wrote:
> >
>
https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/Sour...
> > File
> > third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/Sour...
> >
>
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:10:
> > #include "modules/background_fetch/BackgroundFetchTypeConverters.h"
> > On 2017/03/21 07:03:20, dcheng wrote:
> > > Let's use typemapping on the Blink side. It'll save some temporaries and
be
> a
> > > bit more efficient.
> > > 
> > > For a relatively simple example,
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mojo/...
> > > should provide some useful pointers. I'm happy to provide detailed help as
> > well
> > > if there's issues getting this to work.
> > 
> > These types live in //Source/modules/, whereas the mojom lives in
> > //public/platform/ and are, on the Blink side, linked in as part of
> > //Source/platform/. The issue is that platform cannot depend on modules.
> > 
> > One approach would be to define another layer of wrapper types in platform
> that
> > we provide a typemap for, which the module then uses to create the actual
> types.
> > That seems unfortunate as it's more duplication.
> > 
> > Another approach would be to separate out the background fetch mojom (and
> > potentially other affected mojom files, e.g. NFC) to a new target that still
> > lives in //public/platform/, but whose Blink bindings will actually be
linked
> in
> > with modules. That feels a bit icky as it requires the Blink embedder to
know
> > about this distinction by depending on another target.
> > 
> > I'm not sure what the best way forward here is. What's your take?
> 
> Sorry I ran out of time to research this today =/
> 
> I'm going to poke around and see if i can find anything similar; you're
> definitely right that we shouldn't have to define another layer of wrapper
types
> though. One question I have is: would it be possible to define the types in
> platform, or does that not make any sense here?

The types (IconDefinition, BackgroundFetchOptions and
BackgroundFetchRegistration) are two IDL dictionaries and one IDL interface,
defined and generated in the module. That wouldn't make sense.

[dcheng, 2017-03-23 07:22:19]

On 2017/03/22 14:13:14, Peter Beverloo wrote:
> On 2017/03/22 09:30:38, dcheng wrote:
> > On 2017/03/21 14:05:19, Peter Beverloo wrote:
> > >
> >
>
https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/Sour...
> > > File
> > >
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2762573003/diff/60001/third_party/WebKit/Sour...
> > >
> >
>
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:10:
> > > #include "modules/background_fetch/BackgroundFetchTypeConverters.h"
> > > On 2017/03/21 07:03:20, dcheng wrote:
> > > > Let's use typemapping on the Blink side. It'll save some temporaries and
> be
> > a
> > > > bit more efficient.
> > > > 
> > > > For a relatively simple example,
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mojo/...
> > > > should provide some useful pointers. I'm happy to provide detailed help
as
> > > well
> > > > if there's issues getting this to work.
> > > 
> > > These types live in //Source/modules/, whereas the mojom lives in
> > > //public/platform/ and are, on the Blink side, linked in as part of
> > > //Source/platform/. The issue is that platform cannot depend on modules.
> > > 
> > > One approach would be to define another layer of wrapper types in platform
> > that
> > > we provide a typemap for, which the module then uses to create the actual
> > types.
> > > That seems unfortunate as it's more duplication.
> > > 
> > > Another approach would be to separate out the background fetch mojom (and
> > > potentially other affected mojom files, e.g. NFC) to a new target that
still
> > > lives in //public/platform/, but whose Blink bindings will actually be
> linked
> > in
> > > with modules. That feels a bit icky as it requires the Blink embedder to
> know
> > > about this distinction by depending on another target.
> > > 
> > > I'm not sure what the best way forward here is. What's your take?
> > 
> > Sorry I ran out of time to research this today =/
> > 
> > I'm going to poke around and see if i can find anything similar; you're
> > definitely right that we shouldn't have to define another layer of wrapper
> types
> > though. One question I have is: would it be possible to define the types in
> > platform, or does that not make any sense here?
> 
> The types (IconDefinition, BackgroundFetchOptions and
> BackgroundFetchRegistration) are two IDL dictionaries and one IDL interface,
> defined and generated in the module. That wouldn't make sense.

Ah... can we just define the typemap in the module then? Even though the mojom
is defined elsewhere, it seems like that ought to work.

[Peter Beverloo, 2017-03-23 19:50:01]

On 2017/03/23 07:22:19, dcheng wrote:
> Ah... can we just define the typemap in the module then? Even though the mojom
> is defined elsewhere, it seems like that ought to work.

No, due to garbage collection. I uploaded a CL to try this here:
  https://codereview.chromium.org/2772893002/

Using a typemap for non-GCed objects and enumerations would already work,
though, and it seems worthwhile to figure out if we can change the
static_assert() for cases like this where the objects themselves don't live on
the heap.

I can commit to continue investigation, but would you be OK doing that
asynchronously? There's already a bunch of stuff depending on this CL.

[haraken, 2017-03-24 02:06:42]

On 2017/03/23 19:50:01, Peter Beverloo wrote:
> On 2017/03/23 07:22:19, dcheng wrote:
> > Ah... can we just define the typemap in the module then? Even though the
mojom
> > is defined elsewhere, it seems like that ought to work.
> 
> No, due to garbage collection. I uploaded a CL to try this here:
>   https://codereview.chromium.org/2772893002/
> 
> Using a typemap for non-GCed objects and enumerations would already work,
> though, and it seems worthwhile to figure out if we can change the
> static_assert() for cases like this where the objects themselves don't live on
> the heap.
> 
> I can commit to continue investigation, but would you be OK doing that
> asynchronously? There's already a bunch of stuff depending on this CL.

FWIW, we're hitting a similar issue in Image Capture:
https://bugs.chromium.org/p/chromium/issues/detail?id=700607#c4

i.e., we don't have a way to use a typemap for garbage collected objects.

[dcheng, 2017-03-24 05:06:02]

On 2017/03/23 19:50:01, Peter Beverloo wrote:
> On 2017/03/23 07:22:19, dcheng wrote:
> > Ah... can we just define the typemap in the module then? Even though the
mojom
> > is defined elsewhere, it seems like that ought to work.
> 
> No, due to garbage collection. I uploaded a CL to try this here:
>   https://codereview.chromium.org/2772893002/
> 
> Using a typemap for non-GCed objects and enumerations would already work,
> though, and it seems worthwhile to figure out if we can change the
> static_assert() for cases like this where the objects themselves don't live on
> the heap.
> 
> I can commit to continue investigation, but would you be OK doing that
> asynchronously? There's already a bunch of stuff depending on this CL.

Yeah that's fine. Thank you for looking into it. mojo lgtm

[Peter Beverloo, 2017-03-24 12:18:53]

On 2017/03/24 05:06:02, dcheng wrote:
> On 2017/03/23 19:50:01, Peter Beverloo wrote:
> > On 2017/03/23 07:22:19, dcheng wrote:
> > > Ah... can we just define the typemap in the module then? Even though the
> mojom
> > > is defined elsewhere, it seems like that ought to work.
> > 
> > No, due to garbage collection. I uploaded a CL to try this here:
> >   https://codereview.chromium.org/2772893002/
> > 
> > Using a typemap for non-GCed objects and enumerations would already work,
> > though, and it seems worthwhile to figure out if we can change the
> > static_assert() for cases like this where the objects themselves don't live
on
> > the heap.
> > 
> > I can commit to continue investigation, but would you be OK doing that
> > asynchronously? There's already a bunch of stuff depending on this CL.
> 
> Yeah that's fine. Thank you for looking into it. mojo lgtm

Thanks both! Let's follow up on these issues.

[Peter Beverloo, 2017-03-24 12:19:00]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-03-24 12:19:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
harkness@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2762573003/#ps80001
(title: "First round of comments")

[commit-bot: I haz the power, 2017-03-24 12:19:11]

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-24 13:59:58]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490357940054000,
"parent_rev": "283922b9417adb8918f1176237b3c33bff4dd6b4", "commit_rev":
"a1ab5a9bd84f0a1cf5ebbd322124152bb4cd1323"}

[commit-bot: I haz the power, 2017-03-24 14:00:56]

Description was changed from

==========
Implement BackgroundFetchManager.fetch() and struct traits

This implements the BackgroundFetchManager.fetch() method and sends it
to the browser process over IPC. Struct traits have been added for three
C++ types that are analogous to the Web exposed types:

  - BackgroundFetchOptions
    The options provided by the developer when starting the fetch.

  - BackgroundFetchRegistration
    The data associated with an in-progress background fetch.

  - IconDefinition
    The data associated with an individual icon definition. This will
    eventually move to a more generalized location.

What's still missing in the call is the sequence of Request objects.

BUG=692534, 692535
==========

to

==========
Implement BackgroundFetchManager.fetch() and struct traits

This implements the BackgroundFetchManager.fetch() method and sends it
to the browser process over IPC. Struct traits have been added for three
C++ types that are analogous to the Web exposed types:

  - BackgroundFetchOptions
    The options provided by the developer when starting the fetch.

  - BackgroundFetchRegistration
    The data associated with an in-progress background fetch.

  - IconDefinition
    The data associated with an individual icon definition. This will
    eventually move to a more generalized location.

What's still missing in the call is the sequence of Request objects.

BUG=692534, 692535

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

[commit-bot: I haz the power, 2017-03-24 14:00:58]

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

[dcheng, 2017-03-24 23:12:24]

(I meant to publish this draft, but I guess I didn't hit the right button.)

https://codereview.chromium.org/2762573003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchRegistration.cpp
(right):

https://codereview.chromium.org/2762573003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchRegistration.cpp:28:
m_registration = registration;
Maybe DCHECK(!m_registration) first as well.

[Peter Beverloo, 2017-03-24 23:26:05]

https://codereview.chromium.org/2762573003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchRegistration.cpp
(right):

https://codereview.chromium.org/2762573003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchRegistration.cpp:28:
m_registration = registration;
On 2017/03/24 23:12:24, dcheng wrote:
> Maybe DCHECK(!m_registration) first as well.

Sure, I'll send you a follow-up either tomorrow or after the weekend.