[Background Sync] Use Mojo IPC to fire background sync events

[Background Sync] Use Mojo IPC to fire background sync events

This patch adds a background sync client service to the renderer process, which
accepts connections from the browser, in order to trigger Sync and PeriodicSync
events in the service worker.

At the browser level, the Sync and PeriodicSync events are identical, so all of
the code in this patch deals only with generic "Sync" events. (The code paths
are the same, and the registration structs differ only in the periodicity
value.) They only diverge in the renderer, once we determine which event in the
service worker script proxy to fire.

BUG=498388
Committed: https://crrev.com/77d52b2ac6ec76c56fc487c14dc405a083a23a28
Cr-Commit-Position: refs/heads/master@{#339874}

[iclelland, 2015-07-03 13:15:27]

iclelland@chromium.org changed reviewers:
+ jkarlin@chromium.org, mek@chromium.org

[iclelland, 2015-07-03 13:17:12]

This CL has the same functionality as https://codereview.chromium.org/1171173002
but has been updated to depend on mek's proposal in
https://codereview.chromium.org/1221503003 to exchange mojo service registries
between browser and embedded worker threads.

[iclelland, 2015-07-17 16:03:34]

iclelland@chromium.org changed reviewers:
+ michaeln@chromium.org

[iclelland, 2015-07-17 16:03:34]

I've updated this now that https://codereview.chromium.org/1221503003 has
landed. It's still essentially a rework of
https://codereview.chromium.org/1171173002, but using the new mojo service
registry in the Worker thread to dispatch sync events.

[Marijn Kruisselbrink, 2015-07-17 17:12:15]

https://codereview.chromium.org/1220943003/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1220943003/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:810: //
https://codereview.chromium.org/1210643002 lands.
In the last version of that CL I actually got rid of the template magic for the
mojo error handler and changed it to just have the actual code. I'm not
convinced the earlier template magic abstracted things at the right level, and I
think it made the code less clear.

https://codereview.chromium.org/1220943003/diff/80001/content/common/service_...
File content/common/service_worker/service_worker_type_converters.cc (right):

https://codereview.chromium.org/1220943003/diff/80001/content/common/service_...
content/common/service_worker/service_worker_type_converters.cc:17:
content::ServiceWorkerStatusCode status_code = content::SERVICE_WORKER_OK;
Should the default really be SERVICE_WORKER_OK? It seems a bit odd to treat
invalid data from the renderer as OK (if/when mojo starts validating enums this
of course doesn't matter anymore, but for now it's perfectly possible to receive
a status that is not a valid enum member)

[iclelland, 2015-07-20 16:01:24]

iclelland@chromium.org changed reviewers:
+ jochen@chromium.org, tsepez@chromium.org

[iclelland, 2015-07-20 16:06:16]

tsepez -- PTAL at the mojom files -- this code is a rework of
https://codereview.chromium.org/1171173002, and the mojom is almost identical.

jochen -- PTAL at the .gyp/GN/OWNERS files

michaeln for service worker stuff
jkarlin for background sync

Thanks, everyone!

https://codereview.chromium.org/1220943003/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1220943003/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:810: //
https://codereview.chromium.org/1210643002 lands.
On 2015/07/17 17:12:15, Marijn Kruisselbrink wrote:
> In the last version of that CL I actually got rid of the template magic for
the
> mojo error handler and changed it to just have the actual code. I'm not
> convinced the earlier template magic abstracted things at the right level, and
I
> think it made the code less clear.

Cool, thanks for the heads-up. I've updated this to match.

Agreed that it was a bit of magic. I think we'll end up moving back to something
like that eventually, once we have more of these in place. Let's get to that
problem first, though.

Done.

https://codereview.chromium.org/1220943003/diff/80001/content/common/service_...
File content/common/service_worker/service_worker_type_converters.cc (right):

https://codereview.chromium.org/1220943003/diff/80001/content/common/service_...
content/common/service_worker/service_worker_type_converters.cc:17:
content::ServiceWorkerStatusCode status_code = content::SERVICE_WORKER_OK;
On 2015/07/17 17:12:15, Marijn Kruisselbrink wrote:
> Should the default really be SERVICE_WORKER_OK? It seems a bit odd to treat
> invalid data from the renderer as OK (if/when mojo starts validating enums
this
> of course doesn't matter anymore, but for now it's perfectly possible to
receive
> a status that is not a valid enum member)

Hence the NOTREACHED() further down -- the value here was really here to stop
the compiler from complaining about a possibly uninitialized value.

I had thought that NOTREACHED would save us, but I wasn't thinking that it was
just a DCHECK, so we're still in danger in a release build.

I'll switch the bad-enum-default to SERVICE_WORKER_ERROR_IPC_FAILED, which will
be interpreted as failure in BackgroundSyncManager::EventCompleteImpl. Thanks.

Done.

[Marijn Kruisselbrink, 2015-07-20 16:16:19]

lgtm

https://codereview.chromium.org/1220943003/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1220943003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:621:
BackgroundSyncServiceClientPtr background_sync_dispatcher_;
Maybe keep the declaration of the various mojo services together? I don't
particularly care where they appear, but it might make sense to just keep them
together.

[jkarlin, 2015-07-20 17:17:26]

content/renderer/background_sync lgtm

https://codereview.chromium.org/1220943003/diff/120001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.h (right):

https://codereview.chromium.org/1220943003/diff/120001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.h:8: #include
"base/memory/ref_counted.h"
needed?

https://codereview.chromium.org/1220943003/diff/120001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.h:28: //
BackgroundSyncClient methods:
BackgruondSyncServiceClient

[iclelland, 2015-07-20 18:05:21]

https://codereview.chromium.org/1220943003/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1220943003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:621:
BackgroundSyncServiceClientPtr background_sync_dispatcher_;
On 2015/07/20 16:16:19, Marijn Kruisselbrink wrote:
> Maybe keep the declaration of the various mojo services together? I don't
> particularly care where they appear, but it might make sense to just keep them
> together.

Definitely. Done.

https://codereview.chromium.org/1220943003/diff/120001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.h (right):

https://codereview.chromium.org/1220943003/diff/120001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.h:8: #include
"base/memory/ref_counted.h"
On 2015/07/20 17:17:26, jkarlin wrote:
> needed?

It provided two macros: NON_EXPORTED_BASE and DISALLOW_COPY_AND_ASSIGN. I've
replaced it with the direct includes.
Done.

https://codereview.chromium.org/1220943003/diff/120001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.h:28: //
BackgroundSyncClient methods:
On 2015/07/20 17:17:25, jkarlin wrote:
> BackgruondSyncServiceClient

I spent 5 minutes looking for the typo, before I saw the real difference :)
Done.

