Port the gears geolocation network provider to Chromium...

Port the gears geolocation network provider to Chromium

BUG=http://crbug.com/11246
TEST=See http://codereview.chromium.org/556106


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

[joth, 2010-02-01 11:27:20]

Some more edits to make these compile clean. I'm writing tests for the network
provider next.

[bulach, 2010-02-01 17:19:59]

On 2010/02/01 11:27:20, jothchrome wrote:
> Some more edits to make these compile clean. I'm writing tests for the network
> provider next.

great stuff! I have a few minor comments so far, plus a couple regarding naming
/ flow:

[bulach, 2010-02-01 17:47:36]

oh, yes, the comments :)

http://codereview.chromium.org/552250/diff/6001/6002
File chrome/browser/geolocation/geoposition.h (right):

http://codereview.chromium.org/552250/diff/6001/6002#newcode6
chrome/browser/geolocation/geoposition.h:6: // position fix.
is this the first check-in from Gears? perhaps add a #if 0? otherwise, there a
few comments that needs updating.

http://codereview.chromium.org/552250/diff/6001/6002#newcode16
chrome/browser/geolocation/geoposition.h:16: static const int kBadAccuracy = -1;
 // Accuracy must be non-negative.
hmm.. WebKit has extra bools for "hasFooBar()" / "canProvide()":
http://trac.webkit.org/browser/trunk/WebCore/page/Coordinates.h

I wonder if it'd be easier to mimic that class instead of relying on magic
numbers?

anyhow, rather than declaring here as "global statics", perhaps move to Position
struct itself?

http://codereview.chromium.org/552250/diff/6001/6011
File chrome/browser/geolocation/location_provider.cc (right):

http://codereview.chromium.org/552250/diff/6001/6011#newcode60
chrome/browser/geolocation/location_provider.cc:60: CheckRunningInClientLoop();
here and above: not sure I understood the Check and the subsequent
RunInClientThread?

http://codereview.chromium.org/552250/diff/6001/6012
File chrome/browser/geolocation/location_provider.h (right):

http://codereview.chromium.org/552250/diff/6001/6012#newcode40
chrome/browser/geolocation/location_provider.h:40: private:
protected?

http://codereview.chromium.org/552250/diff/6001/6012#newcode59
chrome/browser/geolocation/location_provider.h:59: virtual bool
MovementDetected(LocationProviderBase* provider) = 0;
\n

http://codereview.chromium.org/552250/diff/6001/6012#newcode60
chrome/browser/geolocation/location_provider.h:60: private:
protected?

http://codereview.chromium.org/552250/diff/6001/6012#newcode87
chrome/browser/geolocation/location_provider.h:87: virtual
~LocationProviderBase();
friend decl and dtor should be private.

http://codereview.chromium.org/552250/diff/6001/6008
File chrome/browser/geolocation/location_provider_pool.cc (right):

http://codereview.chromium.org/552250/diff/6001/6008#newcode20
chrome/browser/geolocation/location_provider_pool.cc:20: const string16
&language);
unindent (and foo* bar)

http://codereview.chromium.org/552250/diff/6001/6009
File chrome/browser/geolocation/location_provider_pool.h (right):

http://codereview.chromium.org/552250/diff/6001/6009#newcode34
chrome/browser/geolocation/location_provider_pool.h:34: const string16
&language,
all around: foo* bar and foo& bar
instead of: foo *bar and foo &bar.

http://codereview.chromium.org/552250/diff/6001/6004
File chrome/browser/geolocation/network_location_provider.cc (right):

http://codereview.chromium.org/552250/diff/6001/6004#newcode22
chrome/browser/geolocation/network_location_provider.cc:22: }  // namespave
s/spave/space/

http://codereview.chromium.org/552250/diff/6001/6004#newcode76
chrome/browser/geolocation/network_location_provider.cc:76: // TODO(steveblock):
Make use of radio_data.
perhas: assert(key);

http://codereview.chromium.org/552250/diff/6001/6004#newcode83
chrome/browser/geolocation/network_location_provider.cc:83: const string16
seperator(ASCIIToUTF16("|"));
s/sepe/sepa/

the compiler would probably do it for you, but perhaps move out of the for and
declare as kSeparator?

http://codereview.chromium.org/552250/diff/6001/6004#newcode162
chrome/browser/geolocation/network_location_provider.cc:162:
is_radio_data_complete_ = radio_data_provider_->GetData(&radio_data_);
perhaps indent?

