Refactor the state-machine & threading out of the windows wifi provider into ...

Refactor the state-machine & threading out of the windows wifi provider into a cross platform base class
This will make the linux & mac impementations much simpler

BUG=11246
TEST=Aded wifi_data_provider_common_unittest.cc. Run: unit_tests.exe --gtest_filer=Geoloc*



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

[bulach, 2010-02-12 10:42:25]

nice refactoring! this will surely make things easier for other platforms. some
nits and suggestions below:

http://codereview.chromium.org/604019/diff/5001/4003
File chrome/browser/geolocation/wifi_data_provider_common.cc (right):

http://codereview.chromium.org/604019/diff/5001/4003#newcode79
chrome/browser/geolocation/wifi_data_provider_common.cc:79: update_available =
wifi_data_.DiffersSignificantly(new_data);
since it's not a block, perhaps declare the bool here and remove 77?

http://codereview.chromium.org/604019/diff/5001/4004
File chrome/browser/geolocation/wifi_data_provider_common.h (right):

http://codereview.chromium.org/604019/diff/5001/4004#newcode72
chrome/browser/geolocation/wifi_data_provider_common.h:72: virtual bool
GetAccessPointData(WifiData::AccessPointDataSet *data) = 0;
foo* bar

http://codereview.chromium.org/604019/diff/5001/4002
File chrome/browser/geolocation/wifi_data_provider_common_unittest.cc (right):

