Chrome side of passing on the service worker registration with geofencing API calls.

Chrome side of passing on the service worker registration with geofencing API calls.

BUG=383125
Committed: https://crrev.com/a38764be36077683d9fd7e5190fbae0ce8733d4c
Cr-Commit-Position: refs/heads/master@{#301010}

[jsbell, 2014-10-08 18:41:29]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2014-10-08 18:41:30]

just a few drive-by nits

https://codereview.chromium.org/623823002/diff/40001/content/browser/geofenci...
File content/browser/geofencing/geofencing_dispatcher_host.cc (right):

https://codereview.chromium.org/623823002/diff/40001/content/browser/geofenci...
content/browser/geofencing/geofencing_dispatcher_host.cc:122: service_worker ?
service_worker->id() : -1;
Can you make this an enum value in the GeofencingManager rather than a magic
number?

(I see it's documented in a comment in
https://codereview.chromium.org/586163003/)

https://codereview.chromium.org/623823002/diff/40001/content/child/geofencing...
File content/child/geofencing/geofencing_dispatcher.cc (right):

https://codereview.chromium.org/623823002/diff/40001/content/child/geofencing...
content/child/geofencing/geofencing_dispatcher.cc:89: if (serviceworker)
nit: need braces around multi-line if body

https://codereview.chromium.org/623823002/diff/40001/content/child/geofencing...
content/child/geofencing/geofencing_dispatcher.cc:102: if (serviceworker)
nit: need braces around multi-line if body

[Michael van Ouwerkerk, 2014-10-09 12:28:09]

mvanouwerkerk@chromium.org changed reviewers:
+ michaeln@chromium.org, mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2014-10-09 12:28:10]

Adding michaeln as reviewer, he was very helpful when I did the push
integration.

https://codereview.chromium.org/623823002/diff/40001/content/browser/geofenci...
File content/browser/geofencing/geofencing_dispatcher_host.cc (right):

https://codereview.chromium.org/623823002/diff/40001/content/browser/geofenci...
content/browser/geofencing/geofencing_dispatcher_host.cc:57:
service_worker_filter_->GetRegistrationHandle(
I think GetRegistration(Handle) should be exposed on ServiceWorkerContextCore or
we should go directly to ServiceWorkerStorage.

https://codereview.chromium.org/623823002/diff/40001/content/browser/geofenci...
content/browser/geofencing/geofencing_dispatcher_host.cc:59:
ServiceWorkerRegistration* service_worker =
Let's be clear and call the variable service_worker_registration.

https://codereview.chromium.org/623823002/diff/40001/content/browser/geofenci...
File content/browser/geofencing/geofencing_dispatcher_host.h (right):

https://codereview.chromium.org/623823002/diff/40001/content/browser/geofenci...
content/browser/geofencing/geofencing_dispatcher_host.h:53:
scoped_refptr<ServiceWorkerDispatcherHost> service_worker_filter_;
It seems strange for one filter to have a pointer to another filter. Is there no
better API to interact with the service worker system?

https://codereview.chromium.org/623823002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/623823002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:897: new
GeofencingDispatcherHost(browser_context_, service_worker_filter));
This should pass in storage_partition_impl_->GetServiceWorkerContext() instead
of the filter.

https://codereview.chromium.org/623823002/diff/40001/content/child/geofencing...
File content/child/geofencing/geofencing_dispatcher.h (right):

https://codereview.chromium.org/623823002/diff/40001/content/child/geofencing...
content/child/geofencing/geofencing_dispatcher.h:38:
blink::WebServiceWorkerRegistration* serviceworker,
Let's call it service_worker_registration, here and below.

[Marijn Kruisselbrink, 2014-10-09 22:28:59]

