Implement a Mojo service for Background Fetch

Implement a Mojo service for Background Fetch

This CL lands the Mojo service that we intend to use for Background
Fetch. Everything but the fetch() event has been hooked up from the
renderer side, and will be received by a strongly binded service
implementation in the browser process.

The fetch() function will be implemented separately, as it will require
some plumbing to support the appropriate types for requests.

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

[Peter Beverloo, 2017-03-13 19:38:32]

Patchset #1 (id:1) has been deleted

[Peter Beverloo, 2017-03-13 19:39:00]

Patchset #1 (id:20001) has been deleted

[Peter Beverloo, 2017-03-13 19:39:06]

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

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

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-13 19:42:04]

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

[Peter Beverloo, 2017-03-13 19:42:05]

+harkness for everything
+haraken for Mojo integration in Blink.

    The BackgroundFetchBridge exists so that we can share
    a connection between different consumers (there's
    three entry points).

+tsepez for Mojo

    Tom, the BackgroundFetchServiceImpl is still very TODO
    heavy as we're developing the backend in parallel. Is
    it OK to iterate? There will be various follow-up CLs.

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

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

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

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

[Tom Sepez, 2017-03-13 21:43:19]

LGTM, please cc me on the iterations ...

[haraken, 2017-03-14 09:13:13]

haraken@chromium.org changed reviewers:
+ dcheng@chromium.org

[haraken, 2017-03-14 09:13:14]

I might want to have dcheng@ take a look at the Mojo integration part.

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

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:29:
icon.setType(iconPtr->type);

Can we probably use a mojo's type-mapping to avoid manually copying
IconDefinition?

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:124:
BackgroundFetchBridge::GetService() {

Nit: getService.

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.h
(right):

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.h:43:
// TODO(peter): Implement suppor for the `fetch()` function in the bridge.

support

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp
(right):

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp:113:
// Not applicable for this callback.

Who resolves or rejects in this case?

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchedEvent.cpp
(right):

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchedEvent.cpp:35:
// Return a Promise that will never settles when a developer calls this

settle

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchedEvent.cpp:59:
// Not applicable for this callback.

Ditto. Who resolves or rejects in this case?

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

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/background_fetch/background_fetch.mojom:26:
// TODO(peter): Implement suppor for the `fetch()` function in Mojo.

support

[harkness, 2017-03-14 11:55:48]

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

https://codereview.chromium.org/2745243002/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_service_impl.h:20: // and
callers from Blink, which implements the Background Fetch JavaScript API.
nit: and calls? or just remove? Not sure what this is supposed to say. Also,
this could be called from other places besides Blink, do we want to say that or
only list Blink because that's the only current user?

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.h
(right):

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.h:47:
// has been updated.
Can we give a weaker time guarantee for this? Instead just say that callback is
invoked when the UI has been told to update and when queries of the UI would
return the updated title?

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

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/background_fetch/background_fetch.mojom:9:
DUPLICATED_TAG
Why not add in NOT_YET_IMPLEMENTED now? It would be appropriate for many of the
calls, and will still be appropriate in the future when there are calls in v2
that are not implemented in v1.

Also, I'm not sure what your intention for DUPLICATED_TAG is. It makes sense for
fetch(), but not for any of the other methods. For Abort/GetRegistration, it
seems like NO_TAG (INVALID_TAG? but that feels like a syntax issues) would be a
better error.

[Peter Beverloo, 2017-03-14 15:15:43]

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

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

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-14 15:17:22]

Tom: Certainly!
Daniel: Mind having a look too? Would love your thoughts on how to improve
further :).

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

https://codereview.chromium.org/2745243002/diff/40001/content/browser/backgro...
content/browser/background_fetch/background_fetch_service_impl.h:20: // and
callers from Blink, which implements the Background Fetch JavaScript API.
On 2017/03/14 11:55:47, harkness wrote:
> nit: and calls? or just remove? Not sure what this is supposed to say. Also,
> this could be called from other places besides Blink, do we want to say that
or
> only list Blink because that's the only current user?

