Consider not using weak_ptr_factory_.HasWeakPtrs() in NetworkLocationProvider::RequestRefresh() |
|||||
Issue descriptionNetworkLocationProvider::RequestRefresh() [1] tests weak_ptr_factory_.HasWeakPtrs(), probably to see if the code execution is between StartProvider() and StopProvider() [2], or if a RequestPosition() has been called [3]. This is an odd use of a WeakPtrFactory, consider using other methods (e.g. a locked enum State) that better convey intention. [1] https://cs.chromium.org/chromium/src/content/browser/geolocation/network_location_provider.cc?q=NetworkLocationProvider::RequestRefresh&sq=package:chromium&dr=CSs&l=144 [2] https://cs.chromium.org/chromium/src/content/browser/geolocation/network_location_provider.cc?q=NetworkLocationProvider::RequestRefresh&sq=package:chromium&dr=CSs&l=227 [3] https://cs.chromium.org/chromium/src/content/browser/geolocation/network_location_provider.cc?q=NetworkLocationProvider::RequestRefresh&sq=package:chromium&dr=CSs&l=257
,
Apr 12 2017
,
Apr 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1361d4b18439259e34712495ad154cdff1d548a7 commit 1361d4b18439259e34712495ad154cdff1d548a7 Author: mcasas <mcasas@chromium.org> Date: Tue Apr 18 17:35:14 2017 Geolocation: cleanup NetworkLocationProvider This CL: - makes NetworkLocationProvider use ThreadChecker instead of being a base::NonThreadSafe. - removes unnecessary OnWifiDataUpdated() method, the contents being merged into OnWifiDataUpdate() which is called instead of semi-duplicated code in RequestPosition(), clarifying the logic. - clarifies the logic in RequestPosition(), were some checks were redundant, in particular: we don't need to test nor invalidate the weak pointers. Rationale: RequestPosition() can be called from OnPermissionGranted(), the new OnWifiDataUpdate() (ToT's OnWifiDataUpdated()) and from the PostDelayedTask in StartUpdate(); |weak_factory_|'s HasWeakPointers() was used to differentiate those call sites; but this can be done via the |is_...| variables: if (!is_new_data_available_ || !is_wifi_data_complete_) and |weak_factory_|'s pointers are only used to launch the delayed RequestPosition() from StartProvider(). Test-wise: network_location_provider_unittest.cc is pretty extensive and it's still passing. BUG= 629158 Review-Url: https://codereview.chromium.org/2820863002 Cr-Commit-Position: refs/heads/master@{#465280} [modify] https://crrev.com/1361d4b18439259e34712495ad154cdff1d548a7/device/geolocation/network_location_provider.cc [modify] https://crrev.com/1361d4b18439259e34712495ad154cdff1d548a7/device/geolocation/network_location_provider.h
,
Apr 18 2017
,
Sep 22 2017
,
Sep 22 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by mcasas@chromium.org
, Apr 12 2017