http://codereview.chromium.org/604019/diff/5001/4002#newcode47
chrome/browser/geolocation/wifi_data_provider_common_unittest.cc:47: class
WifiDataProviderToTest : public WifiDataProviderCommon {
perhaps: 
s/WifiDataProviderToTest/WifiDataProviderCommonWithMock?
reason: ..ToTest seems this is a generic thing that could test different
"WifiDataProvider", but in fact it's the other way around as it injects a mock
in WifiDataProviderCommon..

http://codereview.chromium.org/604019/diff/5001/4002#newcode67
chrome/browser/geolocation/wifi_data_provider_common_unittest.cc:67:
WifiDataProviderImplBase* CreateWifiDataProviderToTest() {
yeah, this would be clearer as well..

http://codereview.chromium.org/604019/diff/5001/4005
File chrome/browser/geolocation/wifi_data_provider_unittest_win.cc (right):

http://codereview.chromium.org/604019/diff/5001/4005#newcode19
chrome/browser/geolocation/wifi_data_provider_unittest_win.cc:19: // it
accessing hardware and making the unittest unpredictable / brittle.
s/"unpredictable / brittle"/flaky/

[joth, 2010-02-12 11:28:29]

Thanks! Will submit once it's all green...

http://codereview.chromium.org/604019/diff/5001/4003
File chrome/browser/geolocation/wifi_data_provider_common.cc (right):

http://codereview.chromium.org/604019/diff/5001/4003#newcode79
chrome/browser/geolocation/wifi_data_provider_common.cc:79: update_available =
wifi_data_.DiffersSignificantly(new_data);
On 2010/02/12 10:42:26, bulach wrote:
> since it's not a block, perhaps declare the bool here and remove 77?

Done.

http://codereview.chromium.org/604019/diff/5001/4004
File chrome/browser/geolocation/wifi_data_provider_common.h (right):

http://codereview.chromium.org/604019/diff/5001/4004#newcode72
chrome/browser/geolocation/wifi_data_provider_common.h:72: virtual bool
GetAccessPointData(WifiData::AccessPointDataSet *data) = 0;
On 2010/02/12 10:42:26, bulach wrote:
> foo* bar

Done.

http://codereview.chromium.org/604019/diff/5001/4002
File chrome/browser/geolocation/wifi_data_provider_common_unittest.cc (right):

http://codereview.chromium.org/604019/diff/5001/4002#newcode47
chrome/browser/geolocation/wifi_data_provider_common_unittest.cc:47: class
WifiDataProviderToTest : public WifiDataProviderCommon {
On 2010/02/12 10:42:26, bulach wrote:
> perhaps: 
> s/WifiDataProviderToTest/WifiDataProviderCommonWithMock?
> reason: ..ToTest seems this is a generic thing that could test different
> "WifiDataProvider", but in fact it's the other way around as it injects a mock
> in WifiDataProviderCommon..
Yeah I originally though of WifiDataProviderUnderTest
WifiDataProviderCommonWithMock is better.

http://codereview.chromium.org/604019/diff/5001/4002#newcode67
chrome/browser/geolocation/wifi_data_provider_common_unittest.cc:67:
WifiDataProviderImplBase* CreateWifiDataProviderToTest() {
On 2010/02/12 10:42:26, bulach wrote:
> yeah, this would be clearer as well..

Done.

http://codereview.chromium.org/604019/diff/5001/4005
File chrome/browser/geolocation/wifi_data_provider_unittest_win.cc (right):

http://codereview.chromium.org/604019/diff/5001/4005#newcode19
chrome/browser/geolocation/wifi_data_provider_unittest_win.cc:19: // it
accessing hardware and making the unittest unpredictable / brittle.
On 2010/02/12 10:42:26, bulach wrote:
> s/"unpredictable / brittle"/flaky/

Done.

[jorlow, 2010-02-14 22:09:27]

A couple comments to roll into your next patch.

http://codereview.chromium.org/604019/diff/3005/2010
File chrome/browser/geolocation/wifi_data_provider_common.cc (right):

http://codereview.chromium.org/604019/diff/3005/2010#newcode20
chrome/browser/geolocation/wifi_data_provider_common.cc:20: : Thread(__FILE__),
Hm...this seems kind of non-standard.

http://codereview.chromium.org/604019/diff/3005/2010#newcode22
chrome/browser/geolocation/wifi_data_provider_common.cc:22:
ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)) {
I think usually this goes inside the task_factor_()

http://codereview.chromium.org/604019/diff/3005/2011
File chrome/browser/geolocation/wifi_data_provider_common.h (right):

http://codereview.chromium.org/604019/diff/3005/2011#newcode120
chrome/browser/geolocation/wifi_data_provider_common.h:120: 
2 newlines

[jorlow, 2010-02-14 22:48:22]

http://codereview.chromium.org/604019/diff/3005/2010
File chrome/browser/geolocation/wifi_data_provider_common.cc (right):

http://codereview.chromium.org/604019/diff/3005/2010#newcode22
chrome/browser/geolocation/wifi_data_provider_common.cc:22:
ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)) {
On 2010/02/14 22:09:27, Jeremy Orlow wrote:
> I think usually this goes inside the task_factor_()

never mind

[joth, 2010-02-15 11:28:53]

Thanks, will roll into my next patch.

http://codereview.chromium.org/604019/diff/3005/2010
File chrome/browser/geolocation/wifi_data_provider_common.cc (right):

http://codereview.chromium.org/604019/diff/3005/2010#newcode20
chrome/browser/geolocation/wifi_data_provider_common.cc:20: : Thread(__FILE__),
On 2010/02/14 22:09:27, Jeremy Orlow wrote:
> Hm...this seems kind of non-standard.

Good point, I did this as a placeholder and never looked back at it.
"Geolocation_wifi_provider" it is.

http://codereview.chromium.org/604019/diff/3005/2010#newcode22
chrome/browser/geolocation/wifi_data_provider_common.cc:22:
ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)) {
On 2010/02/14 22:48:22, Jeremy Orlow wrote:
> On 2010/02/14 22:09:27, Jeremy Orlow wrote:
> > I think usually this goes inside the task_factor_()
> 
> never mind

Now you point it out I do prefer the foo_(ALLOW_THIS_IN_INITIALIZER_LIST(this))
form, but it does seem it's much more common to do it the way I already have it.

http://codereview.chromium.org/604019/diff/3005/2011
File chrome/browser/geolocation/wifi_data_provider_common.h (right):

http://codereview.chromium.org/604019/diff/3005/2011#newcode120
chrome/browser/geolocation/wifi_data_provider_common.h:120: 
On 2010/02/14 22:09:27, Jeremy Orlow wrote:
> 2 newlines

Done.