Fair enough. Realistically I'm not holding my breath for non-Blink users of this
API (there's likely a more appropriate API available to them), but Mojo does
enable this for sure.

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

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:124:
BackgroundFetchBridge::GetService() {
On 2017/03/14 09:13:14, haraken wrote:
> 
> Nit: getService.
> 

Done.

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.h
(right):

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.h:43:
// TODO(peter): Implement suppor for the `fetch()` function in the bridge.
On 2017/03/14 09:13:14, haraken wrote:
> 
> support

Done.

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.h:47:
// has been updated.
On 2017/03/14 11:55:47, harkness wrote:
> Can we give a weaker time guarantee for this? Instead just say that callback
is
> invoked when the UI has been told to update and when queries of the UI would
> return the updated title?

Done.

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp
(right):

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp:113:
// Not applicable for this callback.
On 2017/03/14 09:13:14, haraken wrote:
> 
> Who resolves or rejects in this case?

Nobody. This is a programming error, so we fall back to NOTREACHED, which is
DCHECK(0).

I considered having a separate error status enum per call, but there's very few
error codes I can think of right now and compile-time verification of the
switches makes it easy to verify that all states are covered.

Happy to have multiple enums if anyone has a preference for that.

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchedEvent.cpp
(right):

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchedEvent.cpp:35:
// Return a Promise that will never settles when a developer calls this
On 2017/03/14 09:13:14, haraken wrote:
> 
> settle

Done.

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchedEvent.cpp:59:
// Not applicable for this callback.
On 2017/03/14 09:13:14, haraken wrote:
> 
> Ditto. Who resolves or rejects in this case?

Acknowledged.

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

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/background_fetch/background_fetch.mojom:9:
DUPLICATED_TAG
On 2017/03/14 11:55:47, harkness wrote:
> Why not add in NOT_YET_IMPLEMENTED now? It would be appropriate for many of
the
> calls, and will still be appropriate in the future when there are calls in v2
> that are not implemented in v1.

We shouldn't expose any method that's not implemented yet, which is why the
status of the feature is "test": even the "Experimental Web Platform Features"
flag doesn't capture it yet.

This enum will likely be used for UMA too, so there's a cost to adding values
(we won't be able to remove them).

> Also, I'm not sure what your intention for DUPLICATED_TAG is. It makes sense
for
> fetch(), but not for any of the other methods. For Abort/GetRegistration, it
> seems like NO_TAG (INVALID_TAG? but that feels like a syntax issues) would be
a
> better error.

See my previous comment on generic vs. individual enums.

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/background_fetch/background_fetch.mojom:26:
// TODO(peter): Implement suppor for the `fetch()` function in Mojo.
On 2017/03/14 09:13:14, haraken wrote:
> 
> support

Done.

[commit-bot: I haz the power, 2017-03-14 16:49:26]

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

[commit-bot: I haz the power, 2017-03-14 16:49:26]

Dry run: This issue passed the CQ dry run.

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

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

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/background_fetch/background_fetch.mojom:9:
DUPLICATED_TAG
On 2017/03/14 15:17:21, Peter Beverloo wrote:
> On 2017/03/14 11:55:47, harkness wrote:
> > Why not add in NOT_YET_IMPLEMENTED now? It would be appropriate for many of
> the
> > calls, and will still be appropriate in the future when there are calls in
v2
> > that are not implemented in v1.
> 
> We shouldn't expose any method that's not implemented yet, which is why the
> status of the feature is "test": even the "Experimental Web Platform Features"
> flag doesn't capture it yet.
> 
> This enum will likely be used for UMA too, so there's a cost to adding values
> (we won't be able to remove them).
> 
> > Also, I'm not sure what your intention for DUPLICATED_TAG is. It makes sense
> for
> > fetch(), but not for any of the other methods. For Abort/GetRegistration, it
> > seems like NO_TAG (INVALID_TAG? but that feels like a syntax issues) would
be
> a
> > better error.
> 
> See my previous comment on generic vs. individual enums.

re: Not yet implemented - ok

re: DUPLICATED_TAG

I don't have an issue with either the generic or the individual enum, I think
either approach is reasonable. 

I suppose my real issue here is that while DUPLICATED_TAG makes sense as a
return value for fetch(), it doesn't make any sense to me as an error code for
get() which is where you're currently returning it.

[Peter Beverloo, 2017-03-14 17:28:15]

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

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:29:
icon.setType(iconPtr->type);
On 2017/03/14 09:13:13, haraken wrote:
> 
> Can we probably use a mojo's type-mapping to avoid manually copying
> IconDefinition?

Since IconDefinition is a dictionary and not garbage collected, we can. However,
the mojom is defined in //public/platform/ whereas the IconDefinition file is in
//Source/modules/ - the former cannot access the latter. How do we do this from
a dependency point of view? (Daniel, you may have thoughts?)

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

https://codereview.chromium.org/2745243002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/background_fetch/background_fetch.mojom:9:
DUPLICATED_TAG
On 2017/03/14 17:17:29, harkness wrote:
> On 2017/03/14 15:17:21, Peter Beverloo wrote:
> > On 2017/03/14 11:55:47, harkness wrote:
> > > Why not add in NOT_YET_IMPLEMENTED now? It would be appropriate for many
of
> > the
> > > calls, and will still be appropriate in the future when there are calls in
> v2
> > > that are not implemented in v1.
> > 
> > We shouldn't expose any method that's not implemented yet, which is why the
> > status of the feature is "test": even the "Experimental Web Platform
Features"
> > flag doesn't capture it yet.
> > 
> > This enum will likely be used for UMA too, so there's a cost to adding
values
> > (we won't be able to remove them).
> > 
> > > Also, I'm not sure what your intention for DUPLICATED_TAG is. It makes
sense
> > for
> > > fetch(), but not for any of the other methods. For Abort/GetRegistration,
it
> > > seems like NO_TAG (INVALID_TAG? but that feels like a syntax issues) would
> be
> > a
> > > better error.
> > 
> > See my previous comment on generic vs. individual enums.
> 
> re: Not yet implemented - ok
> 
> re: DUPLICATED_TAG
> 
> I don't have an issue with either the generic or the individual enum, I think
> either approach is reasonable. 
> 
> I suppose my real issue here is that while DUPLICATED_TAG makes sense as a
> return value for fetch(), it doesn't make any sense to me as an error code for
> get() which is where you're currently returning it. 

Ahh got it. Done!

[Peter Beverloo, 2017-03-14 17:29:28]

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

[commit-bot: I haz the power, 2017-03-14 17:29:49]

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

[harkness, 2017-03-14 18:19:36]

lgtm % not resolving on DUPLICATED_TAG.

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

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp:79:
resolver->resolve(registration);
Now that getRegistration shouldn't return DUPLICATED_TAG, I think it should not
result in resolving the promise.  (It shouldn't be applicable to
didGetRegistration, just like for didGetTags.)

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

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

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

Dry run: This issue passed the CQ dry run.

[haraken, 2017-03-14 20:04:02]

LGTM if dcheng@ is happy.

[Peter Beverloo, 2017-03-15 17:24:29]

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

[Peter Beverloo, 2017-03-15 17:24:30]

+avi for //content/browser/{r,s}
+dcheng, mind taking a look?

[Avi (use Gerrit), 2017-03-15 17:31:07]

I don't know how mojo dependencies work, but if that's ok lgtm

[dcheng, 2017-03-16 07:14:08]

LGTM, sorry for the review delay.

The Blink mojo integration seems fine, given the level of support we have today.
I have some questions about the typemapping (I'm hoping we can improve this,
though that is out of the scope of this CL), and I have some random other
comments/questions for small things I noticed.

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

https://codereview.chromium.org/2745243002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_service_impl.h:22:
BackgroundFetchContext* background_fetch_context,
Nit: these are refcounted and this class takes a reference, so we should pass as
scoped_refptr. For bonus points, pass by value and std::move() where appropriate
to avoid refcount churn.

https://codereview.chromium.org/2745243002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2745243002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1323:
base::Unretained(storage_partition_impl_->GetBackgroundFetchContext()),
Which will probably require binding these as scoped_refptr types.

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

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:25:
for (const auto& iconPtr : registrationPtr->icons) {
Is this something that would make sense to typemap in the future? I'm not sure
how feasible it is to typemap Mojo generated structs to IDL generated types...
but it does seem like something we should figure out at some point (there's a
lot of boilerplate code like this for interacting with mojo already)

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:35:
serviceWorkerRegistration, registrationPtr->tag, std::move(icons),
Nit: #include <utility> for std::move

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:107:
// Fetch registration, which can't be a typemap as it's garbage collected.
Can you help me understand why Oilpan heap objects can't be typemapped?

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:116:
const mojom::blink::BackgroundFetchServicePtr&
It seems unusual to mark this const: it kind of makes sense from a logical
perspective, since operator-> is marked const... but it feels weird, as the
methods that are invoked feel very much non-const.

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:118:
if (!m_backgroundFetchService.get()) {
Nit: no .get()

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

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchedEvent.cpp:56:
resolver->resolve();
Nit: this still ends up in the NOTREACHED(), should there be a return after
this?

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

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/background_fetch/background_fetch.mojom:22:
string title = "";
Just curious, but how does default initialization work? I would have imagined
this is the default state, but I'm not sure.

[Peter Beverloo, 2017-03-16 15:32:56]

Thanks! Avi: DEPS to Mojo services implemented in //content/browser/ are fine
indeed.

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

https://codereview.chromium.org/2745243002/diff/80001/content/browser/backgro...
content/browser/background_fetch/background_fetch_service_impl.h:22:
BackgroundFetchContext* background_fetch_context,
On 2017/03/16 07:14:07, dcheng wrote:
> Nit: these are refcounted and this class takes a reference, so we should pass
as
> scoped_refptr. For bonus points, pass by value and std::move() where
appropriate
> to avoid refcount churn.

Bonus points acquired!

https://codereview.chromium.org/2745243002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2745243002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1323:
base::Unretained(storage_partition_impl_->GetBackgroundFetchContext()),
On 2017/03/16 07:14:07, dcheng wrote:
> Which will probably require binding these as scoped_refptr types.

Done.

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

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:25:
for (const auto& iconPtr : registrationPtr->icons) {
On 2017/03/16 07:14:08, dcheng wrote:
> Is this something that would make sense to typemap in the future? I'm not sure
> how feasible it is to typemap Mojo generated structs to IDL generated types...
> but it does seem like something we should figure out at some point (there's a
> lot of boilerplate code like this for interacting with mojo already)

There's two questions here -

  (1) Mojo structs to IDL dictionaries (like this)
  (2) Mojo structs to IDL interfaces (like BackgroundFetchRegistration)

It would definitely be nice to enable (1). The current issue is that the Mojo
type lives in //public/platform/ whereas the Blink type that we'd want it to use
lives in //Source/modules/, which is not an allowed dependency. We could work
around that by having the TypeConverter live in this module and doing the
conversion manually, but in this case I'm not (yet) convinced it's worth the
overhead.

I can't really comment on (2) from a garbage collection perspective, but many
interfaces do need additional information in order to work correctly. (E.g. the
ServiceWorkerRegistration in this case.) There's probably not as much to gain
there.

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:35:
serviceWorkerRegistration, registrationPtr->tag, std::move(icons),
On 2017/03/16 07:14:08, dcheng wrote:
> Nit: #include <utility> for std::move

Done.

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:107:
// Fetch registration, which can't be a typemap as it's garbage collected.
On 2017/03/16 07:14:08, dcheng wrote:
> Can you help me understand why Oilpan heap objects can't be typemapped?

This actually is a left-over comment -- the first iteration of the Mojo service
I played with used an optional type through WTF::Optional that asserts the
contained type isn't garbage collected. In retrospect that makes no sense for
where I ended up. Sorry for leaving this in -- amended.

Left some thoughts about type mapping above.

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:116:
const mojom::blink::BackgroundFetchServicePtr&
On 2017/03/16 07:14:08, dcheng wrote:
> It seems unusual to mark this const: it kind of makes sense from a logical
> perspective, since operator-> is marked const... but it feels weird, as the
> methods that are invoked feel very much non-const.

Done.

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp:118:
if (!m_backgroundFetchService.get()) {
On 2017/03/16 07:14:08, dcheng wrote:
> Nit: no .get()

Done.

[Peter Beverloo, 2017-03-16 15:33:03]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-03-16 15:33:03]

The CQ bit was unchecked by peter@chromium.org

[Peter Beverloo, 2017-03-16 15:33:04]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
haraken@chromium.org, harkness@chromium.org, avi@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2745243002/#ps120001
(title: "add missing underscores")

[commit-bot: I haz the power, 2017-03-16 15:33:37]

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

[Peter Beverloo, 2017-03-16 16:22:53]

The CQ bit was unchecked by peter@chromium.org

[Peter Beverloo, 2017-03-16 16:26:55]

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

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp:79:
resolver->resolve(registration);
On 2017/03/14 18:19:35, harkness wrote:
> Now that getRegistration shouldn't return DUPLICATED_TAG, I think it should
not
> result in resolving the promise.  (It shouldn't be applicable to
> didGetRegistration, just like for didGetTags.)

Done.

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

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchedEvent.cpp:56:
resolver->resolve();
On 2017/03/16 07:14:08, dcheng wrote:
> Nit: this still ends up in the NOTREACHED(), should there be a return after
> this?

Done.

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

https://codereview.chromium.org/2745243002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/background_fetch/background_fetch.mojom:22:
string title = "";
On 2017/03/16 07:14:08, dcheng wrote:
> Just curious, but how does default initialization work? I would have imagined
> this is the default state, but I'm not sure.

I'm not sure, I did the copy-from-prior-art thing. At the very least it's
semantical in creating a distinguishment between required and optional w/
default value.

Looking at the generated code, it does consider what's being defined here.

[Peter Beverloo, 2017-03-16 16:27:22]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-03-16 16:27:22]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
haraken@chromium.org, harkness@chromium.org, avi@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2745243002/#ps140001
(title: "rebase + comments v2")

[commit-bot: I haz the power, 2017-03-16 16:28:00]

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-16 18:06:32]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489681642254820,
"parent_rev": "d2ff1ae2c1175126133ec36bc400b3aaed08092b", "commit_rev":
"3226ce85f3ba466797db44fe134abb8b898d2452"}

[commit-bot: I haz the power, 2017-03-16 18:07:12]

Description was changed from

==========
Implement a Mojo service for Background Fetch

This CL lands the Mojo service that we intend to use for Background
Fetch. Everything but the fetch() event has been hooked up from the
renderer side, and will be received by a strongly binded service
implementation in the browser process.

The fetch() function will be implemented separately, as it will require
some plumbing to support the appropriate types for requests.

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

to

==========
Implement a Mojo service for Background Fetch

This CL lands the Mojo service that we intend to use for Background
Fetch. Everything but the fetch() event has been hooked up from the
renderer side, and will be received by a strongly binded service
implementation in the browser process.

The fetch() function will be implemented separately, as it will require
some plumbing to support the appropriate types for requests.

BUG=692534, 692535

Review-Url: https://codereview.chromium.org/2745243002
Cr-Commit-Position: refs/heads/master@{#457490}
Committed:
https://chromium.googlesource.com/chromium/src/+/3226ce85f3ba466797db44fe134a...
==========

[commit-bot: I haz the power, 2017-03-16 18:07:14]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/3226ce85f3ba466797db44fe134a...