Basic implementation of GeofencingManager class.

Basic implementation of GeofencingManager class.

This class is the single entry point for all Geofencing related API calls, and will be responsible for storing all geofence registrations, and registering them with the underlying platform specific GeofencingProvider.

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

[Marijn Kruisselbrink, 2014-09-26 19:12:27]

Patchset #2 (id:20001) has been deleted

[Marijn Kruisselbrink, 2014-09-26 19:27:50]

Patchset #2 (id:40001) has been deleted

[Marijn Kruisselbrink, 2014-09-26 19:36:59]

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

[Marijn Kruisselbrink, 2014-09-26 19:37:00]

I'm not quite sure if I'm entirely happy with this CL and its interface to
platform specific implementations yet, but it's already getting pretty big, so
probably better to get reviewed separately.
I started implementing an android specific backend
inhttps://codereview.chromium.org/598313004/, but that obviously is still far
from being done.

[Marijn Kruisselbrink, 2014-10-01 00:23:58]

ping?

[Marijn Kruisselbrink, 2014-10-01 16:08:40]

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

[Marijn Kruisselbrink, 2014-10-01 16:08:40]

+peter, maybe you have some time to take a look at this?

[Marijn Kruisselbrink, 2014-10-03 03:47:46]

ping

[Michael van Ouwerkerk, 2014-10-03 12:54:41]

On 2014/10/03 03:47:46, Marijn Kruisselbrink wrote:
> ping

Apologies for the delay, I've been heads down preparing a new working draft
publication for the Push API. I hope to have some time later today.

[Marijn Kruisselbrink, 2014-10-03 19:14:21]

On 2014/10/03 12:54:41, Michael van Ouwerkerk wrote:
> On 2014/10/03 03:47:46, Marijn Kruisselbrink wrote:
> > ping
> 
> Apologies for the delay, I've been heads down preparing a new working draft
> publication for the Push API. I hope to have some time later today.
Okay, it just would be nice to get some feedback before I try to find toplevel
content/ OWNERS...
Anyway, I slightly extended this CL to keep me more sane in maintaining various
patches build on top of this, by actually adding support to GeofencingManager
for keeping registrations separate per service worker (even though nothing is
hooked up yet to actually know which service worker things are associated with).

[Michael van Ouwerkerk, 2014-10-06 16:43:42]

Could you please run the try bots?

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

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.cc:46: 0, /*
service_worker_registration_id */
I assume you need a TODO here? And below.

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.cc:50: this,
Should this be a weak pointer?

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.cc:88: GeofencingStatus
result) {
Let's call this status as you use result for other purposes elsewhere.

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
File content/browser/geofencing/geofencing_manager.h (right):

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.h:110: typedef std::map<int64,
IdRegistrationMap> Registrations;
A ServiceWorkerRegistrationId is unique per-origin. GeofencingManager is a
browser process singleton. Isn't this going to conflict when multiple renderers
with the same sw id (but different origins) lead to this map?

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
File content/browser/geofencing/geofencing_provider.h (right):

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_provider.h:22: // registration failed, the
value of |registration_id| is undefined.
I think it would be clearer to set registration_id to a known constant value on
failure.

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_provider.h:31: // Also GeofencingManager
should be making sure the total number of geofences
How would the caller know what the limit of the platform specific provider is?
You'd need to  expose that as part of the provider API, and it requires the
provider to know. An alternative would be to allow for a 'limit reached'
failure.

https://codereview.chromium.org/586163003/diff/120001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/586163003/diff/120001/content/content_browser...
content/content_browser.gypi:680: 'browser/geofencing/geofencing_manager.h',
geofencing_provider.h seems to be missing here. Where will that interface be
implemented though? If in the browser layer, it needs to become part of the
public content API

https://codereview.chromium.org/586163003/diff/120001/content/content_common....
File content/content_common.gypi (right):

https://codereview.chromium.org/586163003/diff/120001/content/content_common....
content/content_common.gypi:67: 'public/common/geofencing_region.h',
This file does not seem to exist.

[Marijn Kruisselbrink, 2014-10-06 18:38:56]

Patchset #6 (id:140001) has been deleted

[Marijn Kruisselbrink, 2014-10-06 18:58:06]

Patchset #6 (id:160001) has been deleted

[Marijn Kruisselbrink, 2014-10-06 19:14:11]

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

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.cc:46: 0, /*
service_worker_registration_id */
On 2014/10/06 16:43:42, Michael van Ouwerkerk wrote:
> I assume you need a TODO here? And below.
Sure, I can add more TODO's here as well, although it's not like I'm going to
forget to actually do this...

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.cc:50: this,
On 2014/10/06 16:43:41, Michael van Ouwerkerk wrote:
> Should this be a weak pointer?
Yes, that's probably better. Done.

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.cc:88: GeofencingStatus
result) {
On 2014/10/06 16:43:41, Michael van Ouwerkerk wrote:
> Let's call this status as you use result for other purposes elsewhere.

Done.

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
File content/browser/geofencing/geofencing_manager.h (right):

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.h:110: typedef std::map<int64,
IdRegistrationMap> Registrations;
On 2014/10/06 16:43:42, Michael van Ouwerkerk wrote:
> A ServiceWorkerRegistrationId is unique per-origin. GeofencingManager is a
> browser process singleton. Isn't this going to conflict when multiple
renderers
> with the same sw id (but different origins) lead to this map?
Hmm, yes, you're right. It does seem like this is indeed a 64 bit counter scoped
by storage partition. I somehow mistakenly assumed that the counters in
ServiceWorkerStorage were global, but of course they are scoped to a storage
partition.

For simplicity I now changed this to just storing a vector<Registration>, with a
TODO to improve the storage mechanism for better lookup/delete performance
(since all access to this list of registrations is hidden behind the
FindRegistration, AddRegistration and ClearRegistration methods anyway changing
the storage mechanism later won't require significant changes).

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
File content/browser/geofencing/geofencing_provider.h (right):

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_provider.h:22: // registration failed, the
value of |registration_id| is undefined.
On 2014/10/06 16:43:42, Michael van Ouwerkerk wrote:
> I think it would be clearer to set registration_id to a known constant value
on
> failure.
Changed the comment to document what value providers should set registration_id
to on failure, although it's probably still better for GeofencingManager to not
rely on this.

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_provider.h:31: // Also GeofencingManager
should be making sure the total number of geofences
On 2014/10/06 16:43:42, Michael van Ouwerkerk wrote:
> How would the caller know what the limit of the platform specific provider is?
> You'd need to  expose that as part of the provider API, and it requires the
> provider to know. An alternative would be to allow for a 'limit reached'
> failure.
Yes, that's something the provider API will have to expose. Until I actually get
to implementing support for this in the manager it doesn't really make sense to
expose that here though, and instead I imagine providers will indeed return some
kind of limit reached failure (which without special support in the manager will
just get passed through to the website, which isn't very helpful as long as this
is a global limit).

https://codereview.chromium.org/586163003/diff/120001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/586163003/diff/120001/content/content_browser...
content/content_browser.gypi:680: 'browser/geofencing/geofencing_manager.h',
On 2014/10/06 16:43:42, Michael van Ouwerkerk wrote:
> geofencing_provider.h seems to be missing here. Where will that interface be
> implemented though? If in the browser layer, it needs to become part of the
> public content API
Added.
For now I'm working under the assumption that the interface will be implemented
in the content layer, just like the geolocation location_provider_android etc.
If that changes a bunch of classes will have to be moved to the public content
API, and likely another layer of abstraction will be needed.

https://codereview.chromium.org/586163003/diff/120001/content/content_common....
File content/content_common.gypi (right):

https://codereview.chromium.org/586163003/diff/120001/content/content_common....
content/content_common.gypi:67: 'public/common/geofencing_region.h',
On 2014/10/06 16:43:42, Michael van Ouwerkerk wrote:
> This file does not seem to exist.
Oh yes, that's a very old remnant... removed.

[Marijn Kruisselbrink, 2014-10-07 00:59:56]

Patchset #7 (id:200001) has been deleted

[Marijn Kruisselbrink, 2014-10-08 01:23:39]

PTAL

[Michael van Ouwerkerk, 2014-10-08 16:59:01]

lgtm with a question

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

https://codereview.chromium.org/586163003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_dispatcher_host.cc:46: 0, /*
service_worker_registration_id */
On 2014/10/06 19:14:11, Marijn Kruisselbrink wrote:
> On 2014/10/06 16:43:42, Michael van Ouwerkerk wrote:
> > I assume you need a TODO here? And below.
> Sure, I can add more TODO's here as well, although it's not like I'm going to
> forget to actually do this...

TODO's are not just reminders for your future self: it helps your reviewers
understand whether there was an oversight or not, it saves them time and WTFs.
You want to be towards the left side of this:
http://www.osnews.com/story/19266/WTFs_m

https://codereview.chromium.org/586163003/diff/220001/content/browser/geofenc...
File content/browser/geofencing/geofencing_manager.h (right):

https://codereview.chromium.org/586163003/diff/220001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.h:55: static GeofencingManager*
GetInstance();
Why is this class a singleton though? It seems vaguely derived from the
Geolocation system but there it is also a thread, and the reason for that is
that it does horrible things like making blocking dbus calls that can take 3
seconds.

[Marijn Kruisselbrink, 2014-10-08 17:17:09]

https://codereview.chromium.org/586163003/diff/220001/content/browser/geofenc...
File content/browser/geofencing/geofencing_manager.h (right):

https://codereview.chromium.org/586163003/diff/220001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.h:55: static GeofencingManager*
GetInstance();
On 2014/10/08 16:59:01, Michael van Ouwerkerk wrote:
> Why is this class a singleton though? It seems vaguely derived from the
> Geolocation system but there it is also a thread, and the reason for that is
> that it does horrible things like making blocking dbus calls that can take 3
> seconds.
I'm not entirely convinced that making this a singleton is indeed the right
choice, although it does make some things easier. There are a couple of things
with geofencing that will require one central place through which all
registrations go though, since for example android limits all of chrome to have
at most 100 registrations. So my idea was that this class takes all the
registrations from all browser contexts/service workers, and does something
clever to decide which geofences to actually register with the underlying
system.
So I'm not ruling out refactoring this at a later point, but for now this seems
to be the clearest way to implement what I'm trying to accomplish.

[Marijn Kruisselbrink, 2014-10-08 17:19:41]

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

[Marijn Kruisselbrink, 2014-10-08 17:19:42]

+jam for OWNERS review

[jam, 2014-10-08 22:30:08]

where is the tracking bug? i.e. I can't tell if this is a web platform feature
(i.e. should be in src\content) or whether this is a chrome browser feature
(i.e. should be in src\chrome)

[Marijn Kruisselbrink, 2014-10-08 22:33:06]

On 2014/10/08 22:30:08, jam wrote:
> where is the tracking bug? i.e. I can't tell if this is a web platform feature
> (i.e. should be in src\content) or whether this is a chrome browser feature
> (i.e. should be in src\chrome)
Oops, added a BUG= line pointing to the web platform tracking bug.

[Marijn Kruisselbrink, 2014-10-10 17:54:06]

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

[Marijn Kruisselbrink, 2014-10-10 17:54:07]

Since jam is OOO adding another content OWNER
+avi: could you please give this an OWNERS review?

[Avi (use Gerrit), 2014-10-10 18:00:54]

Stampity stamp LGTM

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

The CQ bit was checked by mek@chromium.org

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

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

[commit-bot: I haz the power, 2014-10-10 20:16:48]

Committed patchset #8 (id:240001)

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

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