(note that I didn't actually send this CL out for review yet, since I hadn't
looked it over yet/wasn't quite happy with how I had exposed things from the
service worker system; I merely had asked for feedback on how to better do this.
I'll address the nits/coding style comments once we've come up with an approach
we're all happy with).

https://codereview.chromium.org/623823002/diff/40001/content/browser/geofenci...
File content/browser/geofencing/geofencing_dispatcher_host.cc (right):

https://codereview.chromium.org/623823002/diff/40001/content/browser/geofenci...
content/browser/geofencing/geofencing_dispatcher_host.cc:57:
service_worker_filter_->GetRegistrationHandle(
On 2014/10/09 12:28:09, Michael van Ouwerkerk wrote:
> I think GetRegistration(Handle) should be exposed on ServiceWorkerContextCore
or
> we should go directly to ServiceWorkerStorage.
The problem is that handle IDs currently only exists in the
ServiceWorkerDispatcherHost. These IDs are per-renderer process, and
ServiceWorkerRegistrationHandle instances are tied to a specific renderer
process. But yes, I agree that it would be great to have some API that doesn't
require one message filter directly accessing another filter. One way to maybe
make this less icky would be for ServiceWorkerDispatcherHost to implement a new
"ServiceWorkerRenderHostContext" interface (or something with a better name),
which would expose this GetRegistrationHandle method to get to the handle from
the handle id for a specific renderer.

https://codereview.chromium.org/623823002/diff/40001/content/browser/geofenci...
File content/browser/geofencing/geofencing_dispatcher_host.h (right):

https://codereview.chromium.org/623823002/diff/40001/content/browser/geofenci...
content/browser/geofencing/geofencing_dispatcher_host.h:53:
scoped_refptr<ServiceWorkerDispatcherHost> service_worker_filter_;
On 2014/10/09 12:28:09, Michael van Ouwerkerk wrote:
> It seems strange for one filter to have a pointer to another filter. Is there
no
> better API to interact with the service worker system?
That's the question, yes.

https://codereview.chromium.org/623823002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/623823002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:897: new
GeofencingDispatcherHost(browser_context_, service_worker_filter));
On 2014/10/09 12:28:09, Michael van Ouwerkerk wrote:
> This should pass in storage_partition_impl_->GetServiceWorkerContext() instead
> of the filter.
That's not enough. To go from registration handle ID to RegistrationHandle (and
eventually to Registration) some renderer process specific state is necessary,
and currently that state only lives in ServiceWorkerDispatcherHost. So either
the IPC would have to use some other identifier, the mapping from handle id to
handle would have to live in a different object, or some other way of exposing
this mapping is needed.

[michaeln1, 2014-10-09 23:28:06]

I was just looking at what we did for push messaging, that's done somewhat
differently.

Can i ask some stupid questions about the API?

navigator.geolocation.
43	    [RuntimeEnabled=Geofencing, CallWith=ScriptState] Promise
registerRegion(GeofencingRegion region);
44	    [RuntimeEnabled=Geofencing, CallWith=ScriptState] Promise
unregisterRegion(DOMString regionId);
45	    [RuntimeEnabled=Geofencing, CallWith=ScriptState] Promise
getRegisteredRegions();

Where do the Entered/Exited events get raised?

Lets say a page loads, calls registerRegion, and then the tab is closed.
Sometime later the user strays into the region... wha happens? Suppose there was
no serviceworker for the page when the register call was made... whats different
for that case?

Let's say there's a page open at time the region is entered (or the page that
did the initial registration is still open), does anything different happen in
that (those) cases?

Are the methods above accessible in worker contexts too, if not today, will they
be at some point?

With the handleId stuff, you are tripping on code that probably needs to be
re-factored (read rewritten). Looks like you want registrationId + origin at the
end of the day, is that right?

[michaeln, 2014-10-09 23:33:24]

ah, i see the other cls too, those will probably answer some of my stupid
questions!

Chromium side of geofencing event dispatching.
https://codereview.chromium.org/629393002/

Blink side of exposing the service worker registration associated with
geofencing API calls.
https://codereview.chromium.org/623813002/

[Marijn Kruisselbrink, 2014-10-09 23:40:10]

On 2014/10/09 23:28:06, michaeln1 wrote:
> I was just looking at what we did for push messaging, that's done somewhat
> differently.
> 
> Can i ask some stupid questions about the API?
> 
> navigator.geolocation.
Exposing this on navigator is/was a temporary solution since the service worker
code isn't actually in a state yet where exposing APIs on
ServiceWorkerRegistration is possible, even though that apparently is the way
APIs (including push) are supposed to be exposed. This CL is part of an attempt
to refactor SW stuff to fix this, starting with geofencing since that's the API
I'm working on.

