Refactor WifiDataProviderChromeOs to implement WifiDataProviderImplBase directly.

Refactor WifiDataProviderChromeOs to implement WifiDataProviderImplBase directly and delegate the 
scan occur on the UI thread as currently required by 
Chromium OS.

BUG=chromium-os:13049
TEST=Existing

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

[John Knottenbelt, 2011-03-15 16:29:12]

Hi Marcus,

Can you take a look at this and let me know your thoughts. It is with respect to
http://code.google.com/p/chromium-os/issues/detail?id=13049

Thanks

John

[bulach, 2011-03-15 18:37:51]

thanks john! 
a few nits, and a possible deal breaker concern, please let me know if I
misunderstood the flow (quite possible.. :)

http://codereview.chromium.org/6696022/diff/1/content/browser/geolocation/wif...
File content/browser/geolocation/wifi_data_provider_chromeos.cc (right):

http://codereview.chromium.org/6696022/diff/1/content/browser/geolocation/wif...
content/browser/geolocation/wifi_data_provider_chromeos.cc:85:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
hmm, not sure this would work.. afaict, this would run on the Geolocation
thread, right?

if I followed the chain correctly:
GeolocationProvider is a thread, it creates the GeolocationArbitrator in its
Init() method, and then on GeolocationArbitrator::OnAccessTokenStoresLoaded
it'll create the NewNetworkLocationProvider?

this check would then fail.. (similarly for the other methods...).

I think it's ok to remove these checks, and then only do the thread bouncing on
DoWifiScanTask / a couple of helper methods?

http://codereview.chromium.org/6696022/diff/1/content/browser/geolocation/wif...
content/browser/geolocation/wifi_data_provider_chromeos.cc:161:
task_factory_.NewRunnableMethod(&WifiDataProviderChromeOs::DoWifiScanTask),
nit: >80cols

http://codereview.chromium.org/6696022/diff/1/content/browser/geolocation/wif...
File content/browser/geolocation/wifi_data_provider_chromeos.h (right):

http://codereview.chromium.org/6696022/diff/1/content/browser/geolocation/wif...
content/browser/geolocation/wifi_data_provider_chromeos.h:19: //
WifiDataProviderImplBase implementation
nit: maybe remove "implementation"?

http://codereview.chromium.org/6696022/diff/1/content/browser/geolocation/wif...
content/browser/geolocation/wifi_data_provider_chromeos.h:49: // Whether we have
strated the data provider.
nit:/strated/started/

[stevenjb, 2011-03-16 01:06:26]

http://codereview.chromium.org/6696022/diff/1/content/browser/geolocation/wif...
File content/browser/geolocation/wifi_data_provider_chromeos.cc (right):

http://codereview.chromium.org/6696022/diff/1/content/browser/geolocation/wif...
content/browser/geolocation/wifi_data_provider_chromeos.cc:85:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
As bulach@ suggests, this CHECK gets triggered on ChromeOS.

http://codereview.chromium.org/6696022/diff/1/content/browser/geolocation/wif...
content/browser/geolocation/wifi_data_provider_chromeos.cc:162: interval);
I'm still unfamiliar with Chrome threading, but is it possible / does it make
any sense to issue DoWifiScanTask from the UI thread there?

[John Knottenbelt, 2011-03-16 16:18:27]

This patch keeps most of the logic on the Geolocation thread, delegating to the
UI thread when we need to do the scan.

I think that an alternative and better fix for this issue would be to change
chromeos::NetworkLibraryWlanApi so that it can be called from any thread, so
that we don't end up blocking the user interface while the scan proceeds.
However, I'm not sure how easy this would be.

[stevenjb, 2011-03-16 17:46:56]

This patch seems to do the trick.

I am working on a patch that will replace NetworkLibrary::GetWifiAccessPoints
with an asynchronous version which will not block the UI thread. 

lgtm with one comment.

http://codereview.chromium.org/6696022/diff/4001/content/browser/geolocation/...
File content/browser/geolocation/device_data_provider.h (right):

http://codereview.chromium.org/6696022/diff/4001/content/browser/geolocation/...
content/browser/geolocation/device_data_provider.h:235: MessageLoop*
client_loop_;
We shouldn't expose this publicly if we only need to access it. Instead:
MessageLoop* client_loop() const { return client_loop_; }

[bulach, 2011-03-16 18:01:00]

LGTM

good stuff, thanks john!

a few comments below consolidating our chat:

http://codereview.chromium.org/6696022/diff/4001/content/browser/geolocation/...
File content/browser/geolocation/wifi_data_provider_chromeos.cc (right):

