Move wifi_data_provider_chromeos.* from chrome\browser\geolocation to content\browser\geolocation b…

Move wifi_data_provider_chromeos.* from chrome\browser\geolocation to content\browser\geolocation beside the other wifi_data_provider implementations. It turns out that this implementation actually wasn't in use all this time (the overriding of the factory using a global constructor doesn't seem to be called).

I removed wifi_data_provider_unittest_chromeos.cc since the test now would be pointless (checking that a method is called).

Superceeded by https://chromiumcodereview.appspot.com/11881011


BUG=98716, 156428

[jam, 2013-01-04 19:01:18]

Steven: I believe from our IMs you'd rather this wait until you check in a
change for an alternate way of knowing when wifi is enabled (i.e. so I don't
call chromeos::NetworkStateHandler, but probably the NetworkDeviceHandler you're
adding in https://codereview.chromium.org/11743006/ ?). so this may not be ready
to land now until that's ready, but I figured I'd send this out for review now
as the other parts should be the same.

https://codereview.chromium.org/11710002/diff/55004/content/browser/geolocati...
File content/browser/geolocation/device_data_provider.h (left):

https://codereview.chromium.org/11710002/diff/55004/content/browser/geolocati...
content/browser/geolocation/device_data_provider.h:129:
DCHECK(CalledOnClientThread());
this is needed because in some tests, there are outstanding tasks that keep a
reference to the parent class at shutdown. As a result the parent class gets
destructed on the UI thread instead of the geolocation thread. This only happens
with the chromeos implementation, but it didn't come up before because that
implementation wasn't actually being used on cros (the linux one was).

[joth, 2013-01-04 19:12:43]

Thanks! This looks a much more consistent layering with the other platforms now.

Adding jknotten for the geolocation/ l-g-t-m

[stevenjb, 2013-01-04 20:41:03]

https://codereview.chromium.org/11710002/diff/55004/content/browser/geolocati...
File content/browser/geolocation/wifi_data_provider_chromeos.cc (right):

https://codereview.chromium.org/11710002/diff/55004/content/browser/geolocati...
content/browser/geolocation/wifi_data_provider_chromeos.cc:153: return false;
So, this is OK, we can replace it when it has been re-factored (which I've
started work on)

https://codereview.chromium.org/11710002/diff/55004/content/browser/geolocati...
content/browser/geolocation/wifi_data_provider_chromeos.cc:164:
TechnologyEnabled(flimflam::kTypeWifi);
This is behind a flag and will crash without --enable-new-network-handlers
(chromeos::NetworkStateHandler::Get() will return NULL).

There are two choices here: a) wait for my re-factor which will work with or
without the flag, or b) write a Cros* function to do a blocking call to get the
TechnologyEnabled state.

Also: It doesn't make sense to me to generate the access point data if wifi is
not enabled; we should do this check early. Plus, won't this always return
|false| if wifi is disabled, causing the code to continuously scan? Maybe that's
not a big deal?

[jam, 2013-01-04 21:18:31]

https://codereview.chromium.org/11710002/diff/55004/content/browser/geolocati...
File content/browser/geolocation/wifi_data_provider_chromeos.cc (right):

https://codereview.chromium.org/11710002/diff/55004/content/browser/geolocati...
content/browser/geolocation/wifi_data_provider_chromeos.cc:164:
TechnologyEnabled(flimflam::kTypeWifi);
On 2013/01/04 20:41:04, stevenjb (chromium) wrote:
> This is behind a flag and will crash without --enable-new-network-handlers
> (chromeos::NetworkStateHandler::Get() will return NULL).
> 
> There are two choices here: a) wait for my re-factor which will work with or
> without the flag, or b) write a Cros* function to do a blocking call to get
the
> TechnologyEnabled state.

my plan was just to wait for your refactor

> 
> Also: It doesn't make sense to me to generate the access point data if wifi is
> not enabled; we should do this check early.

sure I can move the order. i think before with NetworkLibrary this was a very
cheap check which is probably why it was written like this

> Plus, won't this always return
> |false| if wifi is disabled, causing the code to continuously scan? Maybe
that's
> not a big deal?

joth or jknotten can confirm, but my reading of the code is that this was the
intended behavior

[joth, 2013-01-04 22:05:30]

On 4 January 2013 13:18, <jam@chromium.org> wrote:

