Update Gears wifi data providers to Chrome style & APIs....

Update Gears wifi data providers to Chrome style & APIs.
Add wifi data provider to project.
Add tests for windows provider. (More to follow)

BUG=11246
TEST=unit_tests - wifi_data_provider_win_unittest.cc


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

[joth, 2010-01-22 18:50:37]

Currently this is work in progress.
Depends on http://codereview.chromium.org/546116. Uploaded this change just for
reference.

[joth, 2010-01-26 20:10:20]

This is all building and passing tests on windows now. I need to check the linux
& mac bots and make them green (I don't intend to completely finish those ports
in this patch, just make sure I don't break them)
Any comments you have at this point very welcome.

[bulach_, 2010-01-27 13:29:01]

hehe, deja vu on some of these pieces! :)

some general comments below, and since I'm not yet too familiar with the
threading / process model, I think jorlow will provide a much better comment on
the lock issue below:

http://codereview.chromium.org/543174/diff/4001/4003
File chrome/browser/geolocation/device_data_provider.h (right):

http://codereview.chromium.org/543174/diff/4001/4003#newcode141
chrome/browser/geolocation/device_data_provider.h:141: const AccessPointData
&data2) const {
nit: foo& bar rather than foo &bar.
(there's some more below, such as 192, 199, etc., for foo *bar).

http://codereview.chromium.org/543174/diff/4001/4003#newcode224
chrome/browser/geolocation/device_data_provider.h:224: void NotifyListeners() {
if I understood correctly, this is being called from the private thread on the
wifi provider, right?

Perhaps instead of a lock, we should check which thread we're running, and then
post if it's not the IO thread (or whatever thread we decide to run the main
geolocation listener / arbitrator)..

and similarly for Add/Remove listener?

http://codereview.chromium.org/543174/diff/4001/4003#newcode238
chrome/browser/geolocation/device_data_provider.h:238: Lock listeners_mutex_;
is this lock needed? can we make it so that listeners are added / removed in the
same thread?

http://codereview.chromium.org/543174/diff/4001/4003#newcode240
chrome/browser/geolocation/device_data_provider.h:240:
DISALLOW_EVIL_CONSTRUCTORS(DeviceDataProviderImplBase);
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/543174/diff/4001/4003#newcode286
chrome/browser/geolocation/device_data_provider.h:286: //
NetworkLocationProvider HTTP request has completed.
this needs updating, and same question for the lock

http://codereview.chromium.org/543174/diff/4001/4004
File chrome/browser/geolocation/wifi_data_provider_common.cc (right):

http://codereview.chromium.org/543174/diff/4001/4004#newcode16
chrome/browser/geolocation/wifi_data_provider_common.cc:16: mac_as_int[3],
mac_as_int[4], mac_as_int[5]));
indent 15:16

http://codereview.chromium.org/543174/diff/4001/4005
File chrome/browser/geolocation/wifi_data_provider_common.h (right):

