Add support for top level geolocation arbitrator, and access token persistenc...

Add support for top level geolocation arbitrator, and access token persistence (via local state prefs)


BUG=11246
TEST=unit_tests.exe --gtest_filer=Geoloc* 


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

[jorlow, 2010-02-14 21:33:17]

Some style based comments.  I don't fully understand what's going on here, so
I'm assuming Marcus is the better reviewer for the patch.

http://codereview.chromium.org/603040/diff/6021/3036
File chrome/browser/geolocation/access_token_store.cc (right):

http://codereview.chromium.org/603040/diff/6021/3036#newcode23
chrome/browser/geolocation/access_token_store.cc:23:
access_token_dictionary->SetWithoutPathExpansion(UTF8ToWide(url.spec()),
Either line up with ( or don't put anything on the ( line.

http://codereview.chromium.org/603040/diff/6021/3034
File chrome/browser/geolocation/location_arbitrator.cc (right):

http://codereview.chromium.org/603040/diff/6021/3034#newcode27
chrome/browser/geolocation/location_arbitrator.cc:27: public NonThreadSafe {  //
For now, may change later.
Not sure if this comment adds any info.

http://codereview.chromium.org/603040/diff/6021/3034#newcode30
chrome/browser/geolocation/location_arbitrator.cc:30: URLRequestContextGetter*
context_getter);
2 more spaces in

http://codereview.chromium.org/603040/diff/6021/3035
File chrome/browser/geolocation/location_arbitrator.h (right):

http://codereview.chromium.org/603040/diff/6021/3035#newcode14
chrome/browser/geolocation/location_arbitrator.h:14: static
GeoLocationArbitrator* New(AccessTokenStore* access_token_store,
line up on ( or don't put anything on ( line

http://codereview.chromium.org/603040/diff/6021/3035#newcode42
chrome/browser/geolocation/location_arbitrator.h:42: 
so many new lines

http://codereview.chromium.org/603040/diff/6021/3029
File chrome/browser/geolocation/location_provider.h (left):

http://codereview.chromium.org/603040/diff/6021/3029#oldcode21
chrome/browser/geolocation/location_provider.h:21: struct Position;
if this still were a struct, it should go after the classes....but it's fine now

http://codereview.chromium.org/603040/diff/6021/3032
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/603040/diff/6021/3032#newcode23
chrome/common/pref_names.cc:23: // This holds an integer value:
How about "is one of several integer values"

[bulach, 2010-02-15 10:46:49]

a few initial comments, let's chat more about this later.
also: we need to take care of the token store when in incognito mode, right?

http://codereview.chromium.org/603040/diff/6021/3036
File chrome/browser/geolocation/access_token_store.cc (right):

http://codereview.chromium.org/603040/diff/6021/3036#newcode15
chrome/browser/geolocation/access_token_store.cc:15: class
ChomrePrefsAccessTokenStore : public AccessTokenStore {
typo, s/Chomr/Chrom/

http://codereview.chromium.org/603040/diff/6021/3035
File chrome/browser/geolocation/location_arbitrator.h (right):

http://codereview.chromium.org/603040/diff/6021/3035#newcode21
chrome/browser/geolocation/location_arbitrator.h:21: // available in the
forseeable future.
so this is where we meet? :)
it'd be nice to expand a bit on what thread I must Add/RemoveObserver, and where
OnLocationUpdate will be called.. I think IO thread would be a good candidate,
and then my geolocationdispatcher will take care of distributing across the
UI/renderers..

there's also notification_service.h, which might help.

[joth, 2010-02-15 14:17:14]

All comments addressed, I think.
Will make some improvements to the access token store threading usage in a
follow up CL.

http://codereview.chromium.org/603040/diff/6021/3036
File chrome/browser/geolocation/access_token_store.cc (right):

http://codereview.chromium.org/603040/diff/6021/3036#newcode15
chrome/browser/geolocation/access_token_store.cc:15: class
ChomrePrefsAccessTokenStore : public AccessTokenStore {
On 2010/02/15 10:46:49, bulach wrote:
> typo, s/Chomr/Chrom/

Done.

http://codereview.chromium.org/603040/diff/6021/3036#newcode23
chrome/browser/geolocation/access_token_store.cc:23:
access_token_dictionary->SetWithoutPathExpansion(UTF8ToWide(url.spec()),
On 2010/02/14 21:33:18, Jeremy Orlow wrote:
> Either line up with ( or don't put anything on the ( line.

Done.

http://codereview.chromium.org/603040/diff/6021/3034
File chrome/browser/geolocation/location_arbitrator.cc (right):

http://codereview.chromium.org/603040/diff/6021/3034#newcode27
chrome/browser/geolocation/location_arbitrator.cc:27: public NonThreadSafe {  //
For now, may change later.
On 2010/02/14 21:33:18, Jeremy Orlow wrote:
> Not sure if this comment adds any info.

Agreed. Removed

http://codereview.chromium.org/603040/diff/6021/3034#newcode30
chrome/browser/geolocation/location_arbitrator.cc:30: URLRequestContextGetter*
context_getter);
On 2010/02/14 21:33:18, Jeremy Orlow wrote:
> 2 more spaces in

Now you pointed it out, I tabbed in to line up with (

http://codereview.chromium.org/603040/diff/6021/3035
File chrome/browser/geolocation/location_arbitrator.h (right):

http://codereview.chromium.org/603040/diff/6021/3035#newcode14
chrome/browser/geolocation/location_arbitrator.h:14: static
GeoLocationArbitrator* New(AccessTokenStore* access_token_store,
On 2010/02/14 21:33:18, Jeremy Orlow wrote:
> line up on ( or don't put anything on ( line

Done.

http://codereview.chromium.org/603040/diff/6021/3035#newcode21
chrome/browser/geolocation/location_arbitrator.h:21: // available in the
forseeable future.
On 2010/02/15 10:46:49, bulach wrote:
> so this is where we meet? :)
> it'd be nice to expand a bit on what thread I must Add/RemoveObserver, and
where
> OnLocationUpdate will be called.. I think IO thread would be a good candidate,
> and then my geolocationdispatcher will take care of distributing across the
> UI/renderers..
> 
> there's also notification_service.h, which might help.
> 

Right. Class level & fn comments added.

http://codereview.chromium.org/603040/diff/6021/3035#newcode42
chrome/browser/geolocation/location_arbitrator.h:42: 
On 2010/02/14 21:33:18, Jeremy Orlow wrote:
> so many new lines

Done.

http://codereview.chromium.org/603040/diff/6021/3029
File chrome/browser/geolocation/location_provider.h (left):

http://codereview.chromium.org/603040/diff/6021/3029#oldcode21
chrome/browser/geolocation/location_provider.h:21: struct Position;
On 2010/02/14 21:33:18, Jeremy Orlow wrote:
> if this still were a struct, it should go after the classes....but it's fine
now

Done.
I realized it must be struct as it has public member data with no trailing _

http://codereview.chromium.org/603040/diff/6021/3032
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/603040/diff/6021/3032#newcode23
chrome/common/pref_names.cc:23: // This holds an integer value:
On 2010/02/14 21:33:18, Jeremy Orlow wrote:
> How about "is one of several integer values"
Refined to
// An integer pref. Holds one of several values: