Refactor GeofencingManager to have one instance per StoragePartition.

Refactor GeofencingManager to have one instance per StoragePartition.

Add a new GeofencingService class for the few global tasks GeofencingManager
used to do.

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

[Marijn Kruisselbrink, 2014-10-15 20:07:39]

Patchset #1 (id:1) has been deleted

[Marijn Kruisselbrink, 2014-10-15 20:07:45]

Patchset #1 (id:20001) has been deleted

[Marijn Kruisselbrink, 2014-10-15 20:07:51]

Patchset #1 (id:40001) has been deleted

[Marijn Kruisselbrink, 2014-10-15 20:14:49]

Patchset #1 (id:60001) has been deleted

[Marijn Kruisselbrink, 2014-10-15 20:19:23]

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

[Marijn Kruisselbrink, 2014-10-15 20:19:24]

As pointed out in https://codereview.chromium.org/629393002/ I really couldn't
postpone having to have a per-storage partition GeofencingManager. So this CL
does most of the necessary refactoring (and I'll rebase the event delivery CL on
top of this one).

[Marijn Kruisselbrink, 2014-10-15 20:31:45]

Actually, I have to figure out the threading situation a bit better. Currently
this CL won't entirely work since some code is called on the wrong thread.

[Marijn Kruisselbrink, 2014-10-15 23:44:52]

On 2014/10/15 20:31:45, Marijn Kruisselbrink wrote:
> Actually, I have to figure out the threading situation a bit better. Currently
> this CL won't entirely work since some code is called on the wrong thread.
That should all be working fine now.

[Ken Russell (switch to Gerrit), 2014-10-16 23:45:35]

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

[Ken Russell (switch to Gerrit), 2014-10-16 23:45:36]

Please don't resubmit this CL in its present state. It's causing the browser to
hang upon exit and that was breaking the Windows GPU bots. See
https://code.google.com/p/chromium/issues/detail?id=383125#c12 .

[Marijn Kruisselbrink, 2014-10-16 23:49:25]

On 2014/10/16 23:45:36, Ken Russell wrote:
> Please don't resubmit this CL in its present state. It's causing the browser
to
> hang upon exit and that was breaking the Windows GPU bots. See
> https://code.google.com/p/chromium/issues/detail?id=383125#c12 .
Actually this is incorrect on many levels.
1) This CL is no longer in "its present" state and hasn't been so for a long
time.
2) It wasn't causing the browser to hang on exit, it was causing the browser to
crash on startup.
3) This CL was never submitted, the tryserver is broken in that this particular
trybot apparently treats chrome crashing on startup as a retry-able error.

[Marijn Kruisselbrink, 2014-10-20 20:28:36]

Michael: could you please review this?

[Marijn Kruisselbrink, 2014-10-20 20:32:12]

On 2014/10/20 20:28:36, Marijn Kruisselbrink wrote:
> Michael: could you please review this?
(uhm, using first names is confusing; I mostly meant mvanouwerkerk@ here)

[Michael van Ouwerkerk, 2014-10-21 11:46:41]

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

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.cc:21: const StatusCallback&
callback,
It seems odd for a struct to own a callback, but maybe it's ok.

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.cc:85:
service_->UnregisterRegion(registration.first);
Does this mean all regions are unregistered whenever the browser is closed? That
would not make for a very useful API, so maybe I misunderstood.

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.cc:105:
callback.Run(GEOFENCING_STATUS_ERROR);
Maybe add a TODO here or file a bug for returning a more specific error. This
won't help web developers understand what went wrong. Same for every other place
that returns GEOFENCING_STATUS_ERROR.

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.cc:180: // If the registration
wasn't succesfull, remove it from our storage.
Nit: successful

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.cc:183: }
Nit: no need for braces with one liners

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.cc:188: const std::string&
region_id) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.cc:214: int64
geofencing_registration_id) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);

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

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.h:87: void
SetServiceForTests(GeofencingServiceInterface* service) {
nit: ForTesting

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

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_service.cc:96: return
geofencing_registration_id;
Minting a new id seems pointless if the operation failed. How about returning a
well known invalid id and not adding a new registration to the map?

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

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_service.h:28: // on GeofencingService. It
defines teh interface exposed by
Nit: the

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_service.h:30: class
GeofencingServiceInterface {
I'd prefer renaming GeofencingService to GeofencingServiceImpl and
GeofencingServiceInterface to GeofencingService. It's the more common pattern in
Chromium.

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_service.h:74: void
SetProviderForTests(scoped_ptr<GeofencingProvider> provider);
nit: ForTesting - http://www.chromium.org/developers/coding-style

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_service.h:75: int
RegistrationCountForTests();
And here.

[Marijn Kruisselbrink, 2014-10-21 23:25:59]

Patchset #5 (id:160001) has been deleted

[Marijn Kruisselbrink, 2014-10-21 23:50:07]

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

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.cc:21: const StatusCallback&
callback,
On 2014/10/21 11:46:40, Michael van Ouwerkerk wrote:
> It seems odd for a struct to own a callback, but maybe it's ok.
Yeah, I'm not entirely thrilled at this part of the current implementation
either, but I think it should be fine to keep it like this for now.

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.cc:85:
service_->UnregisterRegion(registration.first);
On 2014/10/21 11:46:40, Michael van Ouwerkerk wrote:
> Does this mean all regions are unregistered whenever the browser is closed?
Yes, currently it means that.
> That would not make for a very useful API, so maybe I misunderstood.
See also the thread on "Service Workers and multi-profile chrome" I started a
while back. From that thread the conclusion seemed to be that on all platforms
except android the desired behavior is indeed to unregister all regions when the
last trace of a profile/storage partition within chrome is shut down. 
Now on android of course we do want to support geofence registrations that
outlive the browser process, and that will need changes to more than just this
part of the code (for one the actual registration will have to keep track of
which storage partition it is part of).
I added a TODO comment explaining what will need to change here.

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.cc:105:
callback.Run(GEOFENCING_STATUS_ERROR);
On 2014/10/21 11:46:40, Michael van Ouwerkerk wrote:
> Maybe add a TODO here or file a bug for returning a more specific error. This
> won't help web developers understand what went wrong. Same for every other
place
> that returns GEOFENCING_STATUS_ERROR.

Done.

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.cc:180: // If the registration
wasn't succesfull, remove it from our storage.
On 2014/10/21 11:46:41, Michael van Ouwerkerk wrote:
> Nit: successful

Done.

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.cc:183: }
On 2014/10/21 11:46:40, Michael van Ouwerkerk wrote:
> Nit: no need for braces with one liners

Done.

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.cc:188: const std::string&
region_id) {
On 2014/10/21 11:46:40, Michael van Ouwerkerk wrote:
> DCHECK_CURRENTLY_ON(BrowserThread::IO);

Done.

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.cc:214: int64
geofencing_registration_id) {
On 2014/10/21 11:46:41, Michael van Ouwerkerk wrote:
> DCHECK_CURRENTLY_ON(BrowserThread::IO);

Done.

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

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.h:87: void
SetServiceForTests(GeofencingServiceInterface* service) {
On 2014/10/21 11:46:41, Michael van Ouwerkerk wrote:
> nit: ForTesting

Done.

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

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_service.cc:96: return
geofencing_registration_id;
On 2014/10/21 11:46:41, Michael van Ouwerkerk wrote:
> Minting a new id seems pointless if the operation failed. How about returning
a
> well known invalid id and not adding a new registration to the map?
I'd rather not handle different failure cases differently, requiring two
separate code paths for code calling RegisterRegion to handle failures, etc.
Having said that, I could get rid of this part and just DCHECK or something
instead. GeofencingManager already covers the same error condition this is
trying to check for, and there are unit tests to make sure of that, so that
might be a nice way to get rid of the awkwardness of minting a new ID in this
case.

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

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_service.h:28: // on GeofencingService. It
defines teh interface exposed by
On 2014/10/21 11:46:41, Michael van Ouwerkerk wrote:
> Nit: the

Done.

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_service.h:30: class
GeofencingServiceInterface {
On 2014/10/21 11:46:41, Michael van Ouwerkerk wrote:
> I'd prefer renaming GeofencingService to GeofencingServiceImpl and
> GeofencingServiceInterface to GeofencingService. It's the more common pattern
in
> Chromium.

Done.

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_service.h:74: void
SetProviderForTests(scoped_ptr<GeofencingProvider> provider);
On 2014/10/21 11:46:41, Michael van Ouwerkerk wrote:
> nit: ForTesting - http://www.chromium.org/developers/coding-style
The presubmit check seems to check or more tan just ForTesting, but yeah, better
to be as consistent as possible of course. Done.

https://codereview.chromium.org/645763003/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_service.h:75: int
RegistrationCountForTests();
On 2014/10/21 11:46:41, Michael van Ouwerkerk wrote:
> And here.

Done.

[Marijn Kruisselbrink, 2014-10-23 03:26:37]

PTAL

[Michael van Ouwerkerk, 2014-10-23 16:06:32]

lgtm

[Marijn Kruisselbrink, 2014-10-23 16:24:13]

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

[Marijn Kruisselbrink, 2014-10-23 16:24:14]

+avi for content/ OWNERS (at least for the bits that are not in a geofencing
subdirectory)
michaeln: can you review/approve the
chrome/browser/browsing_data/browsing_data_remover_unittest.cc change?

[Avi (use Gerrit), 2014-10-23 17:32:46]

content non-geo directory LGTM mod one nit you must fix.

https://codereview.chromium.org/645763003/diff/180001/content/browser/storage...
File content/browser/storage_partition_impl.h (right):

https://codereview.chromium.org/645763003/diff/180001/content/browser/storage...
content/browser/storage_partition_impl.h:46: virtual GeofencingManager*
GetGeofencingManager() override;
no |virtual|; it's implied by |override|.

[Marijn Kruisselbrink, 2014-10-23 17:43:57]

Patchset #7 (id:220001) has been deleted

[michaeln, 2014-10-23 20:27:06]

lgtm

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

https://codereview.chromium.org/645763003/diff/240001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.h:42: : NON_EXPORTED_BASE(public
GeofencingRegistrationDelegate),
ah, now i can tell that ownership is not transferred and the public virtual dtor
should be protected

https://codereview.chromium.org/645763003/diff/240001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.h:82: // registrations).
The synchrounous return of this method may be hard to maintain once persistence
is added and this data needs to be read in.

https://codereview.chromium.org/645763003/diff/240001/content/browser/geofenc...
File content/browser/geofencing/geofencing_registration_delegate.h (right):

https://codereview.chromium.org/645763003/diff/240001/content/browser/geofenc...
content/browser/geofencing/geofencing_registration_delegate.h:20: virtual
~GeofencingRegistrationDelegate() {}
The ownership semantics of the GeofencingRegistrationDelegates is not clear from
looking at GeofencingService and GeofencingRegistrationDelegate class
interfaces. Also nothing mentioned about that in the doc comments.

If ownership is transferred to the GeofencingService, it could be provided as a
scoped_ptr<>. If ownership is not transferred, the virtual dtor could be
protected.

https://codereview.chromium.org/645763003/diff/240001/content/browser/geofenc...
File content/browser/geofencing/geofencing_service.h (right):

https://codereview.chromium.org/645763003/diff/240001/content/browser/geofenc...
content/browser/geofencing/geofencing_service.h:52: //    underlying
GeofencingProvider.
nit: extra spaces

[Marijn Kruisselbrink, 2014-10-23 20:54:35]

https://codereview.chromium.org/645763003/diff/180001/content/browser/storage...
File content/browser/storage_partition_impl.h (right):

https://codereview.chromium.org/645763003/diff/180001/content/browser/storage...
content/browser/storage_partition_impl.h:46: virtual GeofencingManager*
GetGeofencingManager() override;
On 2014/10/23 17:32:45, Avi wrote:
> no |virtual|; it's implied by |override|.

Done.

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

https://codereview.chromium.org/645763003/diff/240001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.h:42: : NON_EXPORTED_BASE(public
GeofencingRegistrationDelegate),
On 2014/10/23 20:27:06, michaeln wrote:
> ah, now i can tell that ownership is not transferred and the public virtual
dtor
> should be protected

Done.

https://codereview.chromium.org/645763003/diff/240001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.h:82: // registrations).
On 2014/10/23 20:27:06, michaeln wrote:
> The synchrounous return of this method may be hard to maintain once
persistence
> is added and this data needs to be read in.
I think even when persistence is added GeofencingManager will still always need
to keep a copy of all registrations in memory as well, after all they will need
to be registered with the underlying geofencing provider. So I think this will
be able to stay synchronous.

https://codereview.chromium.org/645763003/diff/240001/content/browser/geofenc...
File content/browser/geofencing/geofencing_registration_delegate.h (right):

https://codereview.chromium.org/645763003/diff/240001/content/browser/geofenc...
content/browser/geofencing/geofencing_registration_delegate.h:20: virtual
~GeofencingRegistrationDelegate() {}
On 2014/10/23 20:27:06, michaeln wrote:
> The ownership semantics of the GeofencingRegistrationDelegates is not clear
from
> looking at GeofencingService and GeofencingRegistrationDelegate class
> interfaces. Also nothing mentioned about that in the doc comments.
> 
> If ownership is transferred to the GeofencingService, it could be provided as
a
> scoped_ptr<>. If ownership is not transferred, the virtual dtor could be
> protected.
Ah yes, good point Made the destructor protected and added a comment to
GeofencingService.

https://codereview.chromium.org/645763003/diff/240001/content/browser/geofenc...
File content/browser/geofencing/geofencing_service.h (right):

https://codereview.chromium.org/645763003/diff/240001/content/browser/geofenc...
content/browser/geofencing/geofencing_service.h:52: //    underlying
GeofencingProvider.
On 2014/10/23 20:27:06, michaeln wrote:
> nit: extra spaces

Done.

[Marijn Kruisselbrink, 2014-10-23 20:54:40]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-10-23 20:56:51]

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

[commit-bot: I haz the power, 2014-10-23 22:06:30]

Committed patchset #8 (id:260001)

[commit-bot: I haz the power, 2014-10-23 22:07:07]

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