http://codereview.chromium.org/543174/diff/4001/4005#newcode29
chrome/browser/geolocation/wifi_data_provider_common.h:29: class
DefaultPollingPolicy : public PollingPolicyInterface {
hmm.. since this is a template, perhaps "BasePollingPolicy", or
"GenericPollingPolicy" ? I'd expect "DefaultPollingPolicy" to be a template
instantiantion using default constant values.

http://codereview.chromium.org/543174/diff/4001/4007
File chrome/browser/geolocation/wifi_data_provider_win.cc (right):

http://codereview.chromium.org/543174/diff/4001/4007#newcode553
chrome/browser/geolocation/wifi_data_provider_win.cc:553: static bool
GetNetworkData(const WLAN_BSS_ENTRY &bss_entry,
already in a namespace, can remove static

http://codereview.chromium.org/543174/diff/4001/4007#newcode638
chrome/browser/geolocation/wifi_data_provider_win.cc:638: assert(requested_size
> 0);
assert(buffer);

http://codereview.chromium.org/543174/diff/4001/4009
File chrome/browser/geolocation/wifi_data_provider_win.h (right):

http://codereview.chromium.org/543174/diff/4001/4009#newcode19
chrome/browser/geolocation/wifi_data_provider_win.h:19: // Interface to anstract
the low level data OS library call, and to allow
typo, abstract

http://codereview.chromium.org/543174/diff/4001/4009#newcode31
chrome/browser/geolocation/wifi_data_provider_win.h:31: // Takes ownership of
mock. Must be called before Start().
s/mock/wlan_api/ (here and the param name as well)

also, s/Start()/StartDataProvider()/
(34 as well)

http://codereview.chromium.org/543174/diff/4001/4009#newcode35
chrome/browser/geolocation/wifi_data_provider_win.h:35: void
set_mock_polling_policy(PollingPolicyInterface* mock);
s/mock/polling_policy/

...hmm, perhaps instead of set_, how about "inject_"?

http://codereview.chromium.org/543174/diff/4001/4009#newcode70
chrome/browser/geolocation/wifi_data_provider_win.h:70: HINSTANCE library_;
is it needed here?

http://codereview.chromium.org/543174/diff/4001/4006
File chrome/browser/geolocation/wifi_data_provider_win_unittest.cc (right):

http://codereview.chromium.org/543174/diff/4001/4006#newcode54
chrome/browser/geolocation/wifi_data_provider_win_unittest.cc:54: // Can't call
Quit() driectly on another thread's message loop.
typo, directly

http://codereview.chromium.org/543174/diff/4001/4006#newcode74
chrome/browser/geolocation/wifi_data_provider_win_unittest.cc:74: }
\n

http://codereview.chromium.org/543174/diff/4001/4006#newcode130
chrome/browser/geolocation/wifi_data_provider_win_unittest.cc:130: }
wrap in a namespace?

http://codereview.chromium.org/543174/diff/4001/4011
File chrome/browser/geolocation/wifi_data_provider_windows_common.cc (right):

http://codereview.chromium.org/543174/diff/4001/4011#newcode13
chrome/browser/geolocation/wifi_data_provider_windows_common.cc:13: bool
ConvertToGearsFormat(const NDIS_WLAN_BSSID &data,
s/GearsFormat/AccessPointData/

[joth, 2010-01-27 16:40:46]

Thanks, think I addressed all your suggestions. uploading new snapshot...

http://codereview.chromium.org/543174/diff/4001/4003
File chrome/browser/geolocation/device_data_provider.h (right):

http://codereview.chromium.org/543174/diff/4001/4003#newcode141
chrome/browser/geolocation/device_data_provider.h:141: const AccessPointData
&data2) const {
On 2010/01/27 13:29:01, bulach_ wrote:
> nit: foo& bar rather than foo &bar.
> (there's some more below, such as 192, 199, etc., for foo *bar).

Done.

http://codereview.chromium.org/543174/diff/4001/4003#newcode224
chrome/browser/geolocation/device_data_provider.h:224: void NotifyListeners() {
On 2010/01/27 13:29:01, bulach_ wrote:
> if I understood correctly, this is being called from the private thread on the
> wifi provider, right?
> 
yep
> Perhaps instead of a lock, we should check which thread we're running, and
then
> post if it's not the IO thread (or whatever thread we decide to run the main
> geolocation listener / arbitrator)..
> 

I don't want the low level data providers to know about global chrome thread
names etc. the network location provider can do this. 
Hmmm I could either pass in the client's Thread* or grab MessageLoop::current()
in the constructor, and then post updates back to this thread/loop to implement
the broadcast there. Add/remove can assert that they're only called from this
captured thread.
I'm going to TODO this, it's an optimization for if/when we establish client
will always be single threaded.

> and similarly for Add/Remove listener?

http://codereview.chromium.org/543174/diff/4001/4003#newcode238
chrome/browser/geolocation/device_data_provider.h:238: Lock listeners_mutex_;
On 2010/01/27 13:29:01, bulach_ wrote:
> is this lock needed? can we make it so that listeners are added / removed in
the
> same thread?

ditto

http://codereview.chromium.org/543174/diff/4001/4003#newcode240
chrome/browser/geolocation/device_data_provider.h:240:
DISALLOW_EVIL_CONSTRUCTORS(DeviceDataProviderImplBase);
On 2010/01/27 13:29:01, bulach_ wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/543174/diff/4001/4003#newcode286
chrome/browser/geolocation/device_data_provider.h:286: //
NetworkLocationProvider HTTP request has completed.
On 2010/01/27 13:29:01, bulach_ wrote:
> this needs updating, and same question for the lock

Done.

http://codereview.chromium.org/543174/diff/4001/4004
File chrome/browser/geolocation/wifi_data_provider_common.cc (right):

http://codereview.chromium.org/543174/diff/4001/4004#newcode16
chrome/browser/geolocation/wifi_data_provider_common.cc:16: mac_as_int[3],
mac_as_int[4], mac_as_int[5]));
On 2010/01/27 13:29:01, bulach_ wrote:
> indent 15:16