> 43	    [RuntimeEnabled=Geofencing, CallWith=ScriptState] Promise
> registerRegion(GeofencingRegion region);
> 44	    [RuntimeEnabled=Geofencing, CallWith=ScriptState] Promise
> unregisterRegion(DOMString regionId);
> 45	    [RuntimeEnabled=Geofencing, CallWith=ScriptState] Promise
> getRegisteredRegions();
> 
> Where do the Entered/Exited events get raised?
At least for now, only in the service worker.

> Lets say a page loads, calls registerRegion, and then the tab is closed.
> Sometime later the user strays into the region... wha happens? Suppose there
was
> no serviceworker for the page when the register call was made... whats
different
> for that case?
The intention is for these APIs to only be exposed on ServiceWorkerRegistration,
so there will always be a service worker when the register call is made (this is
assuming I can convince Mozilla and other parties that it is okay to require a
page to have a service worker to use geofencing).

> Let's say there's a page open at time the region is entered (or the page that
> did the initial registration is still open), does anything different happen in
> that (those) cases?
It is at least my intention that no, those cases are all the same.

> Are the methods above accessible in worker contexts too, if not today, will
they
> be at some point?
They should. As far as I know there is an open issue for the service worker team
to expose the ServiceWorkerRegistration within the service worker itself as well
(I'm not sure if/how service workers are currently exposed in regular (shared)
workers?)
Until now since the infrastructure to expose APIs the "service worker" way was
missing, this API was only exposed on navigator (in both worker contexts and
websites). This is a first step to attempt to fix the gaps on the service worker
side.

> With the handleId stuff, you are tripping on code that probably needs to be
> re-factored (read rewritten). Looks like you want registrationId + origin at
the
> end of the day, is that right?
Yes, I think that is what I want. Something I can store (ideally across browser
restarts, but for now I'd already be happy with just during a single browser
session) that will allow me to deliver an event to the correct service worker.

[Marijn Kruisselbrink, 2014-10-09 23:42:49]

(also see the unanswered mail to chrome-serviceworker@ I sent last week)

[michaeln, 2014-10-13 00:32:49]

https://codereview.chromium.org/623823002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/623823002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:897: new
GeofencingDispatcherHost(browser_context_, service_worker_filter));
On 2014/10/09 22:28:59, Marijn Kruisselbrink wrote:
> On 2014/10/09 12:28:09, Michael van Ouwerkerk wrote:
> > This should pass in storage_partition_impl_->GetServiceWorkerContext()
instead
> > of the filter.
> That's not enough. To go from registration handle ID to RegistrationHandle
(and
> eventually to Registration) some renderer process specific state is necessary,
> and currently that state only lives in ServiceWorkerDispatcherHost. So either
> the IPC would have to use some other identifier, the mapping from handle id to
> handle would have to live in a different object, or some other way of exposing
> this mapping is needed.

Kinda messy, no?

I'm not sure the mapping is needed at all. Looks like what you want is the
registraion id and the origin, and that the indirection with handle ids is
getting in the way. There's a bug open about cleaning up how the peer objects
are maintained between the renderer/browser processes (crbug/384119).

I'd vote to arrange something here that is in keeping with the direction
outlined in that cleanup, but w/o having done all of the cleanup. That cleanup
suggests getting rid of handle_ids, so how about we use registration_id as the
message param for this stuff. In that case, sw_context is sufficient to retrieve
a ServiceWorkerRegistration instance given a registration_id and to get is
origin.

The renderer side data structures currently don't store the registration_id
value. I think we could include that value in the
ServiceWorkerRegistrationObjectInfo struct and use it in place of handle_id for
the geofencing messages. On the browser side, you should be able to use
context_->GetLiveRegistration(id) and expect a non-null value back since the
messages your handling are method calls on a registration object which implies a
live instance.

Wdyt, less messy?

[Marijn Kruisselbrink, 2014-10-13 19:26:24]

Patchset #4 (id:120001) has been deleted

[Marijn Kruisselbrink, 2014-10-13 19:29:49]