http://codereview.chromium.org/552250/diff/6001/6004#newcode233
chrome/browser/geolocation/network_location_provider.cc:233: // TODO(joth):
Condider timing out any pending request.
s/Condi/Consi/

also: "MakeRequest" seems misleading, as this might simply return the cached
value.
finally, suppose this scenario:
- we data in the cache for (A, B, C)
- we "differ significantly", and then issue a new request
- the user moves, and is back to the previous position, i.e., (A, B, C) ap's
available from cache

in this scenario, we won't reply with the cached data.. hmm, in fact, I think
we'll never update the position if we move "fast enough", as in, before the
request completes: we'll always keep the old position since.

http://codereview.chromium.org/552250/diff/6001/6004#newcode251
chrome/browser/geolocation/network_location_provider.cc:251: position_.timestamp
= timestamp_;
hmm... this is confusing, I thought it was the position _obtained_ timestamp,
rather than position _accessed_ timestamp.. perhaps rename the member?

http://codereview.chromium.org/552250/diff/6001/6006
File chrome/browser/geolocation/network_location_provider.h (right):

http://codereview.chromium.org/552250/diff/6001/6006#newcode37
chrome/browser/geolocation/network_location_provider.h:37: bool StartProvider();
private?

http://codereview.chromium.org/552250/diff/6001/6015
File chrome/browser/geolocation/network_location_request.cc (right):

http://codereview.chromium.org/552250/diff/6001/6015#newcode189
chrome/browser/geolocation/network_location_request.cc:189:
FormatPositionError(server_url, L"No response recieved", position);
s/recie/recei/

http://codereview.chromium.org/552250/diff/6001/6015#newcode210
chrome/browser/geolocation/network_location_request.cc:210: return;
unnecessary?

http://codereview.chromium.org/552250/diff/6001/6015#newcode256
chrome/browser/geolocation/network_location_request.cc:256: Value* value;
iirc, valgrind used to complain, so perhaps:
Value* value = NULL;
and DCHECK(value); below?

http://codereview.chromium.org/552250/diff/6001/6015#newcode296
chrome/browser/geolocation/network_location_request.cc:296: << error_msg <<
".\n";
indent?

http://codereview.chromium.org/552250/diff/6001/6005
File chrome/browser/geolocation/network_location_request.h (right):

http://codereview.chromium.org/552250/diff/6001/6005#newcode31
chrome/browser/geolocation/network_location_request.h:31: const string16&
access_token) = 0;
\n

[jorlow, 2010-02-02 03:32:36]

One comment.  Otherwise going to have to trust Marcus on this one as I just
don't have time to review at the moment.  (4.1's feature freeze was today and
we're still working on getting this stuff done.)

I'll try to sync up on this geo stuff when I get to London.

http://codereview.chromium.org/552250/diff/8001/8014
File chrome/browser/geolocation/network_location_request.cc (right):

http://codereview.chromium.org/552250/diff/8001/8014#newcode21
chrome/browser/geolocation/network_location_request.cc:21: const wchar_t*
kAccessTokenString = L"access_token";
Is there precedent for something like this somewhere else in the code?  I've
never see anything like this.  If there's nothing else similar, maybe there's a
more standard way to do this?

[joth, 2010-02-02 12:09:08]

Thanks for the feedback, sorry to be hassling you with this during the  4.1
freeze.


On 2 February 2010 03:32, <jorlow@chromium.org> wrote:

> One comment.  Otherwise going to have to trust Marcus on this one as I just
> don't have time to review at the moment.  (4.1's feature freeze was today
> and
> we're still working on getting this stuff done.)
>
> I'll try to sync up on this geo stuff when I get to London.
>
>
> http://codereview.chromium.org/552250/diff/8001/8014
>
> File chrome/browser/geolocation/network_location_request.cc (right):
>
> http://codereview.chromium.org/552250/diff/8001/8014#newcode21
> chrome/browser/geolocation/network_location_request.cc:21: const
> wchar_t* kAccessTokenString = L"access_token";
> Is there precedent for something like this somewhere else in the code?
> I've never see anything like this.  If there's nothing else similar,
> maybe there's a more standard way to do this?
>
>
>
Do you mean having wchar_t* constant to name a wide literal?

