Add content API for requesting the current geolocation

Add content API for requesting the current geolocation

This CL adds a RequestLocationUpdate() function through which the embedder can request a one-time callback when the next location update becomes available.
Unit tests and a method for easy location mocking are provided.

This CL depends on https://chromiumcodereview.appspot.com/10316007/

BUG=chromium-os:18710
TEST=unit_tests, content_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135322

[bartfab (slow), 2012-05-02 19:10:21]

Hi John, Marcus,

Could you please review this CL?

@Marcus: I am looping in as a reviewer because joth@ and jknotten@ are both out
of office and I need this CL for R20.

[jam, 2012-05-02 20:59:04]

http://codereview.chromium.org/10344004/diff/3001/content/public/browser/geol...
File content/public/browser/geolocation.h (right):

http://codereview.chromium.org/10344004/diff/3001/content/public/browser/geol...
content/public/browser/geolocation.h:29: // (crbug.com/125931).
it's a bit odd adding this warning when existing tests already do this.

why not add a way to unhook this instead? otherwise i'd leave this comment out

http://codereview.chromium.org/10344004/diff/3001/content/public/browser/geol...
content/public/browser/geolocation.h:42: void CONTENT_EXPORT
OverrideCallbackLocationForTesting(Geoposition* position);
two ways to override a location is not good. let's not check this in like this.

[bartfab (slow), 2012-05-02 21:06:28]

The issue is that OverrideLocationForTesting() is meant to be used far beyond
the scope of unit tests as well. It is exposed via the browser's automation
interface. It really does have to affect the actual running geolocation stack.

On the other hand, in unit tests, spinning up the geolocation stack is overkill
and also very problematic as it cannot be undone (crbug.com/125342 may change
this but will not be done for R20).

[jam, 2012-05-03 06:17:49]

it's hard to review this change with all the function moves. these should be
done in a separate change, so that one can see what changed vs what just moved.

http://codereview.chromium.org/10344004/diff/7001/content/browser/geolocation...
File content/browser/geolocation/geolocation_provider.cc (right):

http://codereview.chromium.org/10344004/diff/7001/content/browser/geolocation...
content/browser/geolocation/geolocation_provider.cc:48: is_permission_granted_ =
true;
this looks like a process wide security variable. if it's set by cros, doesn't
this override user permission for other usage of this file?

[bartfab (slow), 2012-05-03 08:41:15]

As requested, I put the functions in geolocation_provider.cc back into their
random order for easier reviewing.

http://codereview.chromium.org/10344004/diff/7001/content/browser/geolocation...
File content/browser/geolocation/geolocation_provider.cc (right):

http://codereview.chromium.org/10344004/diff/7001/content/browser/geolocation...
content/browser/geolocation/geolocation_provider.cc:48: is_permission_granted_ =
true;
No. Website permissions are handled elsewhere. The only purpose of this variable
is to avoid spinning up the actual location providers (which costs battery)
until there is at least one client that is actually permitted to access
geolocation information. Callbacks implicitly have that permission.

[bartfab (slow), 2012-05-03 14:25:07]

Replacing reviewer bulach@ with jknotten@ as I believe he will be back in the
office first.

[jam, 2012-05-03 18:22:19]

lgtm

http://codereview.chromium.org/10344004/diff/3005/content/browser/geolocation...
File content/browser/geolocation/geolocation.cc (left):

http://codereview.chromium.org/10344004/diff/3005/content/browser/geolocation...
content/browser/geolocation/geolocation.cc:30: void OverrideLocationForTesting(
nit: no need to change the style in this function. if everyone changed things
that aren't in teh style guide to suit their personal preference, the code would
have a lot of hysteresis and it would be difficult to look at change history.

http://codereview.chromium.org/10344004/diff/3005/content/browser/geolocation...
File content/browser/geolocation/geolocation.cc (right):

http://codereview.chromium.org/10344004/diff/3005/content/browser/geolocation...
content/browser/geolocation/geolocation.cc:40: void
RequestLocationUpdateOnIOThread(
nit: blank line before

[bartfab (slow), 2012-05-04 08:30:21]

http://codereview.chromium.org/10344004/diff/3005/content/browser/geolocation...
File content/browser/geolocation/geolocation.cc (left):

http://codereview.chromium.org/10344004/diff/3005/content/browser/geolocation...
content/browser/geolocation/geolocation.cc:30: void OverrideLocationForTesting(
The intention was not to enforce one style over another. I simply wanted to make
sure that the parts of this function and RequestLocationUpdate() that do the
same also look the same. I undid the changes to this function now - done.

http://codereview.chromium.org/10344004/diff/3005/content/browser/geolocation...
File content/browser/geolocation/geolocation.cc (right):

http://codereview.chromium.org/10344004/diff/3005/content/browser/geolocation...
content/browser/geolocation/geolocation.cc:40: void
RequestLocationUpdateOnIOThread(
On 2012/05/03 18:22:19, John Abd-El-Malek wrote:
> nit: blank line before

Done.

[John Knottenbelt, 2012-05-04 10:16:58]

Thanks for doing this, Bart!

http://codereview.chromium.org/10344004/diff/22001/content/browser/geolocatio...
File content/browser/geolocation/geolocation.cc (right):

http://codereview.chromium.org/10344004/diff/22001/content/browser/geolocatio...
content/browser/geolocation/geolocation.cc:61: if
(!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
Please remove the curly braces here (or add them in your added code) to make the
style consistent in this file.

http://codereview.chromium.org/10344004/diff/22001/content/browser/geolocatio...
File content/browser/geolocation/geolocation_provider.cc (right):

http://codereview.chromium.org/10344004/diff/22001/content/browser/geolocatio...
content/browser/geolocation/geolocation_provider.cc:111: it->Run(position);
It seems possible, perhaps likely, that a callback may call
GeolocationProvider::RequestCallback which could result in the vector being
reallocated and so the iterators may be invalidated.

I can think of two ways to avoid this problem. 
  - instead of using an iterator, use operator [] and index variable. callbacks
registered during callback invocation will be called in the same pass.
  - before iterating through the vector, swap it with an empty one. callbacks
registered during the callback invocation will be called at the next update.

I think that the second option makes more sense than the first.

[bartfab (slow), 2012-05-04 10:35:58]

John Knottenbelt, could you have another look and approve the CL if you see no
further issues? I would be extremely delighted to land this and the two
follow-up CLs today so that this feature makes it into R20.

http://codereview.chromium.org/10344004/diff/22001/content/browser/geolocatio...
File content/browser/geolocation/geolocation.cc (right):

http://codereview.chromium.org/10344004/diff/22001/content/browser/geolocatio...
content/browser/geolocation/geolocation.cc:61: if
(!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
I had removed the braces. jam@ requested that I add them back. Removing them
again - full circle :).

http://codereview.chromium.org/10344004/diff/22001/content/browser/geolocatio...
File content/browser/geolocation/geolocation_provider.cc (right):

http://codereview.chromium.org/10344004/diff/22001/content/browser/geolocatio...
content/browser/geolocation/geolocation_provider.cc:111: it->Run(position);
Nice catch. Done.

[John Knottenbelt, 2012-05-04 10:54:50]

lgtm

[commit-bot: I haz the power, 2012-05-04 10:55:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/10344004/25002

[commit-bot: I haz the power, 2012-05-04 11:10:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/10344004/18010

[commit-bot: I haz the power, 2012-05-04 11:32:54]

Try job failure for 10344004-18010 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-04 11:44:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/10344004/21005

[commit-bot: I haz the power, 2012-05-04 13:04:49]

Change committed as 135322