Pass through geofencing API calls to the content layer.

Pass through geofencing API calls to the content layer.

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

[Marijn Kruisselbrink, 2014-08-13 23:47:12]

This mostly implements (part of) the blink side of things in my design doc
(https://docs.google.com/document/d/1f0YwJDRvoGko4OaD8gWJdmgLIPTMs_SiOd3rjHyd_...).
In particular the code paths to pass API calls from blink on to the content
layer.

There is nothing really new in how data gets wrapped and passed around here,
although it still seems more ugly then it should be. The
GeofencingRegion/WebGeofencingRegion/WebGeofencingRegionPrivate classes are
modelled directly after the CryptoKey/WebCryptoKey/WebCryptoKeyPrivate classes,
and the regionType and templated to/constTo methods are more or less direct
copies of similar code in WebNode.
The combination of those two results in code that is uglier than I would like
though (in particular the webRegion().toConst<> calls in the various
CircularRegion methods seems kind of icky).

So maybe I should go back to the drawing board and come up with a
different/better approach.

(this CL replaces an earlier attempt at this in
https://codereview.chromium.org/393533002/)

[esprehn, 2014-08-14 00:44:53]

l-g-t-m from me, but I think abarth@ should look over it too since he's more
familiar with the platform/ and public/ interface.

https://codereview.chromium.org/464073002/diff/20001/Source/platform/exported...
File Source/platform/exported/WebCircularRegionPrivate.h (right):

https://codereview.chromium.org/464073002/diff/20001/Source/platform/exported...
Source/platform/exported/WebCircularRegionPrivate.h:18: , radius(radius) { }
put { } on the next line.

[haraken, 2014-08-14 00:47:21]

https://codereview.chromium.org/464073002/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/custom/V8GeofencingRegionCustom.cpp (right):

https://codereview.chromium.org/464073002/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/custom/V8GeofencingRegionCustom.cpp:13:
v8::Handle<v8::Object> wrap(GeofencingRegion* region, v8::Handle<v8::Object>
creationContext, v8::Isolate* isolate)

Can we avoid introducing this custom binding by using [SpecialWrapFor] IDL
attribute?

See Blob.idl, Element.idl, Document.idl about how [SpecialWrapFor] works.

[Marijn Kruisselbrink, 2014-08-14 16:03:32]

https://codereview.chromium.org/464073002/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/custom/V8GeofencingRegionCustom.cpp (right):

https://codereview.chromium.org/464073002/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/custom/V8GeofencingRegionCustom.cpp:13:
v8::Handle<v8::Object> wrap(GeofencingRegion* region, v8::Handle<v8::Object>
creationContext, v8::Isolate* isolate)
On 2014/08/14 00:47:21, haraken wrote:
> 
> Can we avoid introducing this custom binding by using [SpecialWrapFor] IDL
> attribute?
> 
> See Blob.idl, Element.idl, Document.idl about how [SpecialWrapFor] works.
Ah yes, that's much nicer. Thanks.

https://codereview.chromium.org/464073002/diff/20001/Source/platform/exported...
File Source/platform/exported/WebCircularRegionPrivate.h (right):

https://codereview.chromium.org/464073002/diff/20001/Source/platform/exported...
Source/platform/exported/WebCircularRegionPrivate.h:18: , radius(radius) { }
On 2014/08/14 00:44:52, esprehn wrote:
> put { } on the next line.

Done.

[esprehn, 2014-08-19 01:15:59]

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

https://codereview.chromium.org/464073002/diff/60001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.cpp:22: // Copy of same class in
ServiceWorkerContainer.cpp
How can we fix this code duplication?

https://codereview.chromium.org/464073002/diff/60001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.cpp:103: }
no braces

https://codereview.chromium.org/464073002/diff/60001/Source/modules/geofencin...
File Source/modules/geofencing/GeofencingRegion.cpp (right):

https://codereview.chromium.org/464073002/diff/60001/Source/modules/geofencin...
Source/modules/geofencing/GeofencingRegion.cpp:19: ASSERT_NOT_REACHED();
Don't add a default case, if you leave it out then the build will fail if you
forget a case. Put the ASSERT_NOT_REACHED outside the switch.

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

https://codereview.chromium.org/464073002/diff/60001/Source/modules/geofencin...
Source/modules/geofencing/WorkerNavigatorGeofencing.cpp:38: return
WorkerNavigatorGeofencing::from(navigator).geofencing();
reference?

https://codereview.chromium.org/464073002/diff/60001/Source/modules/geofencin...
Source/modules/geofencing/WorkerNavigatorGeofencing.cpp:45: return
m_geofencing.get();
Return a reference?

https://codereview.chromium.org/464073002/diff/60001/Source/platform/exported...
File Source/platform/exported/WebCircularRegionPrivate.h (right):

https://codereview.chromium.org/464073002/diff/60001/Source/platform/exported...
Source/platform/exported/WebCircularRegionPrivate.h:12: class
WebCircularRegionPrivate : public WebGeofencingRegionPrivate {
FINAL?

https://codereview.chromium.org/464073002/diff/60001/Source/platform/exported...
Source/platform/exported/WebCircularRegionPrivate.h:25: virtual
WebGeofencingRegion::RegionType regionType() const { return
WebGeofencingRegion::CircularRegionType; }
OVERRIDE

https://codereview.chromium.org/464073002/diff/60001/Source/platform/exported...
File Source/platform/exported/WebGeofencingRegionPrivate.h (right):

https://codereview.chromium.org/464073002/diff/60001/Source/platform/exported...
Source/platform/exported/WebGeofencingRegionPrivate.h:16:
WebGeofencingRegionPrivate(const WebString& id)
explicit

https://codereview.chromium.org/464073002/diff/60001/Source/platform/exported...
Source/platform/exported/WebGeofencingRegionPrivate.h:17: : id(id) { }
braces on the next line.

https://codereview.chromium.org/464073002/diff/60001/public/platform/WebCircu...
File public/platform/WebCircularRegion.h (right):

https://codereview.chromium.org/464073002/diff/60001/public/platform/WebCircu...
public/platform/WebCircularRegion.h:16: class WebCircularRegion : public
WebGeofencingRegion {
FINAL

https://codereview.chromium.org/464073002/diff/60001/public/platform/WebCircu...
public/platform/WebCircularRegion.h:32: friend class WebGeofencingRegion;
Why do you need friends?

[Marijn Kruisselbrink, 2014-08-19 18:55:14]

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

https://codereview.chromium.org/464073002/diff/60001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.cpp:22: // Copy of same class in
ServiceWorkerContainer.cpp
On 2014/08/19 01:15:59, esprehn wrote:
> How can we fix this code duplication?
By adding more explicit support to WebCallbacks and CallbackPromiseAdapter for
promises that resolve to undefined probably. The situation in
ServiceWorkerContainer and ServiceWorkerRegistration is actually worse, since
both those classes define their own blink::UndefinedValue class, neither of them
in an anonymous namespace. Doing that would also solve the "must always pass
null to onSuccess" comments that are sprinkled throughout the serviceworker
code.

https://codereview.chromium.org/464073002/diff/60001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.cpp:103: }
On 2014/08/19 01:15:59, esprehn wrote:
> no braces

Done.

https://codereview.chromium.org/464073002/diff/60001/Source/modules/geofencin...
File Source/modules/geofencing/GeofencingRegion.cpp (right):

https://codereview.chromium.org/464073002/diff/60001/Source/modules/geofencin...
Source/modules/geofencing/GeofencingRegion.cpp:19: ASSERT_NOT_REACHED();
On 2014/08/19 01:15:59, esprehn wrote:
> Don't add a default case, if you leave it out then the build will fail if you
> forget a case. Put the ASSERT_NOT_REACHED outside the switch.

Done.

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

https://codereview.chromium.org/464073002/diff/60001/Source/modules/geofencin...
Source/modules/geofencing/WorkerNavigatorGeofencing.cpp:38: return
WorkerNavigatorGeofencing::from(navigator).geofencing();
On 2014/08/19 01:15:59, esprehn wrote:
> reference?
Doesn't the generated bindings code expect this to return a pointer?

https://codereview.chromium.org/464073002/diff/60001/Source/modules/geofencin...
Source/modules/geofencing/WorkerNavigatorGeofencing.cpp:45: return
m_geofencing.get();
On 2014/08/19 01:15:59, esprehn wrote:
> Return a reference?
Doesn't the generated bindings code expect the static method to return a
pointer, in which case it doesn't seem to make sense to first convert the
pointer to a reference, and then turn it back into a pointer in the static
geofencing getter.

https://codereview.chromium.org/464073002/diff/60001/Source/platform/exported...
File Source/platform/exported/WebCircularRegionPrivate.h (right):

https://codereview.chromium.org/464073002/diff/60001/Source/platform/exported...
Source/platform/exported/WebCircularRegionPrivate.h:12: class
WebCircularRegionPrivate : public WebGeofencingRegionPrivate {
On 2014/08/19 01:15:59, esprehn wrote:
> FINAL?

Done.

https://codereview.chromium.org/464073002/diff/60001/Source/platform/exported...
Source/platform/exported/WebCircularRegionPrivate.h:25: virtual
WebGeofencingRegion::RegionType regionType() const { return
WebGeofencingRegion::CircularRegionType; }
On 2014/08/19 01:15:59, esprehn wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/464073002/diff/60001/Source/platform/exported...
File Source/platform/exported/WebGeofencingRegionPrivate.h (right):

https://codereview.chromium.org/464073002/diff/60001/Source/platform/exported...
Source/platform/exported/WebGeofencingRegionPrivate.h:16:
WebGeofencingRegionPrivate(const WebString& id)
On 2014/08/19 01:15:59, esprehn wrote:
> explicit

Done.

https://codereview.chromium.org/464073002/diff/60001/Source/platform/exported...
Source/platform/exported/WebGeofencingRegionPrivate.h:17: : id(id) { }
On 2014/08/19 01:15:59, esprehn wrote:
> braces on the next line.

Done.

https://codereview.chromium.org/464073002/diff/60001/public/platform/WebCircu...
File public/platform/WebCircularRegion.h (right):

https://codereview.chromium.org/464073002/diff/60001/public/platform/WebCircu...
public/platform/WebCircularRegion.h:16: class WebCircularRegion : public
WebGeofencingRegion {
On 2014/08/19 01:15:59, esprehn wrote:
> FINAL

Done.

https://codereview.chromium.org/464073002/diff/60001/public/platform/WebCircu...
public/platform/WebCircularRegion.h:32: friend class WebGeofencingRegion;
On 2014/08/19 01:15:59, esprehn wrote:
> Why do you need friends?
because WebGeofencingRegion::to and constTo create null instances before
assigning a value. But there really is no reason for that constructor to be
private, which eliminates the need for the friend.

[Marijn Kruisselbrink, 2014-08-19 18:58:50]

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

https://codereview.chromium.org/464073002/diff/60001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.cpp:22: // Copy of same class in
ServiceWorkerContainer.cpp
On 2014/08/19 18:55:13, Marijn Kruisselbrink wrote:
> On 2014/08/19 01:15:59, esprehn wrote:
> > How can we fix this code duplication?
> By adding more explicit support to WebCallbacks and CallbackPromiseAdapter for
> promises that resolve to undefined probably. The situation in
> ServiceWorkerContainer and ServiceWorkerRegistration is actually worse, since
> both those classes define their own blink::UndefinedValue class, neither of
them
> in an anonymous namespace. Doing that would also solve the "must always pass
> null to onSuccess" comments that are sprinkled throughout the serviceworker
> code.
Actually it seems the WebCallbacks side of things already got fixed for that, so
it's just CallbackPromiseAdapter that would need to be fixed, which shouldn't be
hard either.

[Marijn Kruisselbrink, 2014-08-19 19:13:07]

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

https://codereview.chromium.org/464073002/diff/60001/Source/modules/geofencin...
Source/modules/geofencing/Geofencing.cpp:22: // Copy of same class in
ServiceWorkerContainer.cpp
On 2014/08/19 18:58:50, Marijn Kruisselbrink wrote:
> On 2014/08/19 18:55:13, Marijn Kruisselbrink wrote:
> > On 2014/08/19 01:15:59, esprehn wrote:
> > > How can we fix this code duplication?
> > By adding more explicit support to WebCallbacks and CallbackPromiseAdapter
for
> > promises that resolve to undefined probably. The situation in
> > ServiceWorkerContainer and ServiceWorkerRegistration is actually worse,
since
> > both those classes define their own blink::UndefinedValue class, neither of
> them
> > in an anonymous namespace. Doing that would also solve the "must always pass
> > null to onSuccess" comments that are sprinkled throughout the serviceworker
> > code.
> Actually it seems the WebCallbacks side of things already got fixed for that,
so
> it's just CallbackPromiseAdapter that would need to be fixed, which shouldn't
be
> hard either.
In my currently last patchset I got rid of this UndefinedValue class by adding
support for void to CallbackPromiseAdapter. The duplication of most of that
class seems a bit icky, but I'm not sure if there are better template tricks to
do what I want. Also maybe this part should be a separate CL, and certainly
porting the Service Worker code to get rid of the UndefinedValue classes will be
a separate CL as that will involve changing code on both the blink and content
sides.

[esprehn, 2014-08-19 19:24:53]

Yeah can we do the v8/ change in a separate CL?

https://codereview.chromium.org/464073002/diff/120001/Source/bindings/core/v8...
File Source/bindings/core/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/464073002/diff/120001/Source/bindings/core/v8...
Source/bindings/core/v8/CallbackPromiseAdapter.h:114:
CallbackPromiseAdapter(PassRefPtr<ScriptPromiseResolver> resolver)
explicit

[Marijn Kruisselbrink, 2014-08-19 20:45:41]

On 2014/08/19 19:24:53, esprehn wrote:
> Yeah can we do the v8/ change in a separate CL?
moved the v8 change to https://codereview.chromium.org/468913007/ (which
currently depends on this one, but it probably makes sense to reverse that
dependency).

[Marijn Kruisselbrink, 2014-08-21 17:50:39]

On 2014/08/19 20:45:41, Marijn Kruisselbrink wrote:
> On 2014/08/19 19:24:53, esprehn wrote:
> > Yeah can we do the v8/ change in a separate CL?
> moved the v8 change to https://codereview.chromium.org/468913007/ (which
> currently depends on this one, but it probably makes sense to reverse that
> dependency).
And I landed those v8 changes and rebased this CL to make use of the new void
support.
PTAL

[esprehn, 2014-08-21 21:59:45]

lgtm

[abarth-chromium, 2014-08-23 05:43:35]

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
File Source/modules/geofencing/CircularRegion.cpp (right):

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
Source/modules/geofencing/CircularRegion.cpp:27: CircularRegionInit rinit(init);
rinit -> please use complete words in variable names.

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
Source/modules/geofencing/CircularRegion.cpp:28: return new
CircularRegion(WebCircularRegion::create(rinit.id, rinit.latitude,
rinit.longitude, rinit.radius));
So much mallocing.  Can't WebCircularRegion just be a struct that contains its
data?

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
Source/modules/geofencing/CircularRegion.cpp:39: return
webRegion().toConst<WebCircularRegion>().latitude();
So ugly...

Why not just have WebCircularRegion be a struct and have CircularRegion hold a
pointer to that struct?

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
File Source/modules/geofencing/CircularRegion.h (right):

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
Source/modules/geofencing/CircularRegion.h:39: friend class GeofencingRegion;
Please don't use friends.

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
File Source/modules/geofencing/Geofencing.cpp (right):

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
Source/modules/geofencing/Geofencing.cpp:27: OwnPtr<WebType> webRegions =
adoptPtr(regionsRaw);
We try to push the adoptPtr as close to the API as possible so that we can have
leak / double-free protection inside Blink.

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
Source/modules/geofencing/Geofencing.cpp:30:
regions.append(GeofencingRegion::createFromWebRegion((*webRegions)[i]));
s/createFromWebRegion/create/

C++ can dispatch the call based on the type of the argument.

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
Source/modules/geofencing/Geofencing.cpp:38: }
We shouldn't have this.  As soon as we get the region from the API, we should
call adoptPtr on it and we should let OwnPtr call |delete| for us.

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
File Source/modules/geofencing/GeofencingRegion.idl (right):

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
Source/modules/geofencing/GeofencingRegion.idl:7: SpecialWrapFor=CircularRegion,
We can't call this CircularRegion.  That might conflict with CSS regions.

https://codereview.chromium.org/464073002/diff/160001/Source/platform/exporte...
File Source/platform/exported/WebGeofencingRegionPrivate.h (right):

https://codereview.chromium.org/464073002/diff/160001/Source/platform/exporte...
Source/platform/exported/WebGeofencingRegionPrivate.h:24: const WebString id;
Why not just store this data in WebGeofencingRegion directly?

https://codereview.chromium.org/464073002/diff/160001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/464073002/diff/160001/public/platform/Platfor...
public/platform/Platform.h:634: // Geofencing
---------------------------------------------------------
Two blank lines between sections pls

https://codereview.chromium.org/464073002/diff/160001/public/platform/WebCirc...
File public/platform/WebCircularRegion.h (right):

https://codereview.chromium.org/464073002/diff/160001/public/platform/WebCirc...
public/platform/WebCircularRegion.h:16: class WebCircularRegion FINAL : public
WebGeofencingRegion {
Are we not using "geo" in these names?  We don't want to conflict with CSS
regions.

https://codereview.chromium.org/464073002/diff/160001/public/platform/WebGeof...
File public/platform/WebGeofencingProvider.h (right):

https://codereview.chromium.org/464073002/diff/160001/public/platform/WebGeof...
public/platform/WebGeofencingProvider.h:18: typedef
WebCallbacks<WebVector<WebGeofencingRegion>, WebGeofencingError>
WebGeofencingRegionsCallbacks;
Why callbacks and not a virtual interface?

https://codereview.chromium.org/464073002/diff/160001/public/platform/WebGeof...
File public/platform/WebGeofencingRegion.h (right):

https://codereview.chromium.org/464073002/diff/160001/public/platform/WebGeof...
public/platform/WebGeofencingRegion.h:34: T res;
s/res/result/

Please use complete words in variable names.

https://codereview.chromium.org/464073002/diff/160001/public/platform/WebGeof...
public/platform/WebGeofencingRegion.h:34: T res;
s/res/result/

Please use complete words in variable names.

[Marijn Kruisselbrink, 2014-08-25 22:25:25]

Thanks for the review. I'm still not entirely sure what the best process is to
implement a reasonably complicated feature like this in blink, with many
non-obvious design choices, so I'm not always sure what the best way is to get
feedback on how to best do something.

I think I addressed most of your comments, the rest are about the actual design
of the GeofencingRegion, WebGeofencingRegion, WebGeofencingRegionPrivate
classes. First of all, I'm not happy with this design as it is either. This CL
started out as me asking if this was a sane approach at all, or if I should try
thinking of a better way to do things. In the design document I linked to I
briefly describe one alternative aproach, and why I didn't pick that option, but
you're suggesting something else.

What I think you're suggesting is that the classes that currently are
WebGeofencingRegionPrivate/WebCircularGeofencingRegionPrivate should directly be
WebGeofencingRegion and WebCircularGeofencingRegion. The main reason I didn't do
it like that is that this makes ownership/lifetime of these classes a little bit
tricky. I think this would mean that WebGeofencingProvider::registerRegion is
passed a pointer to a WebGeofencingRegion that is only valid for the duration of
the registerRegion call, and if the provider implementation needs to hold on to
it longer it needs to make a copy.
I'm not sure what the best ownership semantics for getRegisteredRegions would be
though. It might make sense for ownership of the returned WebGeofencingRegion's
to be transfered to the GeofencingRegion instance they'll be wrapped in, or it
might make sense for the blink code to create a copy and wrap that in a
GeofencingRegion. But in all of these cases it seems like the only way to
express the desired ownership semantics is in comments, so it's easy to write
code that does this wrong (maybe I'm missing something here).

I'm also not sure which of your concerns this actually would address. It
wouldn't really make the getters in CircularGeofencingRegion any less ugly
(there would still be a need to cast the WebGeofencingRegion instance that is
stored in GeofencingRegion to a WebCircularGeofencingRegion), and it wouldn't
really reduce any mallocing. Currently only the Web*GeofencingRegionPrivate
classes are heap allocated (and the garbage collected *GeofencingRegion classes
of course), and that would just shift to the Web*GeofencingRegion classes.

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
File Source/modules/geofencing/CircularRegion.cpp (right):

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
Source/modules/geofencing/CircularRegion.cpp:27: CircularRegionInit rinit(init);
On 2014/08/23 05:43:34, abarth wrote:
> rinit -> please use complete words in variable names.

Done.

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
File Source/modules/geofencing/CircularRegion.h (right):

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
Source/modules/geofencing/CircularRegion.h:39: friend class GeofencingRegion;
On 2014/08/23 05:43:34, abarth wrote:
> Please don't use friends.

Done.

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
File Source/modules/geofencing/Geofencing.cpp (right):

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
Source/modules/geofencing/Geofencing.cpp:27: OwnPtr<WebType> webRegions =
adoptPtr(regionsRaw);
On 2014/08/23 05:43:34, abarth wrote:
> We try to push the adoptPtr as close to the API as possible so that we can
have
> leak / double-free protection inside Blink.
This and the other comment about dispose seem more to be complaints about the
existence of CallbackPromiseAdapter than of my usage of it? As far as I can tell
my usage here is exactly as the "To use" section in the documentation for that
class.
(okay, I should probably add a comment indicating that this is implementing the
API CallbackPromiseAdapter requires)

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
Source/modules/geofencing/Geofencing.cpp:30:
regions.append(GeofencingRegion::createFromWebRegion((*webRegions)[i]));
On 2014/08/23 05:43:34, abarth wrote:
> s/createFromWebRegion/create/
> 
> C++ can dispatch the call based on the type of the argument.

Done.

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
Source/modules/geofencing/Geofencing.cpp:38: }
On 2014/08/23 05:43:34, abarth wrote:
> We shouldn't have this.  As soon as we get the region from the API, we should
> call adoptPtr on it and we should let OwnPtr call |delete| for us.
Same reply as to your comment on my ::take implementation. This seems to be a
complaint about CallbackPromiseAdapter, not about me using it.

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
File Source/modules/geofencing/GeofencingRegion.idl (right):

https://codereview.chromium.org/464073002/diff/160001/Source/modules/geofenci...
Source/modules/geofencing/GeofencingRegion.idl:7: SpecialWrapFor=CircularRegion,
On 2014/08/23 05:43:34, abarth wrote:
> We can't call this CircularRegion.  That might conflict with CSS regions.

Done.

https://codereview.chromium.org/464073002/diff/160001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/464073002/diff/160001/public/platform/Platfor...
public/platform/Platform.h:634: // Geofencing
---------------------------------------------------------
On 2014/08/23 05:43:34, abarth wrote:
> Two blank lines between sections pls

Done.

https://codereview.chromium.org/464073002/diff/160001/public/platform/WebCirc...
File public/platform/WebCircularRegion.h (right):

https://codereview.chromium.org/464073002/diff/160001/public/platform/WebCirc...
public/platform/WebCircularRegion.h:16: class WebCircularRegion FINAL : public
WebGeofencingRegion {
On 2014/08/23 05:43:34, abarth wrote:
> Are we not using "geo" in these names?  We don't want to conflict with CSS
> regions.

Done.

https://codereview.chromium.org/464073002/diff/160001/public/platform/WebGeof...
File public/platform/WebGeofencingProvider.h (right):

https://codereview.chromium.org/464073002/diff/160001/public/platform/WebGeof...
public/platform/WebGeofencingProvider.h:18: typedef
WebCallbacks<WebVector<WebGeofencingRegion>, WebGeofencingError>
WebGeofencingRegionsCallbacks;
On 2014/08/23 05:43:34, abarth wrote:
> Why callbacks and not a virtual interface?
I'm not sure what kind of virtual interface you would expect, other than the
virtual interface with onSuccess() and onError(...) methods that WebCallbacks
is?
Also every other promise based API I've come across seems to use WebCallbacks
and CallbackPromiseAdapter to deal with resolving/rejecting the promise (the one
exception being the crypto API and its WebCryptoResult class, although I
wouldn't call that a virtual interface either).

https://codereview.chromium.org/464073002/diff/160001/public/platform/WebGeof...
File public/platform/WebGeofencingRegion.h (right):

https://codereview.chromium.org/464073002/diff/160001/public/platform/WebGeof...
public/platform/WebGeofencingRegion.h:34: T res;
On 2014/08/23 05:43:34, abarth wrote:
> s/res/result/
> 
> Please use complete words in variable names.
Done, that's what I get for copying existing blink code...

[Marijn Kruisselbrink, 2014-08-28 16:24:07]

abarth: ping

[Marijn Kruisselbrink, 2014-08-29 23:40:05]

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

[Marijn Kruisselbrink, 2014-08-29 23:40:06]

As suggested by abarth@, adding jochen as a reviewer.
jochen: Hopefully you'll have more time to look at this than abart@

[jochen (gone - plz use gerrit), 2014-09-02 09:14:51]

Can you help me understand why the regions need to be classes as opposed to
PODs?

I guess you'll want to persist the regions somehow in the browser, right? In
that case, we need to IPC them around and serialize/deserialize them somehow
which is far easier for POD data types.

[abarth-chromium, 2014-09-02 19:38:21]

abarth@chromium.org changed reviewers:
- abarth@chromium.org

[Marijn Kruisselbrink, 2014-09-03 03:27:52]

On 2014/09/02 09:14:51, jochen wrote:
> Can you help me understand why the regions need to be classes as opposed to
> PODs?
> 
> I guess you'll want to persist the regions somehow in the browser, right? In
> that case, we need to IPC them around and serialize/deserialize them somehow
> which is far easier for POD data types.
Currently this CL only supports one type of region, but it seems fairly likely
that whatever spec we'll eventually end up with will want to support more than
just simple circular regions. So assuming I want to support multiple region
types that leaves me two options for the blink<->content API; I could have
separate registerCircularRegion, registerPolygonRegion etc methods and have
simple POD data types for each region type, or I could have a base region type
which other regions are derived from. And I'm not even sure how the first option
with separate POD data types would work when it comes to getRegisteredRegions
returning a list of regions, which are not all the same type. I would probably
end up having to have something like this:
struct GeofencingRegion { string id; enum RegionType {} regionType; }; struct
CircularGeofencingRegion { GeofencingRegion region; double latitude; ... }; and
then have getRegisteredRegions pass a list of pointers to GeofencingRegion to
blink. Blink code would then have to check the regionType attributes to figure
out the actual region types and do the appropriate casts to even be able to
correctly delete these structs.
Also see my reponse to adams review where I'm raising some more questions about
what it would actually mean to remove some of the layers of
indirection/wrapping.

But yes, maybe it's better (it's definitely easier) to temporarily ignore the
problem caused by trying to support multiple region types, but it might later
result in having to significantly change the design, which I was trying to
avoid.

[jochen (gone - plz use gerrit), 2014-09-03 08:32:17]

On 2014/09/03 at 03:27:52, mek wrote:
> On 2014/09/02 09:14:51, jochen wrote:
> > Can you help me understand why the regions need to be classes as opposed to
> > PODs?
> > 
> > I guess you'll want to persist the regions somehow in the browser, right? In
> > that case, we need to IPC them around and serialize/deserialize them somehow
> > which is far easier for POD data types.
> Currently this CL only supports one type of region, but it seems fairly likely
that whatever spec we'll eventually end up with will want to support more than
just simple circular regions. So assuming I want to support multiple region
types that leaves me two options for the blink<->content API; I could have
separate registerCircularRegion, registerPolygonRegion etc methods and have
simple POD data types for each region type, or I could have a base region type
which other regions are derived from. And I'm not even sure how the first option
with separate POD data types would work when it comes to getRegisteredRegions
returning a list of regions, which are not all the same type. I would probably
end up having to have something like this:
> struct GeofencingRegion { string id; enum RegionType {} regionType; }; struct
CircularGeofencingRegion { GeofencingRegion region; double latitude; ... }; and
then have getRegisteredRegions pass a list of pointers to GeofencingRegion to
blink. Blink code would then have to check the regionType attributes to figure
out the actual region types and do the appropriate casts to even be able to
correctly delete these structs.
> Also see my reponse to adams review where I'm raising some more questions
about what it would actually mean to remove some of the layers of
indirection/wrapping.
> 
> But yes, maybe it's better (it's definitely easier) to temporarily ignore the
problem caused by trying to support multiple region types, but it might later
result in having to significantly change the design, which I was trying to
avoid.

yes, let's go down that road first.

[Marijn Kruisselbrink, 2014-09-03 23:06:17]

Patchset #10 (id:200001) has been deleted

[Marijn Kruisselbrink, 2014-09-03 23:19:36]

On 2014/09/03 08:32:17, jochen wrote:
> On 2014/09/03 at 03:27:52, mek wrote:
> > On 2014/09/02 09:14:51, jochen wrote:
> > > Can you help me understand why the regions need to be classes as opposed
to
> > > PODs?
> > > 
> > > I guess you'll want to persist the regions somehow in the browser, right?
In
> > > that case, we need to IPC them around and serialize/deserialize them
somehow
> > > which is far easier for POD data types.
> > Currently this CL only supports one type of region, but it seems fairly
likely
> that whatever spec we'll eventually end up with will want to support more than
> just simple circular regions. So assuming I want to support multiple region
> types that leaves me two options for the blink<->content API; I could have
> separate registerCircularRegion, registerPolygonRegion etc methods and have
> simple POD data types for each region type, or I could have a base region type
> which other regions are derived from. And I'm not even sure how the first
option
> with separate POD data types would work when it comes to getRegisteredRegions
> returning a list of regions, which are not all the same type. I would probably
> end up having to have something like this:
> > struct GeofencingRegion { string id; enum RegionType {} regionType; };
struct
> CircularGeofencingRegion { GeofencingRegion region; double latitude; ... };
and
> then have getRegisteredRegions pass a list of pointers to GeofencingRegion to
> blink. Blink code would then have to check the regionType attributes to figure
> out the actual region types and do the appropriate casts to even be able to
> correctly delete these structs.
> > Also see my reponse to adams review where I'm raising some more questions
> about what it would actually mean to remove some of the layers of
> indirection/wrapping.
> > 
> > But yes, maybe it's better (it's definitely easier) to temporarily ignore
the
> problem caused by trying to support multiple region types, but it might later
> result in having to significantly change the design, which I was trying to
> avoid.
> 
> yes, let's go down that road first.
Okay, I changed the public blink API to only support circular regions, and use a
simple struct for those. This struct isn't a POD, since it contains a WebString,
so it won't directly help me with the IPC/serialization issues (and I'll still
need a separate structure in the content layer for the same data).
PTAL

[jochen (gone - plz use gerrit), 2014-09-04 11:14:54]

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
File Source/modules/geofencing/CircularGeofencingRegion.cpp (right):

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
Source/modules/geofencing/CircularGeofencingRegion.cpp:41: :
GeofencingRegion(region.id), m_latitude(region.latitude),
m_longitude(region.longitude), m_radius(region.radius)
nit. initializers on separate lines

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
File Source/modules/geofencing/CircularGeofencingRegion.h (right):

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
Source/modules/geofencing/CircularGeofencingRegion.h:36: double m_latitude;
you could consider to just use a WebCirculerGeofencingRegion m_region

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
File Source/modules/geofencing/Geofencing.cpp (right):

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
Source/modules/geofencing/Geofencing.cpp:32: for (size_t i = 0; i <
webRegions->size(); ++i) {
nit, no { }

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
File Source/modules/geofencing/WorkerNavigatorGeofencing.h (right):

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
Source/modules/geofencing/WorkerNavigatorGeofencing.h:17: : public
NoBaseWillBeGarbageCollectedFinalized<WorkerNavigatorGeofencing>
nit. base classes should all go in one line, in line 16

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
Source/modules/geofencing/WorkerNavigatorGeofencing.h:20: 
nit wtf_make_noncopyable

https://codereview.chromium.org/464073002/diff/220001/public/platform/WebCirc...
File public/platform/WebCircularGeofencingRegion.h (right):

https://codereview.chromium.org/464073002/diff/220001/public/platform/WebCirc...
public/platform/WebCircularGeofencingRegion.h:14: WebString id;
i wouldn't put the ID in the region (the IDL doesn't have it either). Also, e.g.
the remove call only takes the ID but not the rest of the region, so it strikes
me as two separate concepts.

https://codereview.chromium.org/464073002/diff/220001/public/platform/WebGeof...
File public/platform/WebGeofencingProvider.h (right):

https://codereview.chromium.org/464073002/diff/220001/public/platform/WebGeof...
public/platform/WebGeofencingProvider.h:26: virtual void registerRegion(const
WebCircularGeofencingRegion&, WebGeofencingCallbacks*) = 0;
nit. add explicit ID parameter

[Marijn Kruisselbrink, 2014-09-04 17:44:19]

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
File Source/modules/geofencing/CircularGeofencingRegion.cpp (right):

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
Source/modules/geofencing/CircularGeofencingRegion.cpp:41: :
GeofencingRegion(region.id), m_latitude(region.latitude),
m_longitude(region.longitude), m_radius(region.radius)
On 2014/09/04 11:14:54, jochen wrote:
> nit. initializers on separate lines

Done.

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
File Source/modules/geofencing/CircularGeofencingRegion.h (right):

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
Source/modules/geofencing/CircularGeofencingRegion.h:36: double m_latitude;
On 2014/09/04 11:14:54, jochen wrote:
> you could consider to just use a WebCirculerGeofencingRegion m_region

Done.

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
File Source/modules/geofencing/Geofencing.cpp (right):

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
Source/modules/geofencing/Geofencing.cpp:32: for (size_t i = 0; i <
webRegions->size(); ++i) {
On 2014/09/04 11:14:54, jochen wrote:
> nit, no { }

Done.

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
File Source/modules/geofencing/WorkerNavigatorGeofencing.h (right):

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
Source/modules/geofencing/WorkerNavigatorGeofencing.h:17: : public
NoBaseWillBeGarbageCollectedFinalized<WorkerNavigatorGeofencing>
On 2014/09/04 11:14:54, jochen wrote:
> nit. base classes should all go in one line, in line 16
That doesn't actually seem to be a rule? And it's certainly not something that
is consistently adhered to in existing blink code (ServiceWorkerContainer,
ServiceWorkerContainerClient, ServiceWorkerRegistration and a bunch of other
classes all have one base class per line).

So am I now supposed to reformat all my code to follow this unwritten style rule
(including code like NavigatorGeofencing which isn't otherwise touched in this
CL)?

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
Source/modules/geofencing/WorkerNavigatorGeofencing.h:20: 
On 2014/09/04 11:14:54, jochen wrote:
> nit wtf_make_noncopyable

Done.

https://codereview.chromium.org/464073002/diff/220001/public/platform/WebCirc...
File public/platform/WebCircularGeofencingRegion.h (right):

https://codereview.chromium.org/464073002/diff/220001/public/platform/WebCirc...
public/platform/WebCircularGeofencingRegion.h:14: WebString id;
On 2014/09/04 11:14:54, jochen wrote:
> i wouldn't put the ID in the region (the IDL doesn't have it either). Also,
e.g.
> the remove call only takes the ID but not the rest of the region, so it
strikes
> me as two separate concepts.
The IDL does have the ID in the region. In fact this (should the ID be part of
the region or not) was exactly one of the questions I raised the several times I
send out draft specs and IDL files to blink-dev and other lists. Since nobody
ever commented on that particular part of my draft spec I didn't change it. Some
things would definitely be easier if the ID would not be part of the region, but
it would require changing several other parts of the API as well (for one
getRegisteredRegions would now need to return a map instead of a list).
Also I'm not sure if a code review is the best place to discuss API details like
this. API discussions already get fragmented enough between blink-dev, w3c
working groups and private email, without introducing yet another medium.

(but yes, not having a string in WebCircularGeofencingRegion would be somewhat
tempting purely from an implementation point of view, but I think it's wrong to
base decisions for the public API purely on what is marginally easier to
implement)

Having said that, I do agree that it might make the API clearer to pass the ID
separately, but so far in the several months of discussing this API nobody has
ever seemed to agree with that/have an opinion on that...

[jochen (gone - plz use gerrit), 2014-09-05 14:08:29]

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
File Source/modules/geofencing/WorkerNavigatorGeofencing.h (right):

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
Source/modules/geofencing/WorkerNavigatorGeofencing.h:17: : public
NoBaseWillBeGarbageCollectedFinalized<WorkerNavigatorGeofencing>
On 2014/09/04 17:44:19, Marijn Kruisselbrink wrote:
> On 2014/09/04 11:14:54, jochen wrote:
> > nit. base classes should all go in one line, in line 16
> That doesn't actually seem to be a rule? And it's certainly not something that
> is consistently adhered to in existing blink code (ServiceWorkerContainer,
> ServiceWorkerContainerClient, ServiceWorkerRegistration and a bunch of other
> classes all have one base class per line).

Well, whenever I do a review, I ask for the code to be formatted like this. If
you go to the public/ directory for example, there's not a single example where
not all base classes are on one line


> 
> So am I now supposed to reformat all my code to follow this unwritten style
rule
> (including code like NavigatorGeofencing which isn't otherwise touched in this
> CL)?

not in this CL, but please try to stick to it in the future

https://codereview.chromium.org/464073002/diff/220001/public/platform/WebCirc...
File public/platform/WebCircularGeofencingRegion.h (right):

https://codereview.chromium.org/464073002/diff/220001/public/platform/WebCirc...
public/platform/WebCircularGeofencingRegion.h:14: WebString id;
On 2014/09/04 17:44:19, Marijn Kruisselbrink wrote:
> Having said that, I do agree that it might make the API clearer to pass the ID
> separately, but so far in the several months of discussing this API nobody has
> ever seemed to agree with that/have an opinion on that...

I'm sorry, I guess I should first read up on this a bit more.

[Marijn Kruisselbrink, 2014-09-05 18:32:09]

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
File Source/modules/geofencing/WorkerNavigatorGeofencing.h (right):

https://codereview.chromium.org/464073002/diff/220001/Source/modules/geofenci...
Source/modules/geofencing/WorkerNavigatorGeofencing.h:17: : public
NoBaseWillBeGarbageCollectedFinalized<WorkerNavigatorGeofencing>
On 2014/09/05 14:08:28, jochen wrote:
> On 2014/09/04 17:44:19, Marijn Kruisselbrink wrote:
> > On 2014/09/04 11:14:54, jochen wrote:
> > > nit. base classes should all go in one line, in line 16
> > That doesn't actually seem to be a rule? And it's certainly not something
that
> > is consistently adhered to in existing blink code (ServiceWorkerContainer,
> > ServiceWorkerContainerClient, ServiceWorkerRegistration and a bunch of other
> > classes all have one base class per line).
> 
> Well, whenever I do a review, I ask for the code to be formatted like this. If
> you go to the public/ directory for example, there's not a single example
where
> not all base classes are on one line
It's just rather frustrating when different reviewers have different
(undocumented) coding styles they enforce. In public/ there is not a single
class with multiple inheritance, yes. In Source/, especially in the various more
recent modules, there are many classes with each base class on a separate line
(geolocation, indexeddb, mediasource, mediastream, netinfo, screen_orientation,
serviceworkers, vibration all have one base class per line for at least some of
their code).

https://codereview.chromium.org/464073002/diff/220001/public/platform/WebCirc...
File public/platform/WebCircularGeofencingRegion.h (right):

https://codereview.chromium.org/464073002/diff/220001/public/platform/WebCirc...
public/platform/WebCircularGeofencingRegion.h:14: WebString id;
On 2014/09/05 14:08:29, jochen wrote:
> On 2014/09/04 17:44:19, Marijn Kruisselbrink wrote:
> > Having said that, I do agree that it might make the API clearer to pass the
ID
> > separately, but so far in the several months of discussing this API nobody
has
> > ever seemed to agree with that/have an opinion on that...
> 
> I'm sorry, I guess I should first read up on this a bit more.
I now changed the blink<->content API to not have the ID as part of the region,
since it does seem to make more sense to have it separate. For now I've left the
web-exposed API alone.

[Marijn Kruisselbrink, 2014-09-09 23:07:51]

jochen: ptal

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

lgtm

I wonder whether the API should have some way for the platform to tell the
website that it modified the region a bit? e.g. it might want to round the
radius or snap the coordinates to some grid?

right now, it can just outright reject the area.

I'm also not sure that the id is a useful feature of this API. At the least, I'd
expect some restrictions, e.g. on the length of the id. A simpler API would just
tell you that some of your regions triggered and where you are maybe?

anyway, that's just some thoughts for the standard process.

https://codereview.chromium.org/464073002/diff/260001/public/platform/WebGeof...
File public/platform/WebGeofencingRegistration.h (right):

https://codereview.chromium.org/464073002/diff/260001/public/platform/WebGeof...
public/platform/WebGeofencingRegistration.h:13: struct WebGeofencingRegistration
{
can you please add a ctor to this one?

[Marijn Kruisselbrink, 2014-09-10 17:24:05]

On 2014/09/10 07:51:04, jochen wrote:
> I wonder whether the API should have some way for the platform to tell the
> website that it modified the region a bit? e.g. it might want to round the
> radius or snap the coordinates to some grid?
> 
> right now, it can just outright reject the area.
Hmm, yes, that might be a good idea. One way to allow for that would be for the
promise returned by registerRegion to resolve to the region that actually ended
up being registered.
 
> I'm also not sure that the id is a useful feature of this API. At the least,
I'd
> expect some restrictions, e.g. on the length of the id. A simpler API would
just
> tell you that some of your regions triggered and where you are maybe?
> 
> anyway, that's just some thoughts for the standard process.
Thanks for the suggestions, I'll keep them in mind!

https://codereview.chromium.org/464073002/diff/260001/public/platform/WebGeof...
File public/platform/WebGeofencingRegistration.h (right):

https://codereview.chromium.org/464073002/diff/260001/public/platform/WebGeof...
public/platform/WebGeofencingRegistration.h:13: struct WebGeofencingRegistration
{
On 2014/09/10 07:51:04, jochen wrote:
> can you please add a ctor to this one?

Done.

[Marijn Kruisselbrink, 2014-09-10 17:24:13]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-09-10 17:24:52]

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

[Marijn Kruisselbrink, 2014-09-10 17:56:49]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-09-10 17:57:21]

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

[commit-bot: I haz the power, 2014-09-10 18:59:47]

Committed patchset #14 (id:300001) as 181755