Done.

http://codereview.chromium.org/543174/diff/4001/4005
File chrome/browser/geolocation/wifi_data_provider_common.h (right):

http://codereview.chromium.org/543174/diff/4001/4005#newcode29
chrome/browser/geolocation/wifi_data_provider_common.h:29: class
DefaultPollingPolicy : public PollingPolicyInterface {
On 2010/01/27 13:29:01, bulach_ wrote:
> hmm.. since this is a template, perhaps "BasePollingPolicy", or
> "GenericPollingPolicy" ? I'd expect "DefaultPollingPolicy" to be a template
> instantiantion using default constant values.

Done.

http://codereview.chromium.org/543174/diff/4001/4007
File chrome/browser/geolocation/wifi_data_provider_win.cc (right):

http://codereview.chromium.org/543174/diff/4001/4007#newcode553
chrome/browser/geolocation/wifi_data_provider_win.cc:553: static bool
GetNetworkData(const WLAN_BSS_ENTRY &bss_entry,
On 2010/01/27 13:29:01, bulach_ wrote:
> already in a namespace, can remove static

Done.

http://codereview.chromium.org/543174/diff/4001/4007#newcode638
chrome/browser/geolocation/wifi_data_provider_win.cc:638: assert(requested_size
> 0);
On 2010/01/27 13:29:01, bulach_ wrote:
> assert(buffer);

Done.

http://codereview.chromium.org/543174/diff/4001/4009
File chrome/browser/geolocation/wifi_data_provider_win.h (right):

http://codereview.chromium.org/543174/diff/4001/4009#newcode19
chrome/browser/geolocation/wifi_data_provider_win.h:19: // Interface to anstract
the low level data OS library call, and to allow
On 2010/01/27 13:29:01, bulach_ wrote:
> typo, abstract

Done.

http://codereview.chromium.org/543174/diff/4001/4009#newcode31
chrome/browser/geolocation/wifi_data_provider_win.h:31: // Takes ownership of
mock. Must be called before Start().
On 2010/01/27 13:29:01, bulach_ wrote:
> s/mock/wlan_api/ (here and the param name as well)
> 
> also, s/Start()/StartDataProvider()/
> (34 as well)

Done.

http://codereview.chromium.org/543174/diff/4001/4009#newcode35
chrome/browser/geolocation/wifi_data_provider_win.h:35: void
set_mock_polling_policy(PollingPolicyInterface* mock);
On 2010/01/27 13:29:01, bulach_ wrote:
> s/mock/polling_policy/
> 
> ...hmm, perhaps instead of set_, how about "inject_"?

Done.

http://codereview.chromium.org/543174/diff/4001/4009#newcode70
chrome/browser/geolocation/wifi_data_provider_win.h:70: HINSTANCE library_;
On 2010/01/27 13:29:01, bulach_ wrote:
> is it needed here?

Done.

http://codereview.chromium.org/543174/diff/4001/4006
File chrome/browser/geolocation/wifi_data_provider_win_unittest.cc (right):

http://codereview.chromium.org/543174/diff/4001/4006#newcode54
chrome/browser/geolocation/wifi_data_provider_win_unittest.cc:54: // Can't call
Quit() driectly on another thread's message loop.
On 2010/01/27 13:29:01, bulach_ wrote:
> typo, directly

Done.

http://codereview.chromium.org/543174/diff/4001/4006#newcode74
chrome/browser/geolocation/wifi_data_provider_win_unittest.cc:74: }
On 2010/01/27 13:29:01, bulach_ wrote:
> \n

Done.

http://codereview.chromium.org/543174/diff/4001/4006#newcode130
chrome/browser/geolocation/wifi_data_provider_win_unittest.cc:130: }
On 2010/01/27 13:29:01, bulach_ wrote:
> wrap in a namespace?

