Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2820863002:
Geolocation: cleanup NetworkLocationProvider (Closed)
DescriptionGeolocation: 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}
Committed: https://chromium.googlesource.com/chromium/src/+/1361d4b18439259e34712495ad154cdff1d548a7
Patch Set 1 #
Total comments: 8
Patch Set 2 : dougt@ comments #Patch Set 3 : restoring returning in StartProvider() if (!request_->url().is_valid()), used in components_browsertests #
Total comments: 3
Patch Set 4 : scheib@ comment, method order restored #
Messages
Total messages: 36 (24 generated)
Description was changed from ========== Geolocation: cleanup NetworkLocationProvider BUG=629158 ========== to ========== Geolocation: cleanup NetworkLocationProvider This CL: - makes NetworkLocationProvider use ThreadChecker instead of being a base::NonThreadSafe. - removed unnecessary OnWifiDataUpdated() method - clarifies the logic in RequestPosition(), were some check were redundant, in particular we bail if if (!is_new_data_available_ || !is_wifi_data_complete_) and |weak_factory_|'s pointers are only used to launch the delayed RequestPosition() from StartProvider(). - Method OnLocationResponse() is untouched. Test-wise: network_location_provider_unittest.cc is pretty extensive and it's still passing. BUG=629158 ==========
Description was changed from ========== Geolocation: cleanup NetworkLocationProvider This CL: - makes NetworkLocationProvider use ThreadChecker instead of being a base::NonThreadSafe. - removed unnecessary OnWifiDataUpdated() method - clarifies the logic in RequestPosition(), were some check were redundant, in particular we bail if if (!is_new_data_available_ || !is_wifi_data_complete_) and |weak_factory_|'s pointers are only used to launch the delayed RequestPosition() from StartProvider(). - Method OnLocationResponse() is untouched. Test-wise: network_location_provider_unittest.cc is pretty extensive and it's still passing. BUG=629158 ========== to ========== Geolocation: cleanup NetworkLocationProvider This CL: - makes NetworkLocationProvider use ThreadChecker instead of being a base::NonThreadSafe. - removed unnecessary OnWifiDataUpdated() method - reorders the method implementation to follow declarations. - clarifies the logic in RequestPosition(), were some check were redundant, in particular we bail if if (!is_new_data_available_ || !is_wifi_data_complete_) and |weak_factory_|'s pointers are only used to launch the delayed RequestPosition() from StartProvider(). - Method OnLocationResponse() is untouched. Test-wise: network_location_provider_unittest.cc is pretty extensive and it's still passing. BUG=629158 ==========
Description was changed from ========== Geolocation: cleanup NetworkLocationProvider This CL: - makes NetworkLocationProvider use ThreadChecker instead of being a base::NonThreadSafe. - removed unnecessary OnWifiDataUpdated() method - reorders the method implementation to follow declarations. - clarifies the logic in RequestPosition(), were some check were redundant, in particular we bail if if (!is_new_data_available_ || !is_wifi_data_complete_) and |weak_factory_|'s pointers are only used to launch the delayed RequestPosition() from StartProvider(). - Method OnLocationResponse() is untouched. Test-wise: network_location_provider_unittest.cc is pretty extensive and it's still passing. BUG=629158 ========== to ========== Geolocation: cleanup NetworkLocationProvider This CL: - makes NetworkLocationProvider use ThreadChecker instead of being a base::NonThreadSafe. - removed unnecessary OnWifiDataUpdated() method, the contents being merged into OnWifiDataUpdate() which is called instead of semi-duplicated code in RequestPosition(), clarifying the logic. - reorders the method implementation to follow declarations. - clarifies the logic in RequestPosition(), were some check were redundant, in particular we bail if if (!is_new_data_available_ || !is_wifi_data_complete_) and |weak_factory_|'s pointers are only used to launch the delayed RequestPosition() from StartProvider(). - Method OnLocationResponse() is untouched. Test-wise: network_location_provider_unittest.cc is pretty extensive and it's still passing. BUG=629158 ==========
The CQ bit was checked by mcasas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Geolocation: cleanup NetworkLocationProvider This CL: - makes NetworkLocationProvider use ThreadChecker instead of being a base::NonThreadSafe. - removed unnecessary OnWifiDataUpdated() method, the contents being merged into OnWifiDataUpdate() which is called instead of semi-duplicated code in RequestPosition(), clarifying the logic. - reorders the method implementation to follow declarations. - clarifies the logic in RequestPosition(), were some check were redundant, in particular we bail if if (!is_new_data_available_ || !is_wifi_data_complete_) and |weak_factory_|'s pointers are only used to launch the delayed RequestPosition() from StartProvider(). - Method OnLocationResponse() is untouched. Test-wise: network_location_provider_unittest.cc is pretty extensive and it's still passing. BUG=629158 ========== to ========== 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. - reorders the method implementation to follow declarations. - clarifies the logic in RequestPosition(), were some check were redundant, in particular we bail if if (!is_new_data_available_ || !is_wifi_data_complete_) and |weak_factory_|'s pointers are only used to launch the delayed RequestPosition() from StartProvider(). - note: Method OnLocationResponse() is untouched. Test-wise: network_location_provider_unittest.cc is pretty extensive and it's still passing. BUG=629158 ==========
mcasas@chromium.org changed reviewers: + dougt@chromium.org
dougt@ PTAL
ooc, why not add thread_checker DCHECKs to all public methods? https://codereview.chromium.org/2820863002/diff/1/device/geolocation/network_... File device/geolocation/network_location_provider.cc (right): https://codereview.chromium.org/2820863002/diff/1/device/geolocation/network_... device/geolocation/network_location_provider.cc:146: if (!request_->url().is_valid()) // Already DLOG() in constructor. Have you considered just dropping this check? request_ isn't going to change and you've already DLOG(). Maybe just let the provider start at this point? https://codereview.chromium.org/2820863002/diff/1/device/geolocation/network_... device/geolocation/network_location_provider.cc:219: DVLOG(1) << "NetworkLocationProvider - pre-empting pending network request " is this DVLOG() needed anymore? Maybe just drop it. https://codereview.chromium.org/2820863002/diff/1/device/geolocation/network_... device/geolocation/network_location_provider.cc:250: position_ = position; I am confused why this code remembers the position -- even when it's not valid? https://codereview.chromium.org/2820863002/diff/1/device/geolocation/network_... File device/geolocation/network_location_provider.h (right): https://codereview.chromium.org/2820863002/diff/1/device/geolocation/network_... device/geolocation/network_location_provider.h:97: // StartProvider() and StopProvider() and checked via IsStarted(). // StartProvider() and StopProvider() and checked via IsStarted(). ^ You want a comma there.
Patchset #2 (id:20001) has been deleted
ptal https://codereview.chromium.org/2820863002/diff/1/device/geolocation/network_... File device/geolocation/network_location_provider.cc (right): https://codereview.chromium.org/2820863002/diff/1/device/geolocation/network_... device/geolocation/network_location_provider.cc:146: if (!request_->url().is_valid()) // Already DLOG() in constructor. On 2017/04/17 18:58:52, dougt wrote: > Have you considered just dropping this check? request_ isn't going to change > and you've already DLOG(). Maybe just let the provider start at this point? Changed to a DCHECK to document it as being a precondition of this method. https://codereview.chromium.org/2820863002/diff/1/device/geolocation/network_... device/geolocation/network_location_provider.cc:219: DVLOG(1) << "NetworkLocationProvider - pre-empting pending network request " On 2017/04/17 18:58:52, dougt wrote: > is this DVLOG() needed anymore? Maybe just drop it. Hmm well, I didn't want to remove it entirely because of the associated comment in l.215. Will simplify. https://codereview.chromium.org/2820863002/diff/1/device/geolocation/network_... device/geolocation/network_location_provider.cc:250: position_ = position; On 2017/04/17 18:58:52, dougt wrote: > I am confused why this code remembers the position -- even when it's not valid? Bug perhaps? I haven't changed this method's logic. https://codereview.chromium.org/2820863002/diff/1/device/geolocation/network_... File device/geolocation/network_location_provider.h (right): https://codereview.chromium.org/2820863002/diff/1/device/geolocation/network_... device/geolocation/network_location_provider.h:97: // StartProvider() and StopProvider() and checked via IsStarted(). On 2017/04/17 18:58:53, dougt wrote: > // StartProvider() and StopProvider() and checked via IsStarted(). > ^ > > You want a comma there. Done.
lgtm
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougt@chromium.org Link to the patchset: https://codereview.chromium.org/2820863002/#ps60001 (title: "restoring returning in StartProvider() if (!request_->url().is_valid()), used in components_browsertests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by mcasas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mcasas@chromium.org changed reviewers: + scheib@chromium.org
scheib@ ptal/ rs plz
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
In the future, please keep code moves (definitions to match declarations order) in stand alone patches without logical changes. It's not just the reviewer burden, but also any future code blame/change review that will be harder. nit on the DLOG_IF and question about the weak ptrs: https://codereview.chromium.org/2820863002/diff/60001/device/geolocation/netw... File device/geolocation/network_location_provider.cc (right): https://codereview.chromium.org/2820863002/diff/60001/device/geolocation/netw... device/geolocation/network_location_provider.cc:127: DLOG_IF(WARNING, url.is_valid()) DLOG if the url is valid? That doesn't seem to match the warning text of it being a bad URL. You intended !url.is_valid()? https://codereview.chromium.org/2820863002/diff/60001/device/geolocation/netw... device/geolocation/network_location_provider.cc:214: is_new_data_available_ = false; Describe why we don't need to invalidate weak ptrs in Change description.
Description was changed from ========== 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. - reorders the method implementation to follow declarations. - clarifies the logic in RequestPosition(), were some check were redundant, in particular we bail if if (!is_new_data_available_ || !is_wifi_data_complete_) and |weak_factory_|'s pointers are only used to launch the delayed RequestPosition() from StartProvider(). - note: Method OnLocationResponse() is untouched. Test-wise: network_location_provider_unittest.cc is pretty extensive and it's still passing. BUG=629158 ========== to ========== 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 check were redundant, in particular we bail if if (!is_new_data_available_ || !is_wifi_data_complete_) and |weak_factory_|'s pointers are only used to launch the delayed RequestPosition() from StartProvider(). - note: Method OnLocationResponse() is untouched. Test-wise: network_location_provider_unittest.cc is pretty extensive and it's still passing. BUG=629158 ==========
Description was changed from ========== 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 check were redundant, in particular we bail if if (!is_new_data_available_ || !is_wifi_data_complete_) and |weak_factory_|'s pointers are only used to launch the delayed RequestPosition() from StartProvider(). - note: Method OnLocationResponse() is untouched. Test-wise: network_location_provider_unittest.cc is pretty extensive and it's still passing. BUG=629158 ========== to ========== 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 ==========
Also rewritten CL description. https://codereview.chromium.org/2820863002/diff/60001/device/geolocation/netw... File device/geolocation/network_location_provider.cc (right): https://codereview.chromium.org/2820863002/diff/60001/device/geolocation/netw... device/geolocation/network_location_provider.cc:127: DLOG_IF(WARNING, url.is_valid()) On 2017/04/18 03:33:17, scheib wrote: > DLOG if the url is valid? That doesn't seem to match the warning text of it > being a bad URL. You intended !url.is_valid()? Done.
Thanks. LGTM
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougt@chromium.org Link to the patchset: https://codereview.chromium.org/2820863002/#ps80001 (title: "scheib@ comment, method order restored")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492534117002430,
"parent_rev": "8e75e9c587155933fe3e775605e18111a6cc2b9c", "commit_rev":
"1361d4b18439259e34712495ad154cdff1d548a7"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/1361d4b18439259e34712495ad15... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1361d4b18439259e34712495ad15... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