http://codereview.chromium.org/6696022/diff/4001/content/browser/geolocation/...
content/browser/geolocation/wifi_data_provider_chromeos.cc:112: bool
WifiDataProviderChromeOs::GetData(WifiData *data) {
nit: WifiData* data

http://codereview.chromium.org/6696022/diff/4001/content/browser/geolocation/...
content/browser/geolocation/wifi_data_provider_chromeos.cc:148:
new_data.release()));
heh, the thing is that whilst in flight, new_data has will be orphan and leak if
the task gets cancelled before running..
but fear not! :) afaict WifiData is a simple copy-constructable struct, so
instead of a ptr, just pass by value...

and as we chatted, have another method for "DidEmptyWifiScan", or something..

http://codereview.chromium.org/6696022/diff/4001/content/browser/geolocation/...
content/browser/geolocation/wifi_data_provider_chromeos.cc:151: void
WifiDataProviderChromeOs::DidWifiScanTask(WifiData* new_data) {
and this can be come a const WifiData& new_data

http://codereview.chromium.org/6696022/diff/4001/content/browser/geolocation/...
File content/browser/geolocation/wifi_data_provider_chromeos.h (right):

http://codereview.chromium.org/6696022/diff/4001/content/browser/geolocation/...
content/browser/geolocation/wifi_data_provider_chromeos.h:19: //
WifiDataProviderImplBase implementation
nit: maybe remove "implementation"

[John Knottenbelt, 2011-03-16 18:20:14]

This includes Steven and Marcus' suggestions.

http://codereview.chromium.org/6696022/diff/4001/content/browser/geolocation/...
File content/browser/geolocation/device_data_provider.h (right):

http://codereview.chromium.org/6696022/diff/4001/content/browser/geolocation/...
content/browser/geolocation/device_data_provider.h:235: MessageLoop*
client_loop_;
On 2011/03/16 17:46:56, Steven Bennetts wrote:
> We shouldn't expose this publicly if we only need to access it. Instead:
> MessageLoop* client_loop() const { return client_loop_; }

Done.

http://codereview.chromium.org/6696022/diff/4001/content/browser/geolocation/...
File content/browser/geolocation/wifi_data_provider_chromeos.cc (right):

http://codereview.chromium.org/6696022/diff/4001/content/browser/geolocation/...
content/browser/geolocation/wifi_data_provider_chromeos.cc:112: bool
WifiDataProviderChromeOs::GetData(WifiData *data) {
On 2011/03/16 18:01:00, bulach wrote:
> nit: WifiData* data

Done.

http://codereview.chromium.org/6696022/diff/4001/content/browser/geolocation/...
content/browser/geolocation/wifi_data_provider_chromeos.cc:148:
new_data.release()));
On 2011/03/16 18:01:00, bulach wrote:
> heh, the thing is that whilst in flight, new_data has will be orphan and leak
if
> the task gets cancelled before running..
> but fear not! :) afaict WifiData is a simple copy-constructable struct, so
> instead of a ptr, just pass by value...
> 
> and as we chatted, have another method for "DidEmptyWifiScan", or something..

Done.

http://codereview.chromium.org/6696022/diff/4001/content/browser/geolocation/...
content/browser/geolocation/wifi_data_provider_chromeos.cc:151: void
WifiDataProviderChromeOs::DidWifiScanTask(WifiData* new_data) {
On 2011/03/16 18:01:00, bulach wrote:
> and this can be come a const WifiData& new_data

Done.

[bulach, 2011-03-16 18:26:00]

LGTM

thanks, John!
Steven, would you mind trying the latest patch please?

[stevenjb, 2011-03-16 19:50:31]

Tested the patch in Debug mode to catch any DCHECKs. No issues, looks good. Do
submit a 'git try -b linux_chromeos' though.

[stevenjb, 2011-03-16 20:56:01]

I spoke too soon. A Debug build is now failing here:
...
NetworkLibraryImpl::GetWifiAccessPoints()
NetworkLibraryWlanApi::GetAccessPointData()
WifiDataProviderCommon::DoWifiScanTask()
...

[stevenjb, 2011-03-16 21:01:47]

Sorry, that' can't be right, this must not have the patch. Hang on while I
rebuild and re-test.

[stevenjb, 2011-03-16 21:09:17]

OK, my first testing round was correct, all is well. Cheers!

[stevenjb, 2011-03-16 21:29:46]

Also, this is a top ChromeOS crasher for R11, so please drover this to the 696
branch (or ping me if you would like me to do it). Thanks!

[John Knottenbelt, 2011-03-17 13:59:25]

This solves the Geolocation test failure. The other test failures appear to be
unrelated.

[John Knottenbelt, 2011-03-17 16:04:25]

This has now been committed to Chrome trunk (r78540) and to branch 696 (r78553)