Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(224)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 22866005: Remove threading from WifiDataProviderCommon. (Closed)

Created:
7 years, 4 months ago by Michael van Ouwerkerk
Modified:
7 years, 4 months ago
Reviewers:
joth, bulach, stevenjb
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, Michael van Ouwerkerk, Miguel Garcia
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Remove threading from WifiDataProviderCommon. Device data providers no longer need to implement base::Thread as all of geolocation has its own thread. The common code no longer straddles multiple threads. Only WifiDataProviderChromeOs needs to continue being careful as apparently it needs to run the wifi scans on the ui thread. BUG=60739 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217793

Patch Set 1 #

Patch Set 2 : Use client_loop instead of message_loop. #

Total comments: 6

Patch Set 3 : Address review comments. #

Patch Set 4 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+49 lines, -93 lines) Patch
M content/browser/geolocation/device_data_provider.h View 1 2 12 chunks +21 lines, -29 lines 0 comments Download
M content/browser/geolocation/empty_device_data_provider.h View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/geolocation/network_location_provider_unittest.cc View 1 chunk +1 line, -2 lines 0 comments Download
M content/browser/geolocation/wifi_data_provider_chromeos.h View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/geolocation/wifi_data_provider_chromeos.cc View 2 chunks +1 line, -2 lines 0 comments Download
M content/browser/geolocation/wifi_data_provider_common.h View 5 chunks +8 lines, -19 lines 0 comments Download
M content/browser/geolocation/wifi_data_provider_common.cc View 1 2 3 chunks +13 lines, -36 lines 0 comments Download
M content/browser/geolocation/wifi_data_provider_common_unittest.cc View 3 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
Michael van Ouwerkerk
7 years, 4 months ago (2013-08-13 15:48:15 UTC) #1
Michael van Ouwerkerk
Adding Steven for Chrome OS.
7 years, 4 months ago (2013-08-13 16:23:26 UTC) #2
stevenjb
wifi_data_provider_chromeos.cc lgtm
7 years, 4 months ago (2013-08-13 16:50:05 UTC) #3
bulach
lgtm, thanks! https://codereview.chromium.org/22866005/diff/5001/content/browser/geolocation/device_data_provider.h File content/browser/geolocation/device_data_provider.h (right): https://codereview.chromium.org/22866005/diff/5001/content/browser/geolocation/device_data_provider.h#newcode78 content/browser/geolocation/device_data_provider.h:78: : public base::RefCountedThreadSafe<DeviceDataProviderImplBaseHack> { I suppose this ...
7 years, 4 months ago (2013-08-13 16:59:46 UTC) #4
bulach
still lgtm https://codereview.chromium.org/22866005/diff/5001/content/browser/geolocation/device_data_provider.h File content/browser/geolocation/device_data_provider.h (right): https://codereview.chromium.org/22866005/diff/5001/content/browser/geolocation/device_data_provider.h#newcode78 content/browser/geolocation/device_data_provider.h:78: : public base::RefCountedThreadSafe<DeviceDataProviderImplBaseHack> { On 2013/08/13 16:59:46, ...
7 years, 4 months ago (2013-08-13 17:23:32 UTC) #5
Michael van Ouwerkerk
Thanks Marcus! https://codereview.chromium.org/22866005/diff/5001/content/browser/geolocation/device_data_provider.h File content/browser/geolocation/device_data_provider.h (right): https://codereview.chromium.org/22866005/diff/5001/content/browser/geolocation/device_data_provider.h#newcode98 content/browser/geolocation/device_data_provider.h:98: // receiving notifications. On 2013/08/13 16:59:46, bulach ...
7 years, 4 months ago (2013-08-13 17:44:30 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/22866005/35001
7 years, 4 months ago (2013-08-15 09:43:35 UTC) #7
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=160182
7 years, 4 months ago (2013-08-15 10:48:01 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/22866005/35001
7 years, 4 months ago (2013-08-15 10:49:39 UTC) #9
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=160192
7 years, 4 months ago (2013-08-15 12:11:59 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/22866005/35001
7 years, 4 months ago (2013-08-15 13:04:45 UTC) #11
commit-bot: I haz the power
Change committed as 217793
7 years, 4 months ago (2013-08-15 15:06:36 UTC) #12
joth
7 years, 4 months ago (2013-08-15 19:16:56 UTC) #13 
On 13 August 2013 09:59, <bulach@chromium.org> wrote:

> lgtm, thanks!
>
>
> https://codereview.chromium.**org/22866005/diff/5001/**
>
content/browser/geolocation/**device_data_provider.h<https://codereview.chromium.org/22866005/diff/5001/content/browser/geolocation/device_data_provider.h>
> File content/browser/geolocation/**device_data_provider.h (right):
>
> https://codereview.chromium.**org/22866005/diff/5001/**
>
content/browser/geolocation/**device_data_provider.h#**newcode78<https://codereview.chromium.org/22866005/diff/5001/content/browser/geolocation/device_data_provider.h#newcode78>
> content/browser/geolocation/**device_data_provider.h:78: : public
> base::RefCountedThreadSafe<**DeviceDataProviderImplBaseHack**> {
> I suppose this no longer needs to be ThreadSafe, i.e., just
> RefCounted<**DeviceDataProviderImplBaseHack**> ?
>
> also, chrome now requires msvs2010 :)
>
http://dev.chromium.org/**developers/how-tos/build-**instructions-windows<htt...
>
> perhaps give it a try to remove this? probably best as a separate patch
> anyways..
>
>
I'm late to the conversation, but.... +1 for removing the hack, +1 for
removing the templating altogether. +1 for doing it in a separate patch :)

Thanks!





> https://codereview.chromium.**org/22866005/diff/5001/**
>
content/browser/geolocation/**device_data_provider.h#**newcode98<https://codereview.chromium.org/22866005/diff/5001/content/browser/geolocation/device_data_provider.h#newcode98>
> content/browser/geolocation/**device_data_provider.h:98: // receiving
> notifications.
> nit: to be extra safe, add "after this call".
> since it's all running in the same thread, it's important to state that
> StartDataProvider() won't call back up to
> "listener->**DeviceDataUpdateAvailable()" from within itself..
>
> https://codereview.chromium.**org/22866005/diff/5001/**
>
content/browser/geolocation/**wifi_data_provider_common.cc<https://codereview.chromium.org/22866005/diff/5001/content/browser/geolocation/wifi_data_provider_common.cc>
> File content/browser/geolocation/**wifi_data_provider_common.cc (right):
>
> https://codereview.chromium.**org/22866005/diff/5001/**
>
content/browser/geolocation/**wifi_data_provider_common.cc#**newcode59<https://codereview.chromium.org/22866005/diff/5001/content/browser/geolocation/wifi_data_provider_common.cc#newcode59>
> content/browser/geolocation/**wifi_data_provider_common.cc:**59:
> DCHECK(data);
> nit: no need, a null-ptr dereference below would be as clear as this
> DCHECK.
>
>
https://codereview.chromium.**org/22866005/<https://codereview.chromium.org/2...
>

Powered by Google App Engine
This is Rietveld 408576698