Add tests for the geolocation network provider...

Add tests for the geolocation network provider.
Also some small tidy up a few other files.

BUG=http://crbug.com/11246 
TEST=unit_tests.exe --gtest_filter=NetworkLocationProvider* --gtest_break_on_failure


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

[joth, 2010-02-02 21:32:28]

Here's a nice set of network provider tests to start off with. No worries if you
don't get chance to look at them Jeremy.

[bulach, 2010-02-03 11:52:15]

nice tests!! some comments below:

http://codereview.chromium.org/556106/diff/5004/4013
File chrome/browser/geolocation/empty_device_data_provider.cc (right):

http://codereview.chromium.org/556106/diff/5004/4013#newcode17
chrome/browser/geolocation/empty_device_data_provider.cc:17:
WifiDataProviderImplBase *WifiDataProvider::DefaultFactoryFunction() {
foo* bar (and above..)

http://codereview.chromium.org/556106/diff/5004/4005
File chrome/browser/geolocation/network_location_provider_unittest.cc (right):

http://codereview.chromium.org/556106/diff/5004/4005#newcode47
chrome/browser/geolocation/network_location_provider_unittest.cc:47:
message_loop_to_quit_->PostTask(FROM_HERE, new MessageLoop::QuitTask);
this method and the one above are identical, including the long comment.. :)
perhaps make them both call something like:
CheckMessageLoopAndQuit(provider);

http://codereview.chromium.org/556106/diff/5004/4005#newcode104
chrome/browser/geolocation/network_location_provider_unittest.cc:104: virtual
bool GetData(DataType *data_out) {
foo* bar (and on 112)

http://codereview.chromium.org/556106/diff/5004/4005#newcode113
chrome/browser/geolocation/network_location_provider_unittest.cc:113:
data_mutex_.Acquire();
AutoLock as above?

http://codereview.chromium.org/556106/diff/5004/4005#newcode152
chrome/browser/geolocation/network_location_provider_unittest.cc:152:
LocationProviderBase* CreateProvider(const GURL& url = GURL(kTestServerUrl)) {
hmm, looks like all of them are not even passing this param.. 
perhaps remove the param altogether, or just check for is_empty() instead of
having a default param (is it allowed?)

http://codereview.chromium.org/556106/diff/5004/4005#newcode166
chrome/browser/geolocation/network_location_provider_unittest.cc:166: int
IndexToChannal(int index) { return index + 4; }
namespace around these (but as for CheckRequestIsValid, perhaps make all of them
part of the fixture?)

http://codereview.chromium.org/556106/diff/5004/4005#newcode192
chrome/browser/geolocation/network_location_provider_unittest.cc:192: const
char* expected_access_token = NULL) {
same comments about default params, and perhaps make this method part of the
fixture rather than a free function?

http://codereview.chromium.org/556106/diff/5004/4005#newcode268
chrome/browser/geolocation/network_location_provider_unittest.cc:268: #define
REFERENCE_ACCESS_TOKEN "2:k7j3G6LaL6u_lafw:4iXOeOpTh1glSXe"
tricky, but I guess having as a regular const string and then having format
where needed wouldn't be as readable..

http://codereview.chromium.org/556106/diff/5004/4005#newcode269
chrome/browser/geolocation/network_location_provider_unittest.cc:269: static
const char* kNoFixNetworkResponse =
no need for static

http://codereview.chromium.org/556106/diff/5004/4005#newcode290
chrome/browser/geolocation/network_location_provider_unittest.cc:290: static
const int kFirstScanAps = 6;
s/static//

[joth, 2010-02-03 18:32:02]

Thanks, all done. Will commit when the light goes green....

http://codereview.chromium.org/556106/diff/5004/4013
File chrome/browser/geolocation/empty_device_data_provider.cc (right):

http://codereview.chromium.org/556106/diff/5004/4013#newcode17
chrome/browser/geolocation/empty_device_data_provider.cc:17:
WifiDataProviderImplBase *WifiDataProvider::DefaultFactoryFunction() {
On 2010/02/03 11:52:16, bulach wrote:
> foo* bar (and above..)

Done.

http://codereview.chromium.org/556106/diff/5004/4005
File chrome/browser/geolocation/network_location_provider_unittest.cc (right):

http://codereview.chromium.org/556106/diff/5004/4005#newcode47
chrome/browser/geolocation/network_location_provider_unittest.cc:47:
message_loop_to_quit_->PostTask(FROM_HERE, new MessageLoop::QuitTask);
On 2010/02/03 11:52:16, bulach wrote:
> this method and the one above are identical, including the long comment.. :)
> perhaps make them both call something like:
> CheckMessageLoopAndQuit(provider);
Much less duplication in them now...

http://codereview.chromium.org/556106/diff/5004/4005#newcode104
chrome/browser/geolocation/network_location_provider_unittest.cc:104: virtual
bool GetData(DataType *data_out) {
On 2010/02/03 11:52:16, bulach wrote:
> foo* bar (and on 112)

Done.

http://codereview.chromium.org/556106/diff/5004/4005#newcode113
chrome/browser/geolocation/network_location_provider_unittest.cc:113:
data_mutex_.Acquire();
On 2010/02/03 11:52:16, bulach wrote:
> AutoLock as above?

Considered it - I need to release the lock before notifying, but I need the bool
to be scoped outside the lock, so on balance I found this version easier to
follow.

http://codereview.chromium.org/556106/diff/5004/4005#newcode152
chrome/browser/geolocation/network_location_provider_unittest.cc:152:
LocationProviderBase* CreateProvider(const GURL& url = GURL(kTestServerUrl)) {
On 2010/02/03 11:52:16, bulach wrote:
> hmm, looks like all of them are not even passing this param.. 
> perhaps remove the param altogether, or just check for is_empty() instead of
> having a default param (is it allowed?)

For simplicity, I made it a const member of the fixture

http://codereview.chromium.org/556106/diff/5004/4005#newcode166
chrome/browser/geolocation/network_location_provider_unittest.cc:166: int
IndexToChannal(int index) { return index + 4; }
On 2010/02/03 11:52:16, bulach wrote:
> namespace around these (but as for CheckRequestIsValid, perhaps make all of
them
> part of the fixture?)

Done.

http://codereview.chromium.org/556106/diff/5004/4005#newcode192
chrome/browser/geolocation/network_location_provider_unittest.cc:192: const
char* expected_access_token = NULL) {
On 2010/02/03 11:52:16, bulach wrote:
> same comments about default params, and perhaps make this method part of the
> fixture rather than a free function?

Done.

http://codereview.chromium.org/556106/diff/5004/4005#newcode268
chrome/browser/geolocation/network_location_provider_unittest.cc:268: #define
REFERENCE_ACCESS_TOKEN "2:k7j3G6LaL6u_lafw:4iXOeOpTh1glSXe"
On 2010/02/03 11:52:16, bulach wrote:
> tricky, but I guess having as a regular const string and then having format
> where needed wouldn't be as readable..

Done (added comment)

http://codereview.chromium.org/556106/diff/5004/4005#newcode269
chrome/browser/geolocation/network_location_provider_unittest.cc:269: static
const char* kNoFixNetworkResponse =
On 2010/02/03 11:52:16, bulach wrote:
> no need for static

Done.

http://codereview.chromium.org/556106/diff/5004/4005#newcode290
chrome/browser/geolocation/network_location_provider_unittest.cc:290: static
const int kFirstScanAps = 6;
On 2010/02/03 11:52:16, bulach wrote:
> s/static//

Done.