Pass through the geofencing API calls to the content layer.

Pass through the geofencing API calls to the content layer.

Mostly modelled on how the Push API passes its registration call on to the content layer.

BUG=383125

[Marijn Kruisselbrink, 2014-07-14 14:50:16]

Not sure who of you wants to review this. Hopefully this is the correct way to
pass these API calls on to the content layer... I could of course also have gone
through GeolocationController/GeolocationClientProxy etc, but it seemed cleaner
to keep geofencing as separate as possible (and I was told that the current
geolocation API wasn't the best example of how to implement an API). So
hopefully mimicking what the push API seems to be doing was the right thing to
do.

[Peter Beverloo, 2014-07-14 17:23:09]

First round of feedback. I only looked at the API part for now as Michael might
want to review the modules/ part -- happy to take a look at that as well if he
prefers.

Are you planning to make the Geolocation object available in workers? I'm
worried that you're going to run in some large architectural issues with and
after this patch.

- The navigator.geolocation object exposes the |(un)registerRegion| and
|getRegisteredRegions| methods, which make perfect sense in Workers, but isn't
exposed on Workers by itself, meaning they won't be accessible in the current
design.

- There's loads of security and privacy questions about exposing Geolocation to
workers. Then again, very similar questions are being raised by developing this
API. The |sync| event would benefit from the user's location being available.

- Moving Geolocation functionality off WebFrame is going to be a serious
undertaking of its own, but it's due a refactoring indeed.

I think we're going to want to expose Geolocation eventually anyway, so this
would be a very appropriate subject to bring up with Michael and the Geolocation
WG.

https://codereview.chromium.org/393533002/diff/1/Source/web/WebCircularRegion...
File Source/web/WebCircularRegion.cpp (right):

https://codereview.chromium.org/393533002/diff/1/Source/web/WebCircularRegion...
Source/web/WebCircularRegion.cpp:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
This should be in Source/platform/exported/.

https://codereview.chromium.org/393533002/diff/1/Source/web/WebCircularRegion...
Source/web/WebCircularRegion.cpp:16: return
static_cast<CircularRegion*>(*this)->latitude();
|this| is a pointer, the dereference doesn't make sense. Does it even compile?

https://codereview.chromium.org/393533002/diff/1/Source/web/WebCircularRegion...
Source/web/WebCircularRegion.cpp:34: return
static_cast<CircularRegion*>(static_cast<GeofencingRegion*>(*this));
I really don't like all these static_casts. We shouldn't do them on |this|
either.

There isn't a lot of precedence for inheritance in the APIs with implementations
in Blink. If you make |toCircularRegion| return a pointer, they could work like:

double WebCircularRegion::radius() const
{
    return toCircularRegion()->radius();
}

That's already a lot nicer.

https://codereview.chromium.org/393533002/diff/1/Source/web/WebGeofencingRegi...
File Source/web/WebGeofencingRegion.cpp (right):

https://codereview.chromium.org/393533002/diff/1/Source/web/WebGeofencingRegi...
Source/web/WebGeofencingRegion.cpp:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
This should be in Source/platform/exported/.

https://codereview.chromium.org/393533002/diff/1/Source/web/WebGeofencingRegi...
Source/web/WebGeofencingRegion.cpp:41: bool
WebGeofencingRegion::isCircularRegion() const
nit: it's nice if the order of methods in a .cpp matches the header file. We do
sometimes put all constructors/destructors on top, too.

https://codereview.chromium.org/393533002/diff/1/public/platform/WebGeofencin...
File public/platform/WebGeofencingClient.h (right):

https://codereview.chromium.org/393533002/diff/1/public/platform/WebGeofencin...
public/platform/WebGeofencingClient.h:27: // FIXME: somehow cleanup the callback
type to not have a success type.
Has anyone filed a bug to track this? Resolving (or rejecting) a promise with
undefined shouldn't be too uncommon.

Perhaps it would be nice to resolve the promise with the Id of the newly
(un)registered promise, in case a developer wants to re-use a function for the
callbacks.

https://codereview.chromium.org/393533002/diff/1/public/platform/WebGeofencin...
File public/platform/WebGeofencingRegion.h (right):

https://codereview.chromium.org/393533002/diff/1/public/platform/WebGeofencin...
public/platform/WebGeofencingRegion.h:25: ~WebGeofencingRegion() { reset(); }
Mark this as virtual and have empty destructors in the classes inheriting from
this, just to err on the safe side (if any of them ever have non-scalar
members)?

https://codereview.chromium.org/393533002/diff/1/public/platform/WebGeofencin...
public/platform/WebGeofencingRegion.h:33: BLINK_EXPORT WebCircularRegion
asCircularRegion() const;
This would make a copy. Should we instead return a pointer to a casted |this|?

Naming nit: s/as/to/.

https://codereview.chromium.org/393533002/diff/1/public/platform/WebGeofencin...
public/platform/WebGeofencingRegion.h:36:
WebGeofencingRegion(WebCore::GeofencingRegion*);
This constructor should be marked as explicit.

https://codereview.chromium.org/393533002/diff/1/public/web/WebCircularRegion.h
File public/web/WebCircularRegion.h (right):

https://codereview.chromium.org/393533002/diff/1/public/web/WebCircularRegion...
public/web/WebCircularRegion.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
This should be in public/platform/, as there is no relation to a WebFrame.

https://codereview.chromium.org/393533002/diff/1/public/web/WebCircularRegion...
public/web/WebCircularRegion.h:29: WebCircularRegion(WebCore::CircularRegion*);
This constructor should be marked as explicit.

https://codereview.chromium.org/393533002/diff/1/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/393533002/diff/1/public/web/WebFrameClient.h#...
public/web/WebFrameClient.h:470: virtual WebGeofencingClient* geofencingClient()
{ return 0; }
The geofencing client defines methods for (un)registering and getting a list of
registered regions. By plumbing these through WebFrame, the client will only be
available for Document contexts, and thus not for Worker contexts.

Given that the primary power of Geofencing can be found in workers, this doesn't
make sense. Instead, these calls should go through the Platform class, which
isn't related to any specific Frame/View.

[Peter Beverloo, 2014-07-14 18:16:13]

https://codereview.chromium.org/393533002/diff/1/Source/web/WebCircularRegion...
File Source/web/WebCircularRegion.cpp (right):

https://codereview.chromium.org/393533002/diff/1/Source/web/WebCircularRegion...
Source/web/WebCircularRegion.cpp:34: return
static_cast<CircularRegion*>(static_cast<GeofencingRegion*>(*this));
On 2014/07/14 17:23:08, Peter Beverloo wrote:
> I really don't like all these static_casts. We shouldn't do them on |this|
> either.
> 
> There isn't a lot of precedence for inheritance in the APIs with
implementations
> in Blink. If you make |toCircularRegion| return a pointer, they could work
like:
> 
> double WebCircularRegion::radius() const
> {
>     return toCircularRegion()->radius();
> }
> 
> That's already a lot nicer.

Note that |toCircularRegion| would be a private BLINK_IMPLEMENTATION detail of
the WebCircularRegion class in that case.

https://codereview.chromium.org/393533002/diff/1/public/platform/WebGeofencin...
File public/platform/WebGeofencingRegion.h (right):

https://codereview.chromium.org/393533002/diff/1/public/platform/WebGeofencin...
public/platform/WebGeofencingRegion.h:33: BLINK_EXPORT WebCircularRegion
asCircularRegion() const;
On 2014/07/14 17:23:09, Peter Beverloo wrote:
> This would make a copy. Should we instead return a pointer to a casted |this|?
> 
> Naming nit: s/as/to/.

Also needs the Web prefix: toWebCircularRegion, to avoid confusion with the
WebCore CircularRegion type.

[Michael van Ouwerkerk, 2014-07-15 18:04:42]

https://codereview.chromium.org/393533002/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/GeofencingRegion.h (right):

https://codereview.chromium.org/393533002/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/GeofencingRegion.h:21: virtual bool
isCircularRegion() const { return false; }
This seems to violate basic object oriented design: the base class should not
have a method named after one of its sub classes. Isn't there another to do
this?

https://codereview.chromium.org/393533002/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/393533002/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/Geolocation.cpp:31: #include
"bindings/core/v8/CallbackPromiseAdapter.h"
As we discussed offline, it seems quite possible to implement geofencing
independently from geolocation. That way it would be easier to iterate on
geofencing and without the risk of introducing regressions into geolocation.

https://codereview.chromium.org/393533002/diff/1/public/web/WebCircularRegion.h
File public/web/WebCircularRegion.h (right):

https://codereview.chromium.org/393533002/diff/1/public/web/WebCircularRegion...
public/web/WebCircularRegion.h:14: class WebCircularRegion : public
WebGeofencingRegion {
I think that if you make this a plain interface, CircularRegion can implement
it. That way you don't need WebCircularRegion.cpp at all.