Done.

http://codereview.chromium.org/543174/diff/4001/4011
File chrome/browser/geolocation/wifi_data_provider_windows_common.cc (right):

http://codereview.chromium.org/543174/diff/4001/4011#newcode13
chrome/browser/geolocation/wifi_data_provider_windows_common.cc:13: bool
ConvertToGearsFormat(const NDIS_WLAN_BSSID &data,
On 2010/01/27 13:29:01, bulach_ wrote:
> s/GearsFormat/AccessPointData/

Done.

[jorlow, 2010-01-27 18:44:44]

I only skimmed, but looks pretty good to me.

http://codereview.chromium.org/543174/diff/4036/4049
File chrome/browser/geolocation/wifi_data_provider_linux.cc (right):

http://codereview.chromium.org/543174/diff/4036/4049#newcode257
chrome/browser/geolocation/wifi_data_provider_linux.cc:257: #endif  // LINUX &&
!OS_MACOSX
// 0

http://codereview.chromium.org/543174/diff/4036/4054
File chrome/browser/geolocation/wifi_data_provider_mac.cc (right):

http://codereview.chromium.org/543174/diff/4036/4054#newcode146
chrome/browser/geolocation/wifi_data_provider_mac.cc:146: #endif  // OS_MACOSX
// 0

http://codereview.chromium.org/543174/diff/4036/4051
File chrome/browser/geolocation/wifi_data_provider_osx.cc (left):

http://codereview.chromium.org/543174/diff/4036/4051#oldcode133
chrome/browser/geolocation/wifi_data_provider_osx.cc:133: if
(managed_access_points == NULL) {
Why did we delete this file?

[joth, 2010-01-27 19:00:57]

Thanks!

2010/1/27 <jorlow@chromium.org>

> I only skimmed, but looks pretty good to me.
>
>
> http://codereview.chromium.org/543174/diff/4036/4049
> File chrome/browser/geolocation/wifi_data_provider_linux.cc (right):
>
> http://codereview.chromium.org/543174/diff/4036/4049#newcode257
> chrome/browser/geolocation/wifi_data_provider_linux.cc:257: #endif  //
> LINUX && !OS_MACOSX
> // 0
>
> done


> http://codereview.chromium.org/543174/diff/4036/4054
> File chrome/browser/geolocation/wifi_data_provider_mac.cc (right):
>
> http://codereview.chromium.org/543174/diff/4036/4054#newcode146
> chrome/browser/geolocation/wifi_data_provider_mac.cc:146: #endif  //
> OS_MACOSX
> // 0
>
> done


> http://codereview.chromium.org/543174/diff/4036/4051
> File chrome/browser/geolocation/wifi_data_provider_osx.cc (left):
>
> http://codereview.chromium.org/543174/diff/4036/4051#oldcode133
> chrome/browser/geolocation/wifi_data_provider_osx.cc:133: if
> (managed_access_points == NULL) {
> Why did we delete this file?
>
>
> I renamed _osx.  =>  _mac.
and _windows_common.  =>  _common_win.
gyp seems to use the suffix to get the 'exclude' files correct correct?


http://codereview.chromium.org/543174
>