[Background Sync] Sent sync registration details to worker

[Background Sync] Sent sync registration details to worker

This sends the details of a fired sync event from the BackgroundSyncManager down
to the running service worker.

This is part 2 of a three-part blink-chromium-blink dance:
Part 1: https://codereview.chromium.org/1220223003/
Part 2: [This one]
Part 3: https://codereview.chromium.org/1231743004/

BUG=482066
Committed: https://crrev.com/022985a303ea89b8aae001e86ea6855c25b1ab11
Cr-Commit-Position: refs/heads/master@{#339882}

[iclelland, 2015-07-17 17:51:52]

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

[iclelland, 2015-07-17 17:51:52]

jkarlin - PTAL; note this still depends on the Mojo patch at
https://codereview.chromium.org/1220943003/, so it's not ready for commit yet.

[jkarlin, 2015-07-17 18:07:22]

Seems good, one question

https://codereview.chromium.org/1230213004/diff/40001/content/browser/backgro...
File content/browser/background_sync/background_sync_manager.h (right):

https://codereview.chromium.org/1230213004/diff/40001/content/browser/backgro...
content/browser/background_sync/background_sync_manager.h:123:
BackgroundSyncRegistration* registration,
const BackgroundSyncRegistration& registration

https://codereview.chromium.org/1230213004/diff/40001/content/browser/backgro...
File content/browser/background_sync/background_sync_registration.h (right):

https://codereview.chromium.org/1230213004/diff/40001/content/browser/backgro...
content/browser/background_sync/background_sync_registration.h:45: namespace
mojo {
Not sure if it's kosher to have multiple namespaces like this. Are there other
examples?

[iclelland, 2015-07-17 18:34:25]

https://codereview.chromium.org/1230213004/diff/40001/content/browser/backgro...
File content/browser/background_sync/background_sync_manager.h (right):

https://codereview.chromium.org/1230213004/diff/40001/content/browser/backgro...
content/browser/background_sync/background_sync_manager.h:123:
BackgroundSyncRegistration* registration,
On 2015/07/17 18:07:22, jkarlin wrote:
> const BackgroundSyncRegistration& registration

Done.

https://codereview.chromium.org/1230213004/diff/40001/content/browser/backgro...
File content/browser/background_sync/background_sync_registration.h (right):

https://codereview.chromium.org/1230213004/diff/40001/content/browser/backgro...
content/browser/background_sync/background_sync_registration.h:45: namespace
mojo {
On 2015/07/17 18:07:22, jkarlin wrote:
> Not sure if it's kosher to have multiple namespaces like this. Are there other
> examples?

The style guide is mostly silent on the issue, and it'll take more than a regex
to find an example, if there is one... or I could try to find the kosher police
around here.

[iclelland, 2015-07-17 18:56:35]

https://codereview.chromium.org/1230213004/diff/40001/content/browser/backgro...
File content/browser/background_sync/background_sync_registration.h (right):

https://codereview.chromium.org/1230213004/diff/40001/content/browser/backgro...
content/browser/background_sync/background_sync_registration.h:45: namespace
mojo {
On 2015/07/17 18:34:25, iclelland wrote:
> On 2015/07/17 18:07:22, jkarlin wrote:
> > Not sure if it's kosher to have multiple namespaces like this. Are there
other
> > examples?
> 
> The style guide is mostly silent on the issue, and it'll take more than a
regex
> to find an example, if there is one... or I could try to find the kosher
police
> around here.

So I found these examples of this exact thing:
 
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
 
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
 
https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi...
 
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

I thought that chrome/browser/ui/webui/omnibox/omnibox_ui_handler.* did the same
thing, but the OmniboxUIHandler doesn't seem to live in *any* namespace, so
that's something else entirely :)

[jkarlin, 2015-07-17 20:28:10]

lgtm with a couple of comments

https://codereview.chromium.org/1230213004/diff/60001/content/browser/backgro...
File content/browser/background_sync/background_sync_registration.h (right):

https://codereview.chromium.org/1230213004/diff/60001/content/browser/backgro...
content/browser/background_sync/background_sync_registration.h:52: const
content::SyncRegistrationPtr& input);
Why const ref input?

https://codereview.chromium.org/1230213004/diff/60001/content/renderer/backgr...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1230213004/diff/60001/content/renderer/backgr...
content/renderer/background_sync/background_sync_client_impl.cc:34:
mojo::ConvertTo<scoped_ptr<blink::WebSyncRegistration>>(registration);
I think ConvertTo could return a blink::WebSyncRegistration without loss of
performance.

[jkarlin, 2015-07-17 20:29:19]

https://codereview.chromium.org/1230213004/diff/40001/content/browser/backgro...
File content/browser/background_sync/background_sync_registration.h (right):

https://codereview.chromium.org/1230213004/diff/40001/content/browser/backgro...
content/browser/background_sync/background_sync_registration.h:45: namespace
mojo {
On 2015/07/17 18:56:35, iclelland wrote:
> On 2015/07/17 18:34:25, iclelland wrote:
> > On 2015/07/17 18:07:22, jkarlin wrote:
> > > Not sure if it's kosher to have multiple namespaces like this. Are there
> other
> > > examples?
> > 
> > The style guide is mostly silent on the issue, and it'll take more than a
> regex
> > to find an example, if there is one... or I could try to find the kosher
> police
> > around here.
> 
> So I found these examples of this exact thing:
>  
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
>  
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
>  
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi...
>  
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> 
> I thought that chrome/browser/ui/webui/omnibox/omnibox_ui_handler.* did the
same
> thing, but the OmniboxUIHandler doesn't seem to live in *any* namespace, so
> that's something else entirely :)

Sounds good, thanks for looking it up!

[iclelland, 2015-07-20 18:40:23]

https://codereview.chromium.org/1230213004/diff/60001/content/browser/backgro...
File content/browser/background_sync/background_sync_registration.h (right):

https://codereview.chromium.org/1230213004/diff/60001/content/browser/backgro...
content/browser/background_sync/background_sync_registration.h:52: const
content::SyncRegistrationPtr& input);
On 2015/07/17 20:28:10, jkarlin wrote:
> Why const ref input?

No good reason at all. Thanks for seeing that.

https://codereview.chromium.org/1230213004/diff/60001/content/renderer/backgr...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1230213004/diff/60001/content/renderer/backgr...
content/renderer/background_sync/background_sync_client_impl.cc:34:
mojo::ConvertTo<scoped_ptr<blink::WebSyncRegistration>>(registration);
On 2015/07/17 20:28:10, jkarlin wrote:
> I think ConvertTo could return a blink::WebSyncRegistration without loss of
> performance.

The type converter is used in other places; is it worth chasing them all down
for this CL?

[jkarlin, 2015-07-20 18:46:25]

https://codereview.chromium.org/1230213004/diff/60001/content/renderer/backgr...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1230213004/diff/60001/content/renderer/backgr...
content/renderer/background_sync/background_sync_client_impl.cc:34:
mojo::ConvertTo<scoped_ptr<blink::WebSyncRegistration>>(registration);
On 2015/07/20 18:40:22, iclelland wrote:
> On 2015/07/17 20:28:10, jkarlin wrote:
> > I think ConvertTo could return a blink::WebSyncRegistration without loss of
> > performance.
> 
> The type converter is used in other places; is it worth chasing them all down
> for this CL?

Hmm, just the provider and the unit tests. Seems reasonably small. But you can
TODO it for another CL if you prefer.

[iclelland, 2015-07-20 18:57:02]

On 2015/07/20 18:46:25, jkarlin wrote:
>
https://codereview.chromium.org/1230213004/diff/60001/content/renderer/backgr...
> File content/renderer/background_sync/background_sync_client_impl.cc (right):
> 
>
https://codereview.chromium.org/1230213004/diff/60001/content/renderer/backgr...
> content/renderer/background_sync/background_sync_client_impl.cc:34:
> mojo::ConvertTo<scoped_ptr<blink::WebSyncRegistration>>(registration);
> On 2015/07/20 18:40:22, iclelland wrote:
> > On 2015/07/17 20:28:10, jkarlin wrote:
> > > I think ConvertTo could return a blink::WebSyncRegistration without loss
of
> > > performance.
> > 
> > The type converter is used in other places; is it worth chasing them all
down
> > for this CL?
> 
> Hmm, just the provider and the unit tests. Seems reasonably small. But you can
> TODO it for another CL if you prefer.

Fair enough; I suppose I just thought it was out of scope. I'll go and improve
the codebase, as a side effect of this CL :)

[iclelland, 2015-07-21 18:39:46]

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

[iclelland, 2015-07-21 18:40:58]

+r michaeln -- could you PTAL at content/{browser,renderer}/service_worker/* --
thanks!

[michaeln, 2015-07-21 18:46:16]

(mostly r/s) lgtm 2

[iclelland, 2015-07-22 15:48:09]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2015-07-22 15:48:09]

The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/1230213004/#ps80001
(title: "Remove const ref from mojo StructPtr (with rebase)")

[commit-bot: I haz the power, 2015-07-22 15:48:29]

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

[commit-bot: I haz the power, 2015-07-22 15:54:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-22 15:54:48]

Patchset 5 (id:??) landed as
https://crrev.com/022985a303ea89b8aae001e86ea6855c25b1ab11
Cr-Commit-Position: refs/heads/master@{#339882}