Add the first very basic bits of a geofencing API.

Add the first very basic bits of a geofencing API.

Just adding the various methods on navigator.geolocation, which currently all always fail.

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

[Marijn Kruisselbrink, 2014-07-09 13:11:32]

I have (well, had, since so much changed/moved around in blink that resolving
all the merge conflicts is more work than just rewriting things) much more code
written, but instead of trying to send all of it out as one giant review
request, it seemed sensible to split things up, and send out review requests as
I go (like I should have done in the first place).
So this is the first part of that.

mvanouwerkerk@ for the actual review
jochen@ just for the RuntimeEnabledFeatures.in OWNERS approval

[jochen (gone - plz use gerrit), 2014-07-10 11:38:51]

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
File Source/modules/geolocation/CircularRegion.h (right):

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/CircularRegion.h:24: class CircularRegion : public
GeofencingRegion {
wtf_make_noncopyable()?

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
File Source/modules/geolocation/CircularRegion.idl (right):

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/CircularRegion.idl:11: const double MAX_RADIUS =
100.0;
is it clear for geo ppl what unit the radius is in?

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
File Source/modules/geolocation/GeofencingRegion.h (right):

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:14: class GeofencingRegion :
public GarbageCollectedFinalized<GeofencingRegion>, public ScriptWrappable {
wtf_make_noncopyable?

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:21: GeofencingRegion() { }
also needs to call ScriptWrappable::init

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:27: String m_id;
private?

https://codereview.chromium.org/375353002/diff/20001/Source/platform/RuntimeE...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/375353002/diff/20001/Source/platform/RuntimeE...
Source/platform/RuntimeEnabledFeatures.in:67: Geofencing status=experimental
testing maybe? it doesn't do anything yet

[Marijn Kruisselbrink, 2014-07-10 12:36:00]

Thanks for the review. This being my first attempt of doing anything more in
blink than just fixing a bug or two, I'm sure I'll make tons of obvious
mistakes, where I'm misunderstanding some of the not always very well documented
magic going on in blink. Just copying/modifying existing code without really
understanding why it does what it does is dangerous.

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
File Source/modules/geolocation/CircularRegion.h (right):

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/CircularRegion.h:24: class CircularRegion : public
GeofencingRegion {
On 2014/07/10 11:38:51, jochen wrote:
> wtf_make_noncopyable()?

Done.

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
File Source/modules/geolocation/CircularRegion.idl (right):

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/CircularRegion.idl:11: const double MAX_RADIUS =
100.0;
On 2014/07/10 11:38:51, jochen wrote:
> is it clear for geo ppl what unit the radius is in?
I don't know. Are IDL files supposed to document the API? The spec will of
course make it clear what units these are in. Also I'm not entirely convinced
yet it makes sense to have these constants in the first place, but that's a
separate issue.
(for now the only bits of "spec" there are are at 
https://github.com/mkruisselbrink/Geofencing/blob/master/explainer.md and
https://gist.github.com/mkruisselbrink/c5c5cd80f27f3158f451)

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
File Source/modules/geolocation/GeofencingRegion.h (right):

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:14: class GeofencingRegion :
public GarbageCollectedFinalized<GeofencingRegion>, public ScriptWrappable {
On 2014/07/10 11:38:51, jochen wrote:
> wtf_make_noncopyable?

Done.

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:21: GeofencingRegion() { }
On 2014/07/10 11:38:51, jochen wrote:
> also needs to call ScriptWrappable::init

Done.

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:27: String m_id;
On 2014/07/10 11:38:51, jochen wrote:
> private?

Done.

https://codereview.chromium.org/375353002/diff/20001/Source/platform/RuntimeE...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/375353002/diff/20001/Source/platform/RuntimeE...
Source/platform/RuntimeEnabledFeatures.in:67: Geofencing status=experimental
On 2014/07/10 11:38:51, jochen wrote:
> testing maybe? it doesn't do anything yet
ah yes, that's probably better. Although I'm not sure how I would manually test
a feature with status=test? If I understand the docs correctly status=test
features are only enabled for automated tests. Am I missing something/are the
docs missing something?

[Peter Beverloo, 2014-07-10 13:17:28]

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
File Source/modules/geolocation/CircularRegion.idl (right):

https://codereview.chromium.org/375353002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/CircularRegion.idl:11: const double MAX_RADIUS =
100.0;
On 2014/07/10 12:36:00, Marijn Kruisselbrink wrote:
> On 2014/07/10 11:38:51, jochen wrote:
> > is it clear for geo ppl what unit the radius is in?
> I don't know. Are IDL files supposed to document the API? The spec will of
> course make it clear what units these are in. Also I'm not entirely convinced
> yet it makes sense to have these constants in the first place, but that's a
> separate issue.
> (for now the only bits of "spec" there are are at 
> https://github.com/mkruisselbrink/Geofencing/blob/master/explainer.md and
> https://gist.github.com/mkruisselbrink/c5c5cd80f27f3158f451)

FWIW, the IDL files define the Web Exposed API surface of this feature. Would a
JavaScript developer reading the following code understand what the radius will
be?

var my_region = new CircularRegion({
    id: "work",
    latitude: 51.4947691,
    longitude: -0.1463945,
    radius: CircularRegion.MAX_RADIUS
});

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/CircularRegion.h (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/CircularRegion.h:24: class CircularRegion : public
GeofencingRegion {
Could mark this as FINAL.

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/CircularRegion.idl (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/CircularRegion.idl:8: Constructor(Dictionary init)
What is the expected behavior of the following?

var x = new CircularRegion({});

Would it have any use at all? Does the spec allow for it, or should we throw an
exception? If it's allowed, we may as well make the |init| argument optional to
allow the following syntax for empty circular regions:

var x = new CircularRegion();

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/CircularRegion.idl:10: const double MIN_RADIUS = 1.0;
I share Jochen's concern that this isn't clear without a unit. These constants
will be exposed to developers. Even if you're not sure whether they'll stick
around, could we suffix them with a unit?

Given that a range of [1, 100] only makes sense in meters (kilometers would be
way too long), perhaps MIN_RADIUS_METERS? A hundred meters is not a very long
distance either for some of the geofencing use-cases (e.g. nearby attractions).

Let's not even consider imperial units? :-)

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/GeofencingRegion.h (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:14: class GeofencingRegion :
public GarbageCollectedFinalized<GeofencingRegion>, public ScriptWrappable {
Use GarbageCollected, since you don't have a destructor.

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:17: void trace(Visitor*) { }
virtual void trace(Visitor*) { }

I personally prefer the trace() method to come last in the public section,
because it's not part of this class' purpose, annotated by a comment stating
that it's implementing GarbageCollected.

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:22: GeofencingRegion()
This constructor isn't being used.

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/GeofencingRegion.idl (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.idl:9: ] interface GeofencingRegion
{
Interfaces are only exposed on |window| by default. It seems to me like we want
to enable Geofencing from workers as well (most notably Service Workers).

You should define |Exposed=Window&Worker| in the class annotations to enable
this (also on CircularRegion).

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.cpp:695: return promise;
Could just use the ScriptPromise class here (dito in unregisterRegion and
getRegisteredRegions).

return ScriptPromise::rejectWithDOMException(scriptState,
DOMError::create(NotSupportedError));

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/Geolocation.h (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.h:88: ScriptPromise
deregisterRegion(ScriptState*, const String& regionId);
s/deregister/unregister/?

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.h:89: ScriptPromise
getRegisteredRegions(ScriptState*);
Sounds like this would be const?

https://codereview.chromium.org/375353002/diff/40001/Source/modules/modules.gypi
File Source/modules/modules.gypi (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/modules.g...
Source/modules/modules.gypi:440: 'geolocation/CircularRegion.cpp',
We should list the header files here as well. Not technically required, but it
is very common practice (not sure why the geolocation code doesn't do it).

https://codereview.chromium.org/375353002/diff/40001/Source/platform/RuntimeE...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/375353002/diff/40001/Source/platform/RuntimeE...
Source/platform/RuntimeEnabledFeatures.in:67: Geofencing status=test
We should move this to status=experimental as soon as it does something useful.

[Marijn Kruisselbrink, 2014-07-10 15:11:01]

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/CircularRegion.h (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/CircularRegion.h:24: class CircularRegion : public
GeofencingRegion {
On 2014/07/10 13:17:27, Peter Beverloo wrote:
> Could mark this as FINAL.

Done.

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/CircularRegion.idl (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/CircularRegion.idl:8: Constructor(Dictionary init)
On 2014/07/10 13:17:28, Peter Beverloo wrote:
> What is the expected behavior of the following?
> 
> var x = new CircularRegion({});
> 
> Would it have any use at all? Does the spec allow for it, or should we throw
an
> exception? If it's allowed, we may as well make the |init| argument optional
to
> allow the following syntax for empty circular regions:
> 
> var x = new CircularRegion();
I don't think a CircularRegion without at least a latitude and longitude makes
sense, so yes, we should probably throw an exception if one of those fields
isn't present (for radius we can probably come up with some default value).
What do other web APIs do for things like this?

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/CircularRegion.idl:10: const double MIN_RADIUS = 1.0;
On 2014/07/10 13:17:27, Peter Beverloo wrote:
> I share Jochen's concern that this isn't clear without a unit. These constants
> will be exposed to developers. Even if you're not sure whether they'll stick
> around, could we suffix them with a unit?
> 
> Given that a range of [1, 100] only makes sense in meters (kilometers would be
> way too long), perhaps MIN_RADIUS_METERS? A hundred meters is not a very long
> distance either for some of the geofencing use-cases (e.g. nearby
attractions).
> 
> Let's not even consider imperial units? :-)
I agree that it isn't immediately clear what unit "radius" is in, and that that
should be documented somehow. I'm not sure I agree that suffixing these
constants with a unit makes sense though. If they're suffixed like that I would
at least assume that that must mean that they're in a different unit than the
radius field, unless radius is also renamed to radius_meters (but then where do
you stop, should longitude and latitude than also be longitude_degrees?
And I know it isn't really relevant to naming things in web APIs, but at least
the iOS geofencing API also just calls this "maximumRegionMonitoringDistance".
Are there other examples of web APIs where constants embed the unit they are in
in the name of the constant?

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/GeofencingRegion.h (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:14: class GeofencingRegion :
public GarbageCollectedFinalized<GeofencingRegion>, public ScriptWrappable {
On 2014/07/10 13:17:28, Peter Beverloo wrote:
> Use GarbageCollected, since you don't have a destructor.
That's what I had in PS1, it didn't compile with:
../../third_party/WebKit/Source/modules/geolocation/GeofencingRegion.h:14:1:error:
[blink-gc] Class 'GeofencingRegion' requires finalization.
class GeofencingRegion : public GarbageCollected<GeofencingRegion>, public
ScriptWrappable {
^
../../third_party/WebKit/Source/modules/geolocation/GeofencingRegion.h:14:69:
note: [blink-gc] Base class 'ScriptWrappable' requiring finalization declared
here:
class GeofencingRegion : public GarbageCollected<GeofencingRegion>, public
ScriptWrappable {
                                                                    ^
../../third_party/WebKit/Source/modules/geolocation/GeofencingRegion.h:27:5:
note: [blink-gc] Field 'm_id' requiring finalization declared here:
    String m_id;

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:17: void trace(Visitor*) { }
On 2014/07/10 13:17:28, Peter Beverloo wrote:
> virtual void trace(Visitor*) { }
> 
> I personally prefer the trace() method to come last in the public section,
> because it's not part of this class' purpose, annotated by a comment stating
> that it's implementing GarbageCollected.
It seems the vast majority of code doesn't have any kind of comment annotating
their trace methods, but if it's preferred I can of course add one (or I can
probably just remove the entire method since it is empty anyway?)

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:22: GeofencingRegion()
On 2014/07/10 13:17:28, Peter Beverloo wrote:
> This constructor isn't being used.
Good point, removed. If I later turn out to need it I can always re-add it.

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/GeofencingRegion.idl (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.idl:9: ] interface GeofencingRegion
{
On 2014/07/10 13:17:28, Peter Beverloo wrote:
> Interfaces are only exposed on |window| by default. It seems to me like we
want
> to enable Geofencing from workers as well (most notably Service Workers).
> 
> You should define |Exposed=Window&Worker| in the class annotations to enable
> this (also on CircularRegion).

Done.

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.cpp:695: return promise;
On 2014/07/10 13:17:28, Peter Beverloo wrote:
> Could just use the ScriptPromise class here (dito in unregisterRegion and
> getRegisteredRegions).
> 
> return ScriptPromise::rejectWithDOMException(scriptState,
> DOMError::create(NotSupportedError));

Done. Although I'll probably have to change this back to something using
ScriptPromiseResolver once I actually implement these methods, so not sure this
saves me much (but still good to know about what is possible in the promise
API).

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/Geolocation.h (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.h:88: ScriptPromise
deregisterRegion(ScriptState*, const String& regionId);
On 2014/07/10 13:17:28, Peter Beverloo wrote:
> s/deregister/unregister/?
Yeah, that seems to be the more common word used for this. Not sure why the
draft spec I inherited called it deregister. Changed.

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.h:89: ScriptPromise
getRegisteredRegions(ScriptState*);
On 2014/07/10 13:17:28, Peter Beverloo wrote:
> Sounds like this would be const?

Done.

https://codereview.chromium.org/375353002/diff/40001/Source/modules/modules.gypi
File Source/modules/modules.gypi (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/modules.g...
Source/modules/modules.gypi:440: 'geolocation/CircularRegion.cpp',
On 2014/07/10 13:17:28, Peter Beverloo wrote:
> We should list the header files here as well. Not technically required, but it
> is very common practice (not sure why the geolocation code doesn't do it).
I added the new files I added, should I add the existing geolocation header
files as well in the same commit?

https://codereview.chromium.org/375353002/diff/40001/Source/platform/RuntimeE...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/375353002/diff/40001/Source/platform/RuntimeE...
Source/platform/RuntimeEnabledFeatures.in:67: Geofencing status=test
On 2014/07/10 13:17:28, Peter Beverloo wrote:
> We should move this to status=experimental as soon as it does something
useful.

Acknowledged.

[Peter Beverloo, 2014-07-10 16:31:37]

Thanks for the updates, Marijn!

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/CircularRegion.idl (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/CircularRegion.idl:8: Constructor(Dictionary init)
On 2014/07/10 15:11:01, Marijn Kruisselbrink wrote:
> On 2014/07/10 13:17:28, Peter Beverloo wrote:
> > What is the expected behavior of the following?
> > 
> > var x = new CircularRegion({});
> > 
> > Would it have any use at all? Does the spec allow for it, or should we throw
> an
> > exception? If it's allowed, we may as well make the |init| argument optional
> to
> > allow the following syntax for empty circular regions:
> > 
> > var x = new CircularRegion();
> I don't think a CircularRegion without at least a latitude and longitude makes
> sense, so yes, we should probably throw an exception if one of those fields
> isn't present (for radius we can probably come up with some default value).
> What do other web APIs do for things like this?

What else do you imagine to be required for a circular reason? I would imagine
that |longitude| and |latitude| are required, |radius| is optional (to some sane
default) and |id| is optional as well (defaulting to empty, which is fine if we
only need one region).

For consistency with the Geolocation specification, you could have something
like (in which |options| currently provides for [id, radius]:

Constructor(double latitude, double longitude, optional Dictionary options);

However, this really isn't the forum to discuss the API. You should do that on
the Geolocation Working Group list.

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/GeofencingRegion.h (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:14: class GeofencingRegion :
public GarbageCollectedFinalized<GeofencingRegion>, public ScriptWrappable {
On 2014/07/10 15:11:01, Marijn Kruisselbrink wrote:
> On 2014/07/10 13:17:28, Peter Beverloo wrote:
> > Use GarbageCollected, since you don't have a destructor.
> That's what I had in PS1, it didn't compile with:
>
../../third_party/WebKit/Source/modules/geolocation/GeofencingRegion.h:14:1:error:
> [blink-gc] Class 'GeofencingRegion' requires finalization.
> class GeofencingRegion : public GarbageCollected<GeofencingRegion>, public
> ScriptWrappable {
> ^
> ../../third_party/WebKit/Source/modules/geolocation/GeofencingRegion.h:14:69:
> note: [blink-gc] Base class 'ScriptWrappable' requiring finalization declared
> here:
> class GeofencingRegion : public GarbageCollected<GeofencingRegion>, public
> ScriptWrappable {
>                                                                     ^
> ../../third_party/WebKit/Source/modules/geolocation/GeofencingRegion.h:27:5:
> note: [blink-gc] Field 'm_id' requiring finalization declared here:
>     String m_id;
> 

Aah, the String destructor. Indeed, but please just define an empty virtual
destructor (in child classes as well) to ensure that we destroy the right class
and its members.

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:17: void trace(Visitor*) { }
On 2014/07/10 15:11:01, Marijn Kruisselbrink wrote:
> On 2014/07/10 13:17:28, Peter Beverloo wrote:
> > virtual void trace(Visitor*) { }
> > 
> > I personally prefer the trace() method to come last in the public section,
> > because it's not part of this class' purpose, annotated by a comment stating
> > that it's implementing GarbageCollected.
> It seems the vast majority of code doesn't have any kind of comment annotating
> their trace methods, but if it's preferred I can of course add one (or I can
> probably just remove the entire method since it is empty anyway?)

It's my preference as a reviewer, you're welcome to have another opinion.

I would still define an empty virtual trace method, as there are 62 of those.
Please be sure to mark it as virtual.

https://codereview.chromium.org/375353002/diff/40001/Source/modules/modules.gypi
File Source/modules/modules.gypi (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/modules.g...
Source/modules/modules.gypi:440: 'geolocation/CircularRegion.cpp',
On 2014/07/10 15:11:01, Marijn Kruisselbrink wrote:
> On 2014/07/10 13:17:28, Peter Beverloo wrote:
> > We should list the header files here as well. Not technically required, but
it
> > is very common practice (not sure why the geolocation code doesn't do it).
> I added the new files I added, should I add the existing geolocation header
> files as well in the same commit?

No, that's OK. You should tell Michael, however.

https://codereview.chromium.org/375353002/diff/80001/Source/modules/geolocati...
File Source/modules/geolocation/CircularRegion.idl (right):

https://codereview.chromium.org/375353002/diff/80001/Source/modules/geolocati...
Source/modules/geolocation/CircularRegion.idl:9: Constructor(Dictionary init)
Mmm I see that the |id| is required for the unregisterRegion() call. May as well
have three required attributes then (id, latitude, longitude) and one optional
(region) or an optional options bag. Again, however, this is not really the
place to discuss that.

https://codereview.chromium.org/375353002/diff/80001/Source/modules/geolocati...
File Source/modules/geolocation/GeofencingRegion.idl (left):

https://codereview.chromium.org/375353002/diff/80001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.idl:6: NoInterfaceObject,
Why did you decide to remove |NoInterfaceObject| here? Is the GeofencingRegion
constructable, or could developers do something useful with it when it's exposed
(e.g. extend its prototype)?

https://codereview.chromium.org/375353002/diff/80001/Source/modules/geolocati...
File Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/375353002/diff/80001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.cpp:689: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError));
Could we perhaps add a very basic layout test to verify that these in fact
reject? Could be as simple as:

navigator.geolocation.registerRegion(new CircularRegion(0, 0)).then(
    function() { testFailed("Promise unexpectedly resolved."); },
    function() { testPassed("Promise rejected as expected."); });

https://codereview.chromium.org/375353002/diff/80001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.cpp:694: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError));
dito.

https://codereview.chromium.org/375353002/diff/80001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.cpp:699: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError));
dito.

[Marijn Kruisselbrink, 2014-07-11 10:06:40]

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/CircularRegion.idl (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/CircularRegion.idl:8: Constructor(Dictionary init)
On 2014/07/10 16:31:36, Peter Beverloo wrote:
> On 2014/07/10 15:11:01, Marijn Kruisselbrink wrote:
> > On 2014/07/10 13:17:28, Peter Beverloo wrote:
> > > What is the expected behavior of the following?
> > > 
> > > var x = new CircularRegion({});
> > > 
> > > Would it have any use at all? Does the spec allow for it, or should we
throw
> > an
> > > exception? If it's allowed, we may as well make the |init| argument
optional
> > to
> > > allow the following syntax for empty circular regions:
> > > 
> > > var x = new CircularRegion();
> > I don't think a CircularRegion without at least a latitude and longitude
makes
> > sense, so yes, we should probably throw an exception if one of those fields
> > isn't present (for radius we can probably come up with some default value).
> > What do other web APIs do for things like this?
> 
> What else do you imagine to be required for a circular reason? I would imagine
> that |longitude| and |latitude| are required, |radius| is optional (to some
sane
> default) and |id| is optional as well (defaulting to empty, which is fine if
we
> only need one region).
> 
> For consistency with the Geolocation specification, you could have something
> like (in which |options| currently provides for [id, radius]:
> 
> Constructor(double latitude, double longitude, optional Dictionary options);
> 
> However, this really isn't the forum to discuss the API. You should do that on
> the Geolocation Working Group list.
Yes, I agree that this isn't the place to discuss the API. And I'm sure that
whatever API I will commit here will change later. But to actually write an
implementation I will need some API, and I don't think it makes sense to block
that on first figuring out the perfect API.

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/GeofencingRegion.h (right):

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:14: class GeofencingRegion :
public GarbageCollectedFinalized<GeofencingRegion>, public ScriptWrappable {
On 2014/07/10 16:31:36, Peter Beverloo wrote:
> On 2014/07/10 15:11:01, Marijn Kruisselbrink wrote:
> > On 2014/07/10 13:17:28, Peter Beverloo wrote:
> > > Use GarbageCollected, since you don't have a destructor.
> > That's what I had in PS1, it didn't compile with:
> >
>
../../third_party/WebKit/Source/modules/geolocation/GeofencingRegion.h:14:1:error:
> > [blink-gc] Class 'GeofencingRegion' requires finalization.
> > class GeofencingRegion : public GarbageCollected<GeofencingRegion>, public
> > ScriptWrappable {
> > ^
> >
../../third_party/WebKit/Source/modules/geolocation/GeofencingRegion.h:14:69:
> > note: [blink-gc] Base class 'ScriptWrappable' requiring finalization
declared
> > here:
> > class GeofencingRegion : public GarbageCollected<GeofencingRegion>, public
> > ScriptWrappable {
> >                                                                     ^
> > ../../third_party/WebKit/Source/modules/geolocation/GeofencingRegion.h:27:5:
> > note: [blink-gc] Field 'm_id' requiring finalization declared here:
> >     String m_id;
> > 
> 
> Aah, the String destructor. Indeed, but please just define an empty virtual
> destructor (in child classes as well) to ensure that we destroy the right
class
> and its members.

Done.

https://codereview.chromium.org/375353002/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.h:17: void trace(Visitor*) { }
On 2014/07/10 16:31:36, Peter Beverloo wrote:
> On 2014/07/10 15:11:01, Marijn Kruisselbrink wrote:
> > On 2014/07/10 13:17:28, Peter Beverloo wrote:
> > > virtual void trace(Visitor*) { }
> > > 
> > > I personally prefer the trace() method to come last in the public section,
> > > because it's not part of this class' purpose, annotated by a comment
stating
> > > that it's implementing GarbageCollected.
> > It seems the vast majority of code doesn't have any kind of comment
annotating
> > their trace methods, but if it's preferred I can of course add one (or I can
> > probably just remove the entire method since it is empty anyway?)
> 
> It's my preference as a reviewer, you're welcome to have another opinion.
> 
> I would still define an empty virtual trace method, as there are 62 of those.
> Please be sure to mark it as virtual.

Done.

https://codereview.chromium.org/375353002/diff/80001/Source/modules/geolocati...
File Source/modules/geolocation/GeofencingRegion.idl (left):

https://codereview.chromium.org/375353002/diff/80001/Source/modules/geolocati...
Source/modules/geolocation/GeofencingRegion.idl:6: NoInterfaceObject,
On 2014/07/10 16:31:36, Peter Beverloo wrote:
> Why did you decide to remove |NoInterfaceObject| here? Is the GeofencingRegion
> constructable, or could developers do something useful with it when it's
exposed
> (e.g. extend its prototype)?
I removed it because you told me to add Exposed=Window&Worker to this interface
as well (and the two attributes are mutually exclusive). But yes, I think
NoInterfaceObject would be the correct attribute, so I've changed it back.

https://codereview.chromium.org/375353002/diff/80001/Source/modules/geolocati...
File Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/375353002/diff/80001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.cpp:689: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError));
On 2014/07/10 16:31:36, Peter Beverloo wrote:
> Could we perhaps add a very basic layout test to verify that these in fact
> reject? Could be as simple as:
> 
> navigator.geolocation.registerRegion(new CircularRegion(0, 0)).then(
>     function() { testFailed("Promise unexpectedly resolved."); },
>     function() { testPassed("Promise rejected as expected."); });

Done (although it seems a bit silly to write tests that we know will break once
the code is actually correct).

https://codereview.chromium.org/375353002/diff/80001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.cpp:694: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError));
On 2014/07/10 16:31:37, Peter Beverloo wrote:
> dito.

Done.

https://codereview.chromium.org/375353002/diff/80001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.cpp:699: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError));
On 2014/07/10 16:31:37, Peter Beverloo wrote:
> dito.

Done.

[Peter Beverloo, 2014-07-11 10:34:34]

lgtm % a few nits. You will need OWNERS approval as well, perhaps Jochen can
take another look for that.

https://codereview.chromium.org/375353002/diff/100001/LayoutTests/fast/dom/Ge...
File LayoutTests/fast/dom/Geolocation/geofencing-not-implemented.html (right):

https://codereview.chromium.org/375353002/diff/100001/LayoutTests/fast/dom/Ge...
LayoutTests/fast/dom/Geolocation/geofencing-not-implemented.html:17:
shouldReject(navigator.geolocation.registerRegion(new CircularRegion({latitude:
37.421999, longitude: -122.084015})));
Ah, the Googleplex!

https://codereview.chromium.org/375353002/diff/100001/Source/modules/geolocat...
File Source/modules/geolocation/CircularRegion.cpp (right):

https://codereview.chromium.org/375353002/diff/100001/Source/modules/geolocat...
Source/modules/geolocation/CircularRegion.cpp:17: init.get("id", id);
You will likely have to rebase this on top of the following patch once it lands:
  https://codereview.chromium.org/373423002/

https://codereview.chromium.org/375353002/diff/100001/Source/modules/geolocat...
Source/modules/geolocation/CircularRegion.cpp:34: ScriptWrappable::init(this);
We will end up calling ScriptWrappable::init() twice, since we also call it in
the GeofencingRegion constructor. Just remove it there?

https://codereview.chromium.org/375353002/diff/100001/Source/modules/geolocat...
File Source/modules/geolocation/CircularRegion.idl (right):

https://codereview.chromium.org/375353002/diff/100001/Source/modules/geolocat...
Source/modules/geolocation/CircularRegion.idl:12: const double MAX_RADIUS =
100.0;
Ack @ API changes. I trust you to pursue them :-).

https://codereview.chromium.org/375353002/diff/100001/Source/modules/geolocat...
File Source/modules/geolocation/Geolocation.idl (right):

https://codereview.chromium.org/375353002/diff/100001/Source/modules/geolocat...
Source/modules/geolocation/Geolocation.idl:28: GarbageCollected,
Keep in mind that the Geolocation object would have to be exposed in Workers as
well if you want to enable workers to unregister regions. Since the Geolocation
specification makes absolutely no mention of that, it might be nice to discuss
this with Michael.

[jochen (gone - plz use gerrit), 2014-07-14 11:06:18]

lgtm

[Marijn Kruisselbrink, 2014-07-14 11:13:25]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-07-14 11:13:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mek@chromium.org/375353002/120001

[commit-bot: I haz the power, 2014-07-14 12:18:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16723)

[commit-bot: I haz the power, 2014-07-14 12:58:52]

Change committed as 178052