[Tom Sepez, 2015-07-21 00:01:08]

mojom lgtm

[michaeln, 2015-07-21 00:21:16]

lgtm 2

[iclelland, 2015-07-22 02:09:34]

jochen -- friendly bump ;) Thanks

[jochen (gone - plz use gerrit), 2015-07-22 09:41:24]

lgtm

[iclelland, 2015-07-22 13:17:01]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2015-07-22 13:17:01]

The patchset sent to the CQ was uploaded after l-g-t-m from mek@chromium.org,
jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/1220943003/#ps140001
(title: "Addressing post-lgtm review comments")

[commit-bot: I haz the power, 2015-07-22 13:17:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1220943003/140001

[commit-bot: I haz the power, 2015-07-22 14:16:07]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-07-22 14:17:01]

Patchset 8 (id:??) landed as
https://crrev.com/77d52b2ac6ec76c56fc487c14dc405a083a23a28
Cr-Commit-Position: refs/heads/master@{#339874}

[jam, 2016-01-26 22:53:04]

I just saw this.

why is 	content/public/common/service_worker_event_status.mojom in
content/public/common instead of content/common? since it's not used by anyone
outside content, it should be in content/common.

please move it, per
https://www.chromium.org/developers/content-module/content-api

[iclelland, 2016-01-26 22:57:58]

Thanks John, I don't recall exactly why it ended up there, but you're
right, it doesn't belong.

We're doing a bunch of refactoring of the code this quarter, so I'll make
sure that gets moved.

On Tue, Jan 26, 2016 at 5:53 PM, <jam@chromium.org> wrote:

> I just saw this.
>
> why is  content/public/common/service_worker_event_status.mojom in
> content/public/common instead of content/common? since it's not used by
> anyone
> outside content, it should be in content/common.
>
> please move it, per
> https://www.chromium.org/developers/content-module/content-api
>
> https://codereview.chromium.org/1220943003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.