https://codereview.chromium.org/623823002/diff/40001/content/browser/geofenci...
File content/browser/geofencing/geofencing_dispatcher_host.cc (right):

https://codereview.chromium.org/623823002/diff/40001/content/browser/geofenci...
content/browser/geofencing/geofencing_dispatcher_host.cc:59:
ServiceWorkerRegistration* service_worker =
On 2014/10/09 12:28:09, Michael van Ouwerkerk wrote:
> Let's be clear and call the variable service_worker_registration.

Done.

https://codereview.chromium.org/623823002/diff/40001/content/browser/geofenci...
content/browser/geofencing/geofencing_dispatcher_host.cc:122: service_worker ?
service_worker->id() : -1;
On 2014/10/08 18:41:29, jsbell wrote:
> Can you make this an enum value in the GeofencingManager rather than a magic
> number?
> 
> (I see it's documented in a comment in
> https://codereview.chromium.org/586163003/)
I changed the code to just never call GeofencingManager if we don't have a valid
service worker registration.

https://codereview.chromium.org/623823002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/623823002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:897: new
GeofencingDispatcherHost(browser_context_, service_worker_filter));
On 2014/10/13 00:32:49, michaeln wrote:
> Wdyt, less messy?
Yes, that sounds much better. I've now changed it to use the registration_id
directly for all geofencing IPCs.

https://codereview.chromium.org/623823002/diff/40001/content/child/geofencing...
File content/child/geofencing/geofencing_dispatcher.cc (right):

https://codereview.chromium.org/623823002/diff/40001/content/child/geofencing...
content/child/geofencing/geofencing_dispatcher.cc:89: if (serviceworker)
On 2014/10/08 18:41:30, jsbell wrote:
> nit: need braces around multi-line if body

Done.

https://codereview.chromium.org/623823002/diff/40001/content/child/geofencing...
content/child/geofencing/geofencing_dispatcher.cc:102: if (serviceworker)
On 2014/10/08 18:41:30, jsbell wrote:
> nit: need braces around multi-line if body

Done.

https://codereview.chromium.org/623823002/diff/40001/content/child/geofencing...
File content/child/geofencing/geofencing_dispatcher.h (right):

https://codereview.chromium.org/623823002/diff/40001/content/child/geofencing...
content/child/geofencing/geofencing_dispatcher.h:38:
blink::WebServiceWorkerRegistration* serviceworker,
On 2014/10/09 12:28:10, Michael van Ouwerkerk wrote:
> Let's call it service_worker_registration, here and below.

Done.

[michaeln, 2014-10-14 01:03:52]

lgtm, thnx

https://codereview.chromium.org/623823002/diff/250001/content/browser/geofenc...
File content/browser/geofencing/geofencing_dispatcher_host.cc (right):

https://codereview.chromium.org/623823002/diff/250001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.cc:71:
service_worker_registration
don't need to check for null here

https://codereview.chromium.org/623823002/diff/250001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.cc:115: ?
service_worker_registration->pattern().GetOrigin()
ditto, null check not needed

https://codereview.chromium.org/623823002/diff/250001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.cc:150:
service_worker_registration
here too

https://codereview.chromium.org/623823002/diff/250001/content/child/geofencin...
File content/child/geofencing/geofencing_dispatcher.cc (right):

https://codereview.chromium.org/623823002/diff/250001/content/child/geofencin...
content/child/geofencing/geofencing_dispatcher.cc:73: int64
serviceworker_registration_id = -1;
could use kInvalidServiceWorkerRegistrationId here and below
(content/common/service_worker_types.h)

[Michael van Ouwerkerk, 2014-10-14 13:06:46]

lgtm much better!

[Marijn Kruisselbrink, 2014-10-14 18:51:21]

mek@chromium.org changed reviewers:
+ avi@chromium.org, palmer@chromium.org

[Marijn Kruisselbrink, 2014-10-14 18:51:23]

+avi for content/browser/renderer_host/render_process_host_impl.cc OWNERS
+palmer for IPC changes

https://codereview.chromium.org/623823002/diff/250001/content/browser/geofenc...
File content/browser/geofencing/geofencing_dispatcher_host.cc (right):

https://codereview.chromium.org/623823002/diff/250001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.cc:71:
service_worker_registration
On 2014/10/14 01:03:52, michaeln wrote:
> don't need to check for null here

Done.

https://codereview.chromium.org/623823002/diff/250001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.cc:115: ?
service_worker_registration->pattern().GetOrigin()
On 2014/10/14 01:03:52, michaeln wrote:
> ditto, null check not needed

Done.

https://codereview.chromium.org/623823002/diff/250001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.cc:150:
service_worker_registration
On 2014/10/14 01:03:52, michaeln wrote:
> here too

Done.

https://codereview.chromium.org/623823002/diff/250001/content/child/geofencin...
File content/child/geofencing/geofencing_dispatcher.cc (right):

https://codereview.chromium.org/623823002/diff/250001/content/child/geofencin...
content/child/geofencing/geofencing_dispatcher.cc:73: int64
serviceworker_registration_id = -1;
On 2014/10/14 01:03:52, michaeln wrote:
> could use kInvalidServiceWorkerRegistrationId here and below
> (content/common/service_worker_types.h)

Done.

[Marijn Kruisselbrink, 2014-10-15 23:44:08]

mek@chromium.org changed reviewers:
- avi@chromium.org

[Marijn Kruisselbrink, 2014-10-15 23:44:09]

I rebased this on top of https://codereview.chromium.org/645763003. It means
I'll have to wait with landing this CL until I've got the other CL figured out,
but it simplifies some bits in this CL, and makes it easier for me to continue
development on top of it all.
Even if the other CL will need changes before it can be landed, I don't expect
that to influence this much.

[palmer, 2014-10-16 01:22:34]

https://codereview.chromium.org/623823002/diff/370001/content/browser/geofenc...
File content/browser/geofencing/geofencing_dispatcher_host.h (right):

https://codereview.chromium.org/623823002/diff/370001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.h:37: const std::string&
region_id,
What is the lexical/syntactic structure of these region IDs? Can/do we assert
the correct structure somewhere in the browser process?

https://codereview.chromium.org/623823002/diff/370001/content/child/geofencin...
File content/child/geofencing/geofencing_dispatcher.cc (right):

https://codereview.chromium.org/623823002/diff/370001/content/child/geofencin...
content/child/geofencing/geofencing_dispatcher.cc:72: // TODO(mek): Reject
requests without a service worker registration without
Nit: Wording: "without...without..." sounds a bit odd. Maybe "Immediately reject
requests lacking a service worker registration, without bouncing...".

[Marijn Kruisselbrink, 2014-10-16 17:00:19]

https://codereview.chromium.org/623823002/diff/370001/content/browser/geofenc...
File content/browser/geofencing/geofencing_dispatcher_host.h (right):

https://codereview.chromium.org/623823002/diff/370001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.h:37: const std::string&
region_id,
On 2014/10/16 01:22:34, Chromium Palmer wrote:
> What is the lexical/syntactic structure of these region IDs? Can/do we assert
> the correct structure somewhere in the browser process?
There is no lexical/syntactic structure, these are arbitrary website provided
strings. At your suggestion these methods already do a basic sanity check to
make sure the size of these strings is < 200.

https://codereview.chromium.org/623823002/diff/370001/content/child/geofencin...
File content/child/geofencing/geofencing_dispatcher.cc (right):

https://codereview.chromium.org/623823002/diff/370001/content/child/geofencin...
content/child/geofencing/geofencing_dispatcher.cc:72: // TODO(mek): Reject
requests without a service worker registration without
On 2014/10/16 01:22:34, Chromium Palmer wrote:
> Nit: Wording: "without...without..." sounds a bit odd. Maybe "Immediately
reject
> requests lacking a service worker registration, without bouncing...".

Done.

[palmer, 2014-10-16 22:55:36]

Ahh yes. LGTM.

[Marijn Kruisselbrink, 2014-10-23 23:35:00]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-10-23 23:39:27]

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

[commit-bot: I haz the power, 2014-10-24 01:00:58]

Committed patchset #8 (id:410001)

[commit-bot: I haz the power, 2014-10-24 01:01:46]

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