AFAICT it's fairly well established:
http://www.google.com/codesearch?hl=en&lr=&q=package:"src.chromium.org/svn/tr...

example:
http://www.google.com/codesearch/p?hl=en#h0RrPvyPu-c/net/disk_cache/block_fil...


namespace {const wchar_t* kIndexName = L"index";



Cheers
Joth

[joth, 2010-02-02 15:28:26]

(opps hadn't realized I hadn't sent these)
Thanks Marcus & Jeremy.
All comments addressed, I hope.
I'll look to submit once bots are happy.

Joth

http://codereview.chromium.org/552250/diff/6001/6002
File chrome/browser/geolocation/geoposition.h (right):

http://codereview.chromium.org/552250/diff/6001/6002#newcode16
chrome/browser/geolocation/geoposition.h:16: static const int kBadAccuracy = -1;
 // Accuracy must be non-negative.
On 2010/02/01 17:47:36, bulach wrote:
> hmm.. WebKit has extra bools for "hasFooBar()" / "canProvide()":
> http://trac.webkit.org/browser/trunk/WebCore/page/Coordinates.h
> 
> I wonder if it'd be easier to mimic that class instead of relying on magic
> numbers?
> 
> anyhow, rather than declaring here as "global statics", perhaps move to
Position
> struct itself?
> 

Done.
I needed have kept the use of sentinels, but moved the lot into a new .cc file
and encapsulated the validity checks.

http://codereview.chromium.org/552250/diff/6001/6011
File chrome/browser/geolocation/location_provider.cc (right):

http://codereview.chromium.org/552250/diff/6001/6011#newcode60
chrome/browser/geolocation/location_provider.cc:60: CheckRunningInClientLoop();
On 2010/02/01 17:47:36, bulach wrote:
> here and above: not sure I understood the Check and the subsequent
> RunInClientThread?

Done.

http://codereview.chromium.org/552250/diff/6001/6012
File chrome/browser/geolocation/location_provider.h (right):

http://codereview.chromium.org/552250/diff/6001/6012#newcode40
chrome/browser/geolocation/location_provider.h:40: private:
On 2010/02/01 17:47:36, bulach wrote:
> protected?

Done.

http://codereview.chromium.org/552250/diff/6001/6012#newcode59
chrome/browser/geolocation/location_provider.h:59: virtual bool
MovementDetected(LocationProviderBase* provider) = 0;
On 2010/02/01 17:47:36, bulach wrote:
> \n

Done.

http://codereview.chromium.org/552250/diff/6001/6012#newcode60
chrome/browser/geolocation/location_provider.h:60: private:
On 2010/02/01 17:47:36, bulach wrote:
> protected?

Done.

http://codereview.chromium.org/552250/diff/6001/6012#newcode87
chrome/browser/geolocation/location_provider.h:87: virtual
~LocationProviderBase();
On 2010/02/01 17:47:36, bulach wrote:
> friend decl and dtor should be private.

dtor can't be, as derived classes wont be able to access it.
Seems Chrome uses the 'friend declaration near the thing that needs it' pattern,
so I'll keep it here.

http://codereview.chromium.org/552250/diff/6001/6008
File chrome/browser/geolocation/location_provider_pool.cc (right):

http://codereview.chromium.org/552250/diff/6001/6008#newcode20
chrome/browser/geolocation/location_provider_pool.cc:20: const string16
&language);
On 2010/02/01 17:47:36, bulach wrote:
> unindent (and foo* bar)

Done.

http://codereview.chromium.org/552250/diff/6001/6009
File chrome/browser/geolocation/location_provider_pool.h (right):

http://codereview.chromium.org/552250/diff/6001/6009#newcode34
chrome/browser/geolocation/location_provider_pool.h:34: const string16
&language,
On 2010/02/01 17:47:36, bulach wrote:
> all around: foo* bar and foo& bar
> instead of: foo *bar and foo &bar.

Done.

http://codereview.chromium.org/552250/diff/6001/6004
File chrome/browser/geolocation/network_location_provider.cc (right):

http://codereview.chromium.org/552250/diff/6001/6004#newcode22
chrome/browser/geolocation/network_location_provider.cc:22: }  // namespave
On 2010/02/01 17:47:36, bulach wrote:
> s/spave/space/

Done.

http://codereview.chromium.org/552250/diff/6001/6004#newcode76
chrome/browser/geolocation/network_location_provider.cc:76: // TODO(steveblock):
Make use of radio_data.
On 2010/02/01 17:47:36, bulach wrote:
> perhas: assert(key);

Done.

http://codereview.chromium.org/552250/diff/6001/6004#newcode83
chrome/browser/geolocation/network_location_provider.cc:83: const string16
seperator(ASCIIToUTF16("|"));
On 2010/02/01 17:47:36, bulach wrote:
> s/sepe/sepa/
> 
> the compiler would probably do it for you, but perhaps move out of the for and
> declare as kSeparator?

good point, extracted (although it can't be a k-constant as it's a class
instance...)

http://codereview.chromium.org/552250/diff/6001/6004#newcode162
chrome/browser/geolocation/network_location_provider.cc:162:
is_radio_data_complete_ = radio_data_provider_->GetData(&radio_data_);
On 2010/02/01 17:47:36, bulach wrote:
> perhaps indent?

Done.

http://codereview.chromium.org/552250/diff/6001/6004#newcode233
chrome/browser/geolocation/network_location_provider.cc:233: // TODO(joth):
Condider timing out any pending request.
On 2010/02/01 17:47:36, bulach wrote:
> s/Condi/Consi/
> 
> also: "MakeRequest" seems misleading, as this might simply return the cached
> value.
> finally, suppose this scenario:
> - we data in the cache for (A, B, C)
> - we "differ significantly", and then issue a new request
> - the user moves, and is back to the previous position, i.e., (A, B, C) ap's
> available from cache
> 
> in this scenario, we won't reply with the cached data.. hmm, in fact, I think
> we'll never update the position if we move "fast enough", as in, before the
> request completes: we'll always keep the old position since.
> 

The final scenario is covered by the previous request's completion
(LocationResponseAvailable) noticing that is_new_data_available_ (via
UpdatePosition) and so issuing a new request.
Agree about the name, I kept it for Gears similarity.
Now RequestPosition.

http://codereview.chromium.org/552250/diff/6001/6004#newcode251
chrome/browser/geolocation/network_location_provider.cc:251: position_.timestamp
= timestamp_;
On 2010/02/01 17:47:36, bulach wrote:
> hmm... this is confusing, I thought it was the position _obtained_ timestamp,
> rather than position _accessed_ timestamp.. perhaps rename the member?

Had me confused at first too :-)
It's the sensor data timestamp, which *is* the position obtained timestamp (the
time we got the response from server is irrelevant)

http://codereview.chromium.org/552250/diff/6001/6006
File chrome/browser/geolocation/network_location_provider.h (right):

http://codereview.chromium.org/552250/diff/6001/6006#newcode37
chrome/browser/geolocation/network_location_provider.h:37: bool StartProvider();
On 2010/02/01 17:47:36, bulach wrote:
> private?

good point, but turns out it's better in public (base class) API (and not call
from ctor)

http://codereview.chromium.org/552250/diff/6001/6015
File chrome/browser/geolocation/network_location_request.cc (right):

http://codereview.chromium.org/552250/diff/6001/6015#newcode189
chrome/browser/geolocation/network_location_request.cc:189:
FormatPositionError(server_url, L"No response recieved", position);
On 2010/02/01 17:47:36, bulach wrote:
> s/recie/recei/

Done.

http://codereview.chromium.org/552250/diff/6001/6015#newcode210
chrome/browser/geolocation/network_location_request.cc:210: return;
On 2010/02/01 17:47:36, bulach wrote:
> unnecessary?

Done.

http://codereview.chromium.org/552250/diff/6001/6015#newcode256
chrome/browser/geolocation/network_location_request.cc:256: Value* value;
On 2010/02/01 17:47:36, bulach wrote:
> iirc, valgrind used to complain, so perhaps:
> Value* value = NULL;
> and DCHECK(value); below?

Done.

http://codereview.chromium.org/552250/diff/6001/6015#newcode296
chrome/browser/geolocation/network_location_request.cc:296: << error_msg <<
".\n";
On 2010/02/01 17:47:36, bulach wrote:
> indent?

Done.

http://codereview.chromium.org/552250/diff/6001/6005
File chrome/browser/geolocation/network_location_request.h (right):

http://codereview.chromium.org/552250/diff/6001/6005#newcode31
chrome/browser/geolocation/network_location_request.h:31: const string16&
access_token) = 0;
On 2010/02/01 17:47:36, bulach wrote:
> \n

Done.