Blink side of exposing the service worker registration associated with geofencing API calls.

Blink side of exposing the service worker registration associated with geofencing API calls.
This still leaves the old codepaths for the geofencing API in place, those will be cleaned up in a followup CL.

BUG=383125
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183931

[Marijn Kruisselbrink, 2014-10-06 20:55:42]

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

[Marijn Kruisselbrink, 2014-10-06 20:55:43]

Even though most of the code changes are not directly in
modules/serviceworkers/, a serviceworkers OWNERS is probably the best person to
review this.
This is the blink side of things, which seems like it would be fairly
uncontroversial (the only thing I'm not sure about is if the
Member<ServiceWorkerRegistration> fields should maybe be WeakMembers instead...

The chrome side of this in https://codereview.chromium.org/623823002/ can
probably be done cleaner then I've currently done it.

[jsbell, 2014-10-08 18:51:57]

Sorry about the delay - OOO then swamped.

I think michaeln or falken should do the OWNERS review.

Can you provide a link to any documentation you have for the API? (the test
explains it, but pawing through mail and gists also helped give it context, so a
canonical link would be great; maybe I just missed the spec?)

Re: WeakMember<> - I don't think it's necessary; script could also keep the SWR
object alive indefinitely, and it would complicate your logic.

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
File LayoutTests/http/tests/geofencing/apis_not_implemented.html (right):

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:2: <title>Basic test
for some stuff</title>
Real title

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:20: var sw_scope =
window.location.origin + '/service-worker-scope' +
Don't need window.location.origin here - the scope will be resolved relative to
it automatically.

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:31:
.catch(function() { });
Assert something about the rejection here, so you know the registration is
failing for the reasons you expect (and not e.g. part of the test throwing due
to a typo in the script)

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:39: return
r.geofencing.unregisterRegion("")
Prefer single quotes: ''

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:41:
.catch(function() { });
Assert something about the rejection here.

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:51:
.catch(function() { });
Assert something about the rejection here.

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
File Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.cpp (right):

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.cpp:27:
ServiceWorkerRegistrationGeofencing&
ServiceWorkerRegistrationGeofencing::from(ServiceWorkerRegistration& navigator)
Rename 'navigator' to 'registration'

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.cpp:37:
Geofencing*
ServiceWorkerRegistrationGeofencing::geofencing(ServiceWorkerRegistration&
navigator)
Ditto

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
File Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.h (right):

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.h:16: class
ServiceWorkerRegistrationGeofencing FINAL : public
GarbageCollectedFinalized<ServiceWorkerRegistrationGeofencing>, public
HeapSupplement<ServiceWorkerRegistration> {
nit: 'final' now.

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.h:26: virtual void
trace(Visitor*) OVERRIDE;
nit: 'override' now.

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
File Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.idl (right):

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.idl:7: ] partial
interface ServiceWorkerRegistration {
Include URL of spec in a comment

[Marijn Kruisselbrink, 2014-10-08 23:00:31]

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

[Marijn Kruisselbrink, 2014-10-08 23:00:32]

On 2014/10/08 18:51:57, jsbell wrote:
> Sorry about the delay - OOO then swamped.
no problem, thanks for the review

> I think michaeln or falken should do the OWNERS review.
Okay; the reason I picked you was that you were the geographically closest to
me, but if there are better reviewers I'll happily defer to them.
+michaeln: can you take a look?

> Can you provide a link to any documentation you have for the API? (the test
> explains it, but pawing through mail and gists also helped give it context, so
a
> canonical link would be great; maybe I just missed the spec?)
Unfortunately there isn't really a canonical URL I can link to yet. Various
versions of this API proposal live in gists and emails, but none made it into a
more permanent location so far. I'll follow up with cwilso to figure out how to
move forward with that side of things. But anyway,
http://lists.w3.org/Archives/Public/public-geolocation/2014Sep/0012.html has my
most recent proposal, but a lot of details are scattered through other emails in
that thread and other threads.

> Re: WeakMember<> - I don't think it's necessary; script could also keep the
SWR
> object alive indefinitely, and it would complicate your logic.
I wasn't quite sure how oilpan would deal with the circular references (between
SWR and SWRGeofencing), but using garbage collection instead of reference
counting should make sure that isn't a problem.

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
File LayoutTests/http/tests/geofencing/apis_not_implemented.html (right):

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:2: <title>Basic test
for some stuff</title>
On 2014/10/08 18:51:57, jsbell wrote:
> Real title

Done.

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:20: var sw_scope =
window.location.origin + '/service-worker-scope' +
On 2014/10/08 18:51:57, jsbell wrote:
> Don't need window.location.origin here - the scope will be resolved relative
to
> it automatically.
Okay, I just copied this from some of the existing service worker tests, but
I've removed that bit.

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:31:
.catch(function() { });
On 2014/10/08 18:51:56, jsbell wrote:
> Assert something about the rejection here, so you know the registration is
> failing for the reasons you expect (and not e.g. part of the test throwing due
> to a typo in the script)
I now did that by copying even more code from worker-test-harness.js into this
test... Is there any reason things like promise_test and assert_promise_rejects
live in worker-test-harness.js instead of in some file that can actually be
included from non worker based tests as well? Any suggestions what file to move
these to?

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:39: return
r.geofencing.unregisterRegion("")
On 2014/10/08 18:51:57, jsbell wrote:
> Prefer single quotes: ''

Done.

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:41:
.catch(function() { });
On 2014/10/08 18:51:56, jsbell wrote:
> Assert something about the rejection here.

Done.

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:51:
.catch(function() { });
On 2014/10/08 18:51:57, jsbell wrote:
> Assert something about the rejection here.

Done.

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
File Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.cpp (right):

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.cpp:27:
ServiceWorkerRegistrationGeofencing&
ServiceWorkerRegistrationGeofencing::from(ServiceWorkerRegistration& navigator)
On 2014/10/08 18:51:57, jsbell wrote:
> Rename 'navigator' to 'registration'
argh, the danger of copy-paste coding... fixed.

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.cpp:37:
Geofencing*
ServiceWorkerRegistrationGeofencing::geofencing(ServiceWorkerRegistration&
navigator)
On 2014/10/08 18:51:57, jsbell wrote:
> Ditto

Done.

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
File Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.h (right):

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.h:16: class
ServiceWorkerRegistrationGeofencing FINAL : public
GarbageCollectedFinalized<ServiceWorkerRegistrationGeofencing>, public
HeapSupplement<ServiceWorkerRegistration> {
On 2014/10/08 18:51:57, jsbell wrote:
> nit: 'final' now.

Done.

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.h:26: virtual void
trace(Visitor*) OVERRIDE;
On 2014/10/08 18:51:57, jsbell wrote:
> nit: 'override' now.

Done.

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
File Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.idl (right):

https://codereview.chromium.org/623813002/diff/20001/Source/modules/geofencin...
Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.idl:7: ] partial
interface ServiceWorkerRegistration {
On 2014/10/08 18:51:57, jsbell wrote:
> Include URL of spec in a comment
There isn't really anything that can be linked to unfortunately. So far all that
exists is mailing list threads.

[jsbell, 2014-10-08 23:13:54]

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
File LayoutTests/http/tests/geofencing/apis_not_implemented.html (right):

https://codereview.chromium.org/623813002/diff/20001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:31:
.catch(function() { });
On 2014/10/08 23:00:32, Marijn Kruisselbrink wrote:
> On 2014/10/08 18:51:56, jsbell wrote:
> > Assert something about the rejection here, so you know the registration is
> > failing for the reasons you expect (and not e.g. part of the test throwing
due
> > to a typo in the script)
> I now did that by copying even more code from worker-test-harness.js into this
> test... Is there any reason things like promise_test and
assert_promise_rejects
> live in worker-test-harness.js instead of in some file that can actually be
> included from non worker based tests as well? Any suggestions what file to
move
> these to?

Yeah, sorry about that. We'd been hoping to upstream these but the W3C reviewers
are being slow and we haven't been pushing.

You should coordinate with asanka@ who's been working on cleaning up the tests
and owns the upstreaming patch.

I completely support factoring some of these helpers out and landing them in
e.g. LayoutTests/http/tests/resources/testharness-helpers.js or something. We
were just waiting 'til someone else wanted them.

[Michael van Ouwerkerk, 2014-10-09 10:56:26]

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

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

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
File Source/modules/geofencing/Geofencing.cpp (right):

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.cpp:65: WebServiceWorkerRegistration*
serviceworker = nullptr;
Here and below, probably clearer to call the variable
'serviceWorkerRegistration'

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.cpp:68:
provider->registerRegion(region->id(),
toCircularGeofencingRegion(region)->webRegion(), serviceworker, callbacks);
Why is registerRegion called twice?

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.cpp:85:
provider->unregisterRegion(regionId, serviceworker, callbacks);
Why is unregisterRegion called twice?

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.cpp:102:
provider->getRegisteredRegions(serviceworker, callbacks);
Why is getRegisteredRegions called twice?

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
File Source/modules/geofencing/Geofencing.h (right):

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.h:35:
Geofencing(ServiceWorkerRegistration*);
explicit

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
File Source/modules/geofencing/NavigatorGeofencing.cpp (right):

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/NavigatorGeofencing.cpp:13:
NavigatorGeofencing::NavigatorGeofencing()
Shouldn't this whole class be deleted now?

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
File Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.h (right):

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.h:29:
ServiceWorkerRegistrationGeofencing(ServiceWorkerRegistration*);
explicit

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
File Source/modules/geofencing/WorkerNavigatorGeofencing.cpp (right):

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/WorkerNavigatorGeofencing.cpp:13:
WorkerNavigatorGeofencing::WorkerNavigatorGeofencing()
Shouldn't this whole class be deleted also?

[Marijn Kruisselbrink, 2014-10-09 18:10:54]

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
File Source/modules/geofencing/Geofencing.cpp (right):

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.cpp:65: WebServiceWorkerRegistration*
serviceworker = nullptr;
On 2014/10/09 10:56:27, Michael van Ouwerkerk wrote:
> Here and below, probably clearer to call the variable
> 'serviceWorkerRegistration'

Done.

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.cpp:68:
provider->registerRegion(region->id(),
toCircularGeofencingRegion(region)->webRegion(), serviceworker, callbacks);
On 2014/10/09 10:56:27, Michael van Ouwerkerk wrote:
> Why is registerRegion called twice?
Because I can't update blink and chromium in the same commit. So to avoid
leaking |callbacks| I just call both the old and the new method. After updating
chromium I'm of course going to remove the calls to the methods that don't pass
a service worker registration. I suppose alternatively I could have just made
the default registerRegion implementation call the error callback and delete the
callbacks, but since this is only a transient state of the code that didn't seem
worth it.
But you're right, I should have added a comment or two for this.

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.cpp:85:
provider->unregisterRegion(regionId, serviceworker, callbacks);
On 2014/10/09 10:56:27, Michael van Ouwerkerk wrote:
> Why is unregisterRegion called twice?
Same as for registerRegion.

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.cpp:102:
provider->getRegisteredRegions(serviceworker, callbacks);
On 2014/10/09 10:56:27, Michael van Ouwerkerk wrote:
> Why is getRegisteredRegions called twice?
Same as for registerRegion.

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
File Source/modules/geofencing/Geofencing.h (right):

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.h:35:
Geofencing(ServiceWorkerRegistration*);
On 2014/10/09 10:56:27, Michael van Ouwerkerk wrote:
> explicit

Done.

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
File Source/modules/geofencing/NavigatorGeofencing.cpp (right):

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/NavigatorGeofencing.cpp:13:
NavigatorGeofencing::NavigatorGeofencing()
On 2014/10/09 10:56:27, Michael van Ouwerkerk wrote:
> Shouldn't this whole class be deleted now?
Quoting the CL description "This still leaves the old codepaths for the
geofencing API in place, those will be cleaned up in a followup CL."
But if you want I can of course do more of the deletion/cleanup in this CL.

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
File Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.h (right):

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/ServiceWorkerRegistrationGeofencing.h:29:
ServiceWorkerRegistrationGeofencing(ServiceWorkerRegistration*);
On 2014/10/09 10:56:27, Michael van Ouwerkerk wrote:
> explicit

Done.

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
File Source/modules/geofencing/WorkerNavigatorGeofencing.cpp (right):

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/WorkerNavigatorGeofencing.cpp:13:
WorkerNavigatorGeofencing::WorkerNavigatorGeofencing()
On 2014/10/09 10:56:27, Michael van Ouwerkerk wrote:
> Shouldn't this whole class be deleted also?
Same as for NavigatorGeofencing, although here removing this whole class would
mean removing all geofencing methods from service workers, since
ServiceWorkerRegistration is not yet exposed to the service worker (not that
methods called via WorkerNavigatorGeofencing will have the correct service
worker associated with them anyway, but that's a separate issue).

[michaeln, 2014-10-12 23:51:02]

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
File Source/modules/geofencing/NavigatorGeofencing.cpp (right):

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/NavigatorGeofencing.cpp:13:
NavigatorGeofencing::NavigatorGeofencing()
On 2014/10/09 18:10:54, Marijn Kruisselbrink wrote:
> On 2014/10/09 10:56:27, Michael van Ouwerkerk wrote:
> > Shouldn't this whole class be deleted now?
> Quoting the CL description "This still leaves the old codepaths for the
> geofencing API in place, those will be cleaned up in a followup CL."
> But if you want I can of course do more of the deletion/cleanup in this CL.

From these review comments, looks like the existing navigator.geofencing surface
area is being replaced by registration.geofencing. Is that right?

It might be good to delete the old and commit the new at the same time.
Otherwise, it's hard to know what api your actually working towards. Its not so
clear in from the comments in the CL description, or from crbug/383125, or from
the code itself what the "old codepaths" are. If you want to leave old in place
for now, maybe put FIXMEs to delete the old stuff.

https://codereview.chromium.org/623813002/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerRegistration.h (right):

https://codereview.chromium.org/623813002/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerRegistration.h:31: , public
HeapSupplementable<ServiceWorkerRegistration> {
Can you briefly explain what this voodoo means and why its needed? Did something
about this CL require its addition? Thnx.

Oh, because this CL extends the api available on registration objects!

[Marijn Kruisselbrink, 2014-10-13 19:09:59]

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
File Source/modules/geofencing/NavigatorGeofencing.cpp (right):

https://codereview.chromium.org/623813002/diff/40001/Source/modules/geofencin...
Source/modules/geofencing/NavigatorGeofencing.cpp:13:
NavigatorGeofencing::NavigatorGeofencing()
On 2014/10/12 23:51:02, michaeln wrote:
> On 2014/10/09 18:10:54, Marijn Kruisselbrink wrote:
> > On 2014/10/09 10:56:27, Michael van Ouwerkerk wrote:
> > > Shouldn't this whole class be deleted now?
> > Quoting the CL description "This still leaves the old codepaths for the
> > geofencing API in place, those will be cleaned up in a followup CL."
> > But if you want I can of course do more of the deletion/cleanup in this CL.
> 
> From these review comments, looks like the existing navigator.geofencing
surface
> area is being replaced by registration.geofencing. Is that right?
> 
> It might be good to delete the old and commit the new at the same time.
> Otherwise, it's hard to know what api your actually working towards. Its not
so
> clear in from the comments in the CL description, or from crbug/383125, or
from
> the code itself what the "old codepaths" are. If you want to leave old in
place
> for now, maybe put FIXMEs to delete the old stuff.
I removed NavigatorGeofencing, and added FIXMEs to WorkerNavigatorGeofencing to
delete that one once ServiceWorkerRegistration is exposed in service workers.
I'd like to not completely lose the ability to call this API from a worker, just
to make sure nothing blows up if it is called that way. But obviously the API
won't actually work yet when called from a service worker since there isn't a
way yet to associate API calls with the service worker they are called from...

https://codereview.chromium.org/623813002/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerRegistration.h (right):

https://codereview.chromium.org/623813002/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerRegistration.h:31: , public
HeapSupplementable<ServiceWorkerRegistration> {
On 2014/10/12 23:51:02, michaeln wrote:
> Can you briefly explain what this voodoo means and why its needed? Did
something
> about this CL require its addition? Thnx.
> 
> Oh, because this CL extends the api available on registration objects!
Yes. For other modules to be able to extend ServiceWorkerRegistration, it needs
to implement HeapSupplementable (just like Navigator already did).

[michaeln, 2014-10-14 01:16:16]

lgtm, but somebody more blink savvy should say so too

https://codereview.chromium.org/623813002/diff/340001/LayoutTests/http/tests/...
File LayoutTests/http/tests/geofencing/apis_not_implemented.html (right):

https://codereview.chromium.org/623813002/diff/340001/LayoutTests/http/tests/...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:37: return
r.geofencing.getRegisteredRegions();
indent here doesn't match the indent on line 15, i don't know which style is
right?

https://codereview.chromium.org/623813002/diff/340001/Source/modules/geofenci...
File Source/modules/geofencing/Geofencing.h (right):

https://codereview.chromium.org/623813002/diff/340001/Source/modules/geofenci...
Source/modules/geofencing/Geofencing.h:37: Member<ServiceWorkerRegistration>
m_registration;
leap of faith on the smart  gc'able reference cycling ptr types :)

https://codereview.chromium.org/623813002/diff/340001/Source/modules/geofenci...
File Source/modules/geofencing/WorkerNavigatorGeofencing.cpp (right):

https://codereview.chromium.org/623813002/diff/340001/Source/modules/geofenci...
Source/modules/geofencing/WorkerNavigatorGeofencing.cpp:44: m_geofencing =
Geofencing::create(nullptr);
maybe add a FIXME here about passing in the registration for |this| service
worker context?

[jsbell, 2014-10-14 17:19:23]

lgtm as well

https://codereview.chromium.org/623813002/diff/340001/LayoutTests/http/tests/...
File LayoutTests/http/tests/geofencing/apis_not_implemented.html (right):

https://codereview.chromium.org/623813002/diff/340001/LayoutTests/http/tests/...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:27: return
r.geofencing.unregisterRegion('');
nit: 2 more spaces.

https://codereview.chromium.org/623813002/diff/340001/LayoutTests/http/tests/...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:37: return
r.geofencing.getRegisteredRegions();
On 2014/10/14 01:16:15, michaeln wrote:
> indent here doesn't match the indent on line 15, i don't know which style is
> right?

Should be 4. 2 from wrapping the contents of `then()` and 2 more from the
`function() {}`

(Neither my text editor nor my guts agree, but that's what I've been told. ES6
arrow functions FTW?)

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

mek@chromium.org changed reviewers:
+ japhet@chromium.org

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

+japhet for at least Source/platform and public/platform OWNERS

https://codereview.chromium.org/623813002/diff/340001/LayoutTests/http/tests/...
File LayoutTests/http/tests/geofencing/apis_not_implemented.html (right):

https://codereview.chromium.org/623813002/diff/340001/LayoutTests/http/tests/...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:27: return
r.geofencing.unregisterRegion('');
On 2014/10/14 17:19:23, jsbell wrote:
> nit: 2 more spaces.

Done.

https://codereview.chromium.org/623813002/diff/340001/LayoutTests/http/tests/...
LayoutTests/http/tests/geofencing/apis_not_implemented.html:37: return
r.geofencing.getRegisteredRegions();
On 2014/10/14 17:19:23, jsbell wrote:
> On 2014/10/14 01:16:15, michaeln wrote:
> > indent here doesn't match the indent on line 15, i don't know which style is
> > right?
> 
> Should be 4. 2 from wrapping the contents of `then()` and 2 more from the
> `function() {}`
> 
> (Neither my text editor nor my guts agree, but that's what I've been told. ES6
> arrow functions FTW?)
> 

Done.

https://codereview.chromium.org/623813002/diff/340001/Source/modules/geofenci...
File Source/modules/geofencing/WorkerNavigatorGeofencing.cpp (right):

https://codereview.chromium.org/623813002/diff/340001/Source/modules/geofenci...
Source/modules/geofencing/WorkerNavigatorGeofencing.cpp:44: m_geofencing =
Geofencing::create(nullptr);
On 2014/10/14 01:16:16, michaeln wrote:
> maybe add a FIXME here about passing in the registration for |this| service
> worker context?
Well, when the registration is accessible from the service worker itself, this
entire class can go away since then the geofencing API is reachable through that
ServiceWorkerRegistration instance (hence the FIXME: delete this class I added
to the header and idl files).

[Nate Chapin, 2014-10-15 21:07:44]

public/platform LGTM, I don't have Source/platform/OWNERS though

[Marijn Kruisselbrink, 2014-10-15 22:16:09]

mek@chromium.org changed reviewers:
+ esprehn@chromium.org

[Marijn Kruisselbrink, 2014-10-15 22:16:09]

+esprehn: can you OWNERS approve the Source/platform/RuntimeEnabledFeatures.in
change?

[esprehn, 2014-10-17 20:42:37]

lgtm, I'll admit that having properties on the registration related to every
feature in the engine we add seems like a mistake though. Why is it
registration.geolocation?

[Marijn Kruisselbrink, 2014-10-17 20:55:32]

On 2014/10/17 20:42:37, esprehn wrote:
> lgtm, I'll admit that having properties on the registration related to every
> feature in the engine we add seems like a mistake though. Why is it
> registration.geolocation?
https://github.com/slightlyoff/ServiceWorker/issues/445#issuecomment-54599192
seems to be where exposing APIs like this was first suggested, and from there
this apparently became the canonical way of doing things (and for example the
push API spec also was updated to be exposed via ServiceWorkerRegistration
instead of Navigator,
https://w3c.github.io/push-api/#extensions-to-the-serviceworkerregistration-i...
).
The other option I've seen mentioned is for an API to take a registration as
parameter, I'm not sure why one option got chosen over the other.

[Marijn Kruisselbrink, 2014-10-17 20:55:41]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-10-17 20:56:34]

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

[Marijn Kruisselbrink, 2014-10-17 21:59:18]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-10-17 22:00:00]

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

[commit-bot: I haz the power, 2014-10-17 22:31:17]

Committed patchset #8 (id:530001) as 183931

[Michael van Ouwerkerk, 2014-10-20 17:37:03]

belated lgtm :-)