>
> https://codereview.chromium.**org/11710002/diff/55004/**
>
content/browser/geolocation/**wifi_data_provider_chromeos.cc<https://codereview.chromium.org/11710002/diff/55004/content/browser/geolocation/wifi_data_provider_chromeos.cc>
> File content/browser/geolocation/**wifi_data_provider_chromeos.cc (right):
>
> https://codereview.chromium.**org/11710002/diff/55004/**
>
content/browser/geolocation/**wifi_data_provider_chromeos.**cc#newcode164<https://codereview.chromium.org/11710002/diff/55004/content/browser/geolocation/wifi_data_provider_chromeos.cc#newcode164>
> content/browser/geolocation/**wifi_data_provider_chromeos.**cc:164:
> TechnologyEnabled(flimflam::**kTypeWifi);
> On 2013/01/04 20:41:04, stevenjb (chromium) wrote:
>
>> This is behind a flag and will crash without
>>
> --enable-new-network-handlers
>
>> (chromeos::**NetworkStateHandler::Get() will return NULL).
>>
>
>  There are two choices here: a) wait for my re-factor which will work
>>
> with or
>
>> without the flag, or b) write a Cros* function to do a blocking call
>>
> to get the
>
>> TechnologyEnabled state.
>>
>
> my plan was just to wait for your refactor
>
>
>
>  Also: It doesn't make sense to me to generate the access point data if
>>
> wifi is
>
>> not enabled; we should do this check early.
>>
>
> sure I can move the order. i think before with NetworkLibrary this was a
> very cheap check which is probably why it was written like this
>
>
>  Plus, won't this always return
>> |false| if wifi is disabled,
>
>
By 'this', you mean the change proposed?


> causing the code to continuously scan?
>>
> Maybe that's
>
>> not a big deal?
>>
>
> joth or jknotten can confirm, but my reading of the code is that this
> was the intended behavior
>
>
yeah GetAccessPointData() could use a bit more documentation,  but IIRC
 the general gist is returning false means we know wifi is disabled for
sure, and so it polls on a slow cycle to detect wifi turning on (there's
issues with that, but that's another discussion)

So if we get results *or* we know wifi is definitely on, we return 'true'
to keep the higher frequency poll going. (some platforms that have caching
will return valid results for a while even after wifi was turned off, IIRC)

jknotten may fill in details I've forgotten though...



>
https://codereview.chromium.**org/11710002/<https://codereview.chromium.org/1...
>

[John Knottenbelt, 2013-01-07 10:39:40]

On 2013/01/04 22:05:30, joth wrote:
> On 4 January 2013 13:18, <mailto:jam@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/11710002/diff/55004/**
> >
>
content/browser/geolocation/**wifi_data_provider_chromeos.cc<https://codereview.chromium.org/11710002/diff/55004/content/browser/geolocation/wifi_data_provider_chromeos.cc>
> > File content/browser/geolocation/**wifi_data_provider_chromeos.cc (right):
> >
> > https://codereview.chromium.**org/11710002/diff/55004/**
> >
>
content/browser/geolocation/**wifi_data_provider_chromeos.**cc#newcode164<https://codereview.chromium.org/11710002/diff/55004/content/browser/geolocation/wifi_data_provider_chromeos.cc#newcode164>
> > content/browser/geolocation/**wifi_data_provider_chromeos.**cc:164:
> > TechnologyEnabled(flimflam::**kTypeWifi);
> > On 2013/01/04 20:41:04, stevenjb (chromium) wrote:
> >
> >> This is behind a flag and will crash without
> >>
> > --enable-new-network-handlers
> >
> >> (chromeos::**NetworkStateHandler::Get() will return NULL).
> >>
> >
> >  There are two choices here: a) wait for my re-factor which will work
> >>
> > with or
> >
> >> without the flag, or b) write a Cros* function to do a blocking call
> >>
> > to get the
> >
> >> TechnologyEnabled state.
> >>
> >
> > my plan was just to wait for your refactor
> >
> >
> >
> >  Also: It doesn't make sense to me to generate the access point data if
> >>
> > wifi is
> >
> >> not enabled; we should do this check early.
> >>
> >
> > sure I can move the order. i think before with NetworkLibrary this was a
> > very cheap check which is probably why it was written like this
> >
> >
> >  Plus, won't this always return
> >> |false| if wifi is disabled,
> >
> >
> By 'this', you mean the change proposed?
> 
> 
> > causing the code to continuously scan?
> >>
> > Maybe that's
> >
> >> not a big deal?
> >>
> >
> > joth or jknotten can confirm, but my reading of the code is that this
> > was the intended behavior
> >
> >
> yeah GetAccessPointData() could use a bit more documentation,  but IIRC
>  the general gist is returning false means we know wifi is disabled for
> sure, and so it polls on a slow cycle to detect wifi turning on (there's
> issues with that, but that's another discussion)
> 
> So if we get results *or* we know wifi is definitely on, we return 'true'
> to keep the higher frequency poll going. (some platforms that have caching
> will return valid results for a while even after wifi was turned off, IIRC)
> 
> jknotten may fill in details I've forgotten though...
> 
> 
> 
> >
>
https://codereview.chromium.**org/11710002/%3Chttps://codereview.chromium.org...>
> >


Yes, I agree with Joth about GetAccessPointData().

LGTM.

[jam, 2013-01-09 02:35:38]

Steven: is https://codereview.chromium.org/11743006/ is the change that I'm
supposed to use to check if wifi is enabled? If so, how do I use it? i.e. I
don't see how to get to that new class, and do I need to check devices_ready()
or can I go to devices() right away?

Thanks

[stevenjb (google-dont-use), 2013-01-09 02:53:59]

That's Step 1. I have a follow-up CL in progress.




On Tue, Jan 8, 2013 at 7:35 PM, <jam@chromium.org> wrote:

> Steven: is
https://codereview.chromium.**org/11743006/<https://codereview.chromium.org/1...
the change that I'm
> supposed to use to check if wifi is enabled? If so, how do I use it? i.e. I
> don't see how to get to that new class, and do I need to check
> devices_ready()
> or can I go to devices() right away?
>
> Thanks
>
>
https://codereview.chromium.**org/11710002/<https://codereview.chromium.org/1...
>

[stevenjb, 2013-01-24 01:20:43]

This CL was superceeded by https://chromiumcodereview.appspot.com/11881011,
closing this one.