Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2226143002:
Gets rid of the LocationArbitrator interface, in preference for LocationProvider. (Closed)
DescriptionGets rid of the LocationArbitrator interface, in preference for LocationProvider.
This CL reorganizes classes that derived from LocationArbitrator to instead
derive from LocationProvider. There was a quite a bit of overlap between the
two interfaces, and merging the two in favor of LocationProvider makes later
work with Blimp much easier.
Other work included:
*Addresses TODO to remove CreateArbitrator in favor of SetArbitratorForTest
BUG=614486, 637869
Committed: https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9
Cr-Commit-Position: refs/heads/master@{#414735}
Patch Set 1 : LocationArbitratorImpl becomes a LocationProvider #Patch Set 2 : Removes LocationArbitrator.h #
Total comments: 32
Patch Set 3 : TODO SetArbitratorForTest In Progress #Patch Set 4 : Addreses Wez's #4 comments #Patch Set 5 : Revert "Merge branch 'client_feature' into lai #Patch Set 6 : Addresses kmarshall's #5 comments. #Patch Set 7 : Fix up Build file #Patch Set 8 : Updates LocationProviderBase derived classes. #
Total comments: 18
Patch Set 9 : Addresses Wez's #22 comments #Patch Set 10 : Merge branch 'master' into lai #Patch Set 11 : Updated location_provider_android.h #
Total comments: 17
Patch Set 12 : Fixes try bot issue caused by AtExitManager. Now ShadowingAtExitManager. #Patch Set 13 : Addresses kmarshall's #41 comments #
Total comments: 14
Patch Set 14 : Addresses kmarshall's #47 comments #Patch Set 15 : Update GetPosition. #Patch Set 16 : Export LocationProvider #Patch Set 17 : Merge branch 'master' into lai #Patch Set 18 : Merge branch 'master' into lai #
Total comments: 21
Patch Set 19 : In response to Wez's #73-75 comments. #
Total comments: 6
Patch Set 20 : Addresses Wez's #80 comments. #
Total comments: 4
Patch Set 21 : In response to Wez's #82 comments. #Patch Set 22 : Merge branch 'master' into lai #Patch Set 23 : Merge branch 'master' into lai #Patch Set 24 : Merge branch 'master' into lai #Depends on Patchset: Messages
Total messages: 122 (78 generated)
lethalantidote@chromium.org changed reviewers: + kmarshall@chromium.org, mvanouwerkerk@chromium.org, wez@chromium.org
Description was changed from ========== Gets rid of the LocationArbitrator interface, in preference for LocationProvider. This CL reorganizes classes that derived from LocationArbitrator to instead derive from LocationProvider. There was a quite a bit of overlap between the two interfaces, and merging the two in favor of LocationProvider makes later work with Blimp much easier. BUG=614486 ========== to ========== Gets rid of the LocationArbitrator interface, in preference for LocationProvider. This CL reorganizes classes that derived from LocationArbitrator to instead derive from LocationProvider. There was a quite a bit of overlap between the two interfaces, and merging the two in favor of LocationProvider makes later work with Blimp much easier. Unittests forthcoming. BUG=614486 ==========
Ahhh, loving the cleanup :) https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/geol... File device/geolocation/geolocation_provider_impl.h (right): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/geol... device/geolocation/geolocation_provider_impl.h:60: // TODO(mvanouwerkerk): Use something like SetArbitratorForTesting instead. nit: Consider implementing this TODO() while you're here. :) e.g. Create the LocationArbitrator in the Init() and allow it to be replaced with a MockLocationProvider via the API as he suggests. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... File device/geolocation/location_arbitrator.h (left): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator.h:3: // found in the LICENSE file. Not Actionable: \o/ https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... File device/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator_impl.cc:75: DoStartProviders(); Looks like it might be worthwhile to have DoStartProviders return a bool indicating whether providers_ is empty, and to return that here. (Reason being that if there is no system provider and the caller asks for no network providers then providers_ is empty, so we should probably fail :) https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator_impl.cc:149: provider->RequestRefresh(); nit: Interesting that there are no other RequestRefresh() mentions in LocationArbitratorImpl - are we sure that anything actually needs it? Perhaps we could just remove it from the LocationProvider API..? https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator_impl.cc:154: DCHECK(position); nit: No need to DCHECK - you're about to assign to |position|, things will explode cleanly enough then if it's null. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator_impl.cc:155: position = &position_; This isn't doing what you intend. You're setting the function-local variable |position| to point to |position_| - when GetPosition() returns there will be no observable side-effect. What you _want_ to do is copy/assign the value of |position_| into the memory that |position| points to, I suspect. :P https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... File device/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator_impl.h:9: #include <memory> What's this extra include needed for? https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator_impl.h:53: // LocationProvider nit: "LocationProvider implementation." https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator_impl.h:62: bool HasPermissionBeenGranted() const; Is this only used in tests, as discussed? If so then rename HasPermissionBeenGrantedForTest() so that only test code can call it. :) https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/mock... File device/geolocation/mock_location_arbitrator.h (right): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/mock... device/geolocation/mock_location_arbitrator.h:16: class MockLocationArbitrator : public LocationProvider { It looks like MockLocationArbitrator is only used in the GeolocationProviderImpl unit-tests, and the only bit specific to this mock that is used is the providers_started() getter, which as far as I can tell just checks whether StartProvider() was called, so can be implemented as an EXPECT_CALL, or alternatively would probably make a reasonable getter to add to MockLocationProvider. Either way, I think you can just remove this mock.
https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/geol... File device/geolocation/geolocation_provider_impl.h (right): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/geol... device/geolocation/geolocation_provider_impl.h:28: class LocationArbitrator; Not used? https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/geol... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/geol... device/geolocation/geolocation_provider_impl_unittest.cc:20: #include "device/geolocation/mock_location_arbitrator.h" Does this build? MockLocationArbitrator subclasses LocationArbitrator, which has been removed? This include should probably be removed. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/geol... device/geolocation/geolocation_provider_impl_unittest.cc:29: namespace device { Use an anonymous namespace under device? https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/mock... File device/geolocation/mock_location_arbitrator.h (right): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/mock... device/geolocation/mock_location_arbitrator.h:16: class MockLocationArbitrator : public LocationProvider { Agreed about the removal, but a FYI point - mocks should be renamed if the object that they mock is renamed. So this would become MockLocationProvider. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/mock... device/geolocation/mock_location_arbitrator.h:31: bool HasPermissionBeenGranted() const; No point in declaring new methods in mocks that aren't virtual overrides unless the test code is going to call them - no vtable entries => no dynamic dispatch!
https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/geol... File device/geolocation/geolocation_provider_impl.h (right): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/geol... device/geolocation/geolocation_provider_impl.h:60: // TODO(mvanouwerkerk): Use something like SetArbitratorForTesting instead. On 2016/08/09 01:17:04, Wez wrote: > nit: Consider implementing this TODO() while you're here. :) > > e.g. Create the LocationArbitrator in the Init() and allow it to be replaced > with a MockLocationProvider via the API as he suggests. Done. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... File device/geolocation/location_arbitrator.h (left): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator.h:3: // found in the LICENSE file. On 2016/08/09 01:17:04, Wez wrote: > Not Actionable: > > \o/ Acknowledged. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... File device/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator_impl.cc:75: DoStartProviders(); On 2016/08/09 01:17:04, Wez wrote: > Looks like it might be worthwhile to have DoStartProviders return a bool > indicating whether providers_ is empty, and to return that here. > > (Reason being that if there is no system provider and the caller asks for no > network providers then providers_ is empty, so we should probably fail :) Done. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator_impl.cc:149: provider->RequestRefresh(); On 2016/08/09 01:17:04, Wez wrote: > nit: Interesting that there are no other RequestRefresh() mentions in > LocationArbitratorImpl - are we sure that anything actually needs it? Perhaps we > could just remove it from the LocationProvider API..? Done. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator_impl.cc:154: DCHECK(position); On 2016/08/09 01:17:04, Wez wrote: > nit: No need to DCHECK - you're about to assign to |position|, things will > explode cleanly enough then if it's null. Done. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator_impl.cc:155: position = &position_; On 2016/08/09 01:17:04, Wez wrote: > This isn't doing what you intend. > > You're setting the function-local variable |position| to point to |position_| - > when GetPosition() returns there will be no observable side-effect. > > What you _want_ to do is copy/assign the value of |position_| into the memory > that |position| points to, I suspect. :P Done. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... File device/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator_impl.h:9: #include <memory> On 2016/08/09 01:17:04, Wez wrote: > What's this extra include needed for? Lint complains without it. It's because we are using unique_ptrs. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator_impl.h:53: // LocationProvider On 2016/08/09 01:17:04, Wez wrote: > nit: "LocationProvider implementation." Done. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator_impl.h:62: bool HasPermissionBeenGranted() const; On 2016/08/09 01:17:04, Wez wrote: > Is this only used in tests, as discussed? If so then rename > HasPermissionBeenGrantedForTest() so that only test code can call it. :) Done. Question, does adding "ForTest" to the end have an actual side effect, or is just a guideline? https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/mock... File device/geolocation/mock_location_arbitrator.h (right): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/mock... device/geolocation/mock_location_arbitrator.h:16: class MockLocationArbitrator : public LocationProvider { On 2016/08/09 01:17:04, Wez wrote: > It looks like MockLocationArbitrator is only used in the GeolocationProviderImpl > unit-tests, and the only bit specific to this mock that is used is the > providers_started() getter, which as far as I can tell just checks whether > StartProvider() was called, so can be implemented as an EXPECT_CALL, or > alternatively would probably make a reasonable getter to add to > MockLocationProvider. > > Either way, I think you can just remove this mock. Done. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/mock... device/geolocation/mock_location_arbitrator.h:16: class MockLocationArbitrator : public LocationProvider { On 2016/08/10 23:16:19, Kevin M wrote: > Agreed about the removal, but a FYI point - mocks should be renamed if the > object that they mock is renamed. So this would become MockLocationProvider. Acknowledged. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/mock... device/geolocation/mock_location_arbitrator.h:31: bool HasPermissionBeenGranted() const; On 2016/08/10 23:16:19, Kevin M wrote: > No point in declaring new methods in mocks that aren't virtual overrides unless > the test code is going to call them - no vtable entries => no dynamic dispatch! Acknowledged. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/mock... device/geolocation/mock_location_arbitrator.h:31: bool HasPermissionBeenGranted() const; On 2016/08/10 23:16:19, Kevin M wrote: > No point in declaring new methods in mocks that aren't virtual overrides unless > the test code is going to call them - no vtable entries => no dynamic dispatch! Acknowledged.
https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/geol... File device/geolocation/geolocation_provider_impl.h (right): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/geol... device/geolocation/geolocation_provider_impl.h:28: class LocationArbitrator; On 2016/08/10 23:16:19, Kevin M wrote: > Not used? Done. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/geol... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/geol... device/geolocation/geolocation_provider_impl_unittest.cc:20: #include "device/geolocation/mock_location_arbitrator.h" On 2016/08/10 23:16:19, Kevin M wrote: > Does this build? MockLocationArbitrator subclasses LocationArbitrator, which has > been removed? > > This include should probably be removed. Done. https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/geol... device/geolocation/geolocation_provider_impl_unittest.cc:29: namespace device { On 2016/08/10 23:16:19, Kevin M wrote: > Use an anonymous namespace under device? Done.
Description was changed from ========== Gets rid of the LocationArbitrator interface, in preference for LocationProvider. This CL reorganizes classes that derived from LocationArbitrator to instead derive from LocationProvider. There was a quite a bit of overlap between the two interfaces, and merging the two in favor of LocationProvider makes later work with Blimp much easier. Unittests forthcoming. BUG=614486 ========== to ========== Gets rid of the LocationArbitrator interface, in preference for LocationProvider. This CL reorganizes classes that derived from LocationArbitrator to instead derive from LocationProvider. There was a quite a bit of overlap between the two interfaces, and merging the two in favor of LocationProvider makes later work with Blimp much easier. Unittests forthcoming. Other work included: *Addresses TODO to remove CreateArbitrator in favor of SetArbitratorForTest BUG=614486 ==========
Description was changed from ========== Gets rid of the LocationArbitrator interface, in preference for LocationProvider. This CL reorganizes classes that derived from LocationArbitrator to instead derive from LocationProvider. There was a quite a bit of overlap between the two interfaces, and merging the two in favor of LocationProvider makes later work with Blimp much easier. Unittests forthcoming. Other work included: *Addresses TODO to remove CreateArbitrator in favor of SetArbitratorForTest BUG=614486 ========== to ========== Gets rid of the LocationArbitrator interface, in preference for LocationProvider. This CL reorganizes classes that derived from LocationArbitrator to instead derive from LocationProvider. There was a quite a bit of overlap between the two interfaces, and merging the two in favor of LocationProvider makes later work with Blimp much easier. Other work included: *Addresses TODO to remove CreateArbitrator in favor of SetArbitratorForTest BUG=614486 ==========
The CQ bit was checked by lethalantidote@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lethalantidote@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: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lethalantidote@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... File device/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2226143002/diff/20001/device/geolocation/loca... device/geolocation/location_arbitrator_impl.h:62: bool HasPermissionBeenGranted() const; On 2016/08/11 22:06:43, CJ wrote: > On 2016/08/09 01:17:04, Wez wrote: > > Is this only used in tests, as discussed? If so then rename > > HasPermissionBeenGrantedForTest() so that only test code can call it. :) > > Done. Question, does adding "ForTest" to the end have an actual side effect, or > is just a guideline? IIRC there is a Clang plugin that will complain if you add code that calls ForTest, but which is not in a _test.cc file, but yes, even without that, it's good to be clear that this is not intended for production use. :) https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl.cc (right): https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... device/geolocation/geolocation_provider_impl.cc:191: new LocationArbitratorImpl(callback, g_delegate.Get().get())); nit: You can use MakeUnique here. :) https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl.h (right): https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... device/geolocation/geolocation_provider_impl.h:53: // Useful for injecting mock geolocation arbitrator in tests. nit: This comment is not super helpful - caller can work that out from the name :P What's helpful to know is that it _is_ safe to call from the "caller" thread, but that it must be called while there are no GeolocationProviderImpl clients registered, I think. https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... device/geolocation/geolocation_provider_impl.h:54: void SetArbitratorForTesting(LocationProvider* arbitrator); Use unique_ptr<> here, so that it's clear that the caller is giving up ownership of the supplied arbitrator. https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... device/geolocation/geolocation_provider_impl.h:56: protected: You can merge this with the private section, below, now that nothing needs to derive from it. https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... device/geolocation/geolocation_provider_impl.h:62: std::unique_ptr<LocationProvider> arbitrator_; Move this down to sit alongisde the other data members when you merge this with the private section. https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:121: // correctly reinitializing GeolocationProviderImpl (Singleton) It needs to be before all other variables so that it is available for Singletons, etc to register with, to be torn-down when the test completes. https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:123: // on the correct thread. nit: Move this comment down to the message_loop_ member https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/loc... File device/geolocation/location_provider_base.h (right): https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/loc... device/geolocation/location_provider_base.h:25: virtual void RequestRefresh(); Is there any point in keeping this at all? Since it's no longer part of LocationProvider, just a protected method here, nothing external can call it. The only LocationProvider that does actually call it is the NetworkLocationProvider - it's doing that internally, so there's no need for it to be part of this base class - it can be a non-virtual in NetworkLocationProvider itself. https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/net... File device/geolocation/network_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/net... device/geolocation/network_location_provider.h:74: void RequestRefresh() override; There doesn't seem to be any need for this to be part of LocationProviderBase - let's make it purely an internal detail of this provider?
https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl.cc (right): https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... device/geolocation/geolocation_provider_impl.cc:191: new LocationArbitratorImpl(callback, g_delegate.Get().get())); On 2016/08/12 00:33:44, Wez wrote: > nit: You can use MakeUnique here. :) Done. https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl.h (right): https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... device/geolocation/geolocation_provider_impl.h:53: // Useful for injecting mock geolocation arbitrator in tests. On 2016/08/12 00:33:44, Wez wrote: > nit: This comment is not super helpful - caller can work that out from the name > :P > > What's helpful to know is that it _is_ safe to call from the "caller" thread, > but that it must be called while there are no GeolocationProviderImpl clients > registered, I think. Done. https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... device/geolocation/geolocation_provider_impl.h:54: void SetArbitratorForTesting(LocationProvider* arbitrator); On 2016/08/12 00:33:44, Wez wrote: > Use unique_ptr<> here, so that it's clear that the caller is giving up ownership > of the supplied arbitrator. Done. https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... device/geolocation/geolocation_provider_impl.h:56: protected: On 2016/08/12 00:33:44, Wez wrote: > You can merge this with the private section, below, now that nothing needs to > derive from it. Done. https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... device/geolocation/geolocation_provider_impl.h:62: std::unique_ptr<LocationProvider> arbitrator_; On 2016/08/12 00:33:44, Wez wrote: > Move this down to sit alongisde the other data members when you merge this with > the private section. Done. https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:121: // correctly reinitializing GeolocationProviderImpl (Singleton) On 2016/08/12 00:33:44, Wez wrote: > It needs to be before all other variables so that it is available for > Singletons, etc to register with, to be torn-down when the test completes. Done. https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:123: // on the correct thread. On 2016/08/12 00:33:44, Wez wrote: > nit: Move this comment down to the message_loop_ member Done. https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/loc... File device/geolocation/location_provider_base.h (right): https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/loc... device/geolocation/location_provider_base.h:25: virtual void RequestRefresh(); On 2016/08/12 00:33:44, Wez wrote: > Is there any point in keeping this at all? > > Since it's no longer part of LocationProvider, just a protected method here, > nothing external can call it. > > The only LocationProvider that does actually call it is the > NetworkLocationProvider - it's doing that internally, so there's no need for it > to be part of this base class - it can be a non-virtual in > NetworkLocationProvider itself. Done. https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/net... File device/geolocation/network_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/140001/device/geolocation/net... device/geolocation/network_location_provider.h:74: void RequestRefresh() override; On 2016/08/12 00:33:44, Wez wrote: > There doesn't seem to be any need for this to be part of LocationProviderBase - > let's make it purely an internal detail of this provider? Done.
The CQ bit was checked by lethalantidote@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: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by lethalantidote@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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lethalantidote@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Gets rid of the LocationArbitrator interface, in preference for LocationProvider. This CL reorganizes classes that derived from LocationArbitrator to instead derive from LocationProvider. There was a quite a bit of overlap between the two interfaces, and merging the two in favor of LocationProvider makes later work with Blimp much easier. Other work included: *Addresses TODO to remove CreateArbitrator in favor of SetArbitratorForTest BUG=614486 ========== to ========== Gets rid of the LocationArbitrator interface, in preference for LocationProvider. This CL reorganizes classes that derived from LocationArbitrator to instead derive from LocationProvider. There was a quite a bit of overlap between the two interfaces, and merging the two in favor of LocationProvider makes later work with Blimp much easier. Other work included: *Addresses TODO to remove CreateArbitrator in favor of SetArbitratorForTest BUG=614486, 637869 ==========
The CQ bit was checked by lethalantidote@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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl.h (right): https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/geo... device/geolocation/geolocation_provider_impl.h:53: // Safe to call from the caller thread while there are no Safe to call from any thread? https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:125: // |message_loop_| must come next to include all other variable construction Relative statements like "come next to" is confusing... better to be exact, e.g. "must be declared after |at_exit_|" https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:130: MockLocationProvider* arbitrator_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:88: provider->StartProvider(enable_high_accuracy_); This is throwing away the return value of StartProvider... intentional? https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:146: void LocationArbitratorImpl::GetPosition(Geoposition* position) { Change this to return a const Geoposition& and just return *position_ directly. This is more syntactically pleasant to read, plus it gives the caller the opportunity to avoid making a copy by just accessing fields from the reference. https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:151: const LocationProviderUpdateCallback& callback) { DCHECK(!callback.is_null()) https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.h:61: bool HasPermissionBeenGrantedForTest() const; Move above "LocationProvider implementation" https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/loc... File device/geolocation/location_provider_android.h (right): https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/loc... device/geolocation/location_provider_android.h:25: // LocationProvider. ...implementation. https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/moc... File device/geolocation/mock_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/moc... device/geolocation/mock_location_provider.h:26: bool providers_started() const { return state_ != STOPPED; } This style of inline getter is only really allowed for just getting the value of a field, not for computed results. This should be something like IsProviderStarted(). nit: This is a LocationProvider (singular), but this getter is plural.
https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl.h (right): https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/geo... device/geolocation/geolocation_provider_impl.h:53: // Safe to call from the caller thread while there are no On 2016/08/15 19:12:46, Kevin M wrote: > Safe to call from any thread? Done. https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:125: // |message_loop_| must come next to include all other variable construction On 2016/08/15 19:12:46, Kevin M wrote: > Relative statements like "come next to" is confusing... better to be exact, e.g. > "must be declared after |at_exit_|" Done. https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:130: MockLocationProvider* arbitrator_; On 2016/08/15 19:12:46, Kevin M wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:88: provider->StartProvider(enable_high_accuracy_); On 2016/08/15 19:12:46, Kevin M wrote: > This is throwing away the return value of StartProvider... intentional? Done. https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:146: void LocationArbitratorImpl::GetPosition(Geoposition* position) { On 2016/08/15 19:12:46, Kevin M wrote: > Change this to return a const Geoposition& and just return *position_ directly. > This is more syntactically pleasant to read, plus it gives the caller the > opportunity to avoid making a copy by just accessing fields from the reference. Yeah I was wondering if I should refactor this. I wasn't sure if there was a good reason to do it the way it is now. https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:151: const LocationProviderUpdateCallback& callback) { On 2016/08/15 19:12:46, Kevin M wrote: > DCHECK(!callback.is_null()) Done. https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.h:61: bool HasPermissionBeenGrantedForTest() const; On 2016/08/15 19:12:46, Kevin M wrote: > Move above "LocationProvider implementation" Done. https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/moc... File device/geolocation/mock_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/200001/device/geolocation/moc... device/geolocation/mock_location_provider.h:26: bool providers_started() const { return state_ != STOPPED; } On 2016/08/15 19:12:46, Kevin M wrote: > This style of inline getter is only really allowed for just getting the value of > a field, not for computed results. This should be something like > IsProviderStarted(). > > > nit: This is a LocationProvider (singular), but this getter is plural. > > Done.
The CQ bit was checked by lethalantidote@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: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:125: // All other variables must be delcared after |at_exit_| to ensure they typo: declared Also, this comment is already spelled out by the one for at_exit_. https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:89: started = true && provider->StartProvider(enable_high_accuracy_); "true &&" is a no-op ;) I think you mean "started &&" ? Also, do *all* providers need to be started in order for this to work? It seems to me that a single provider should suffice. https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/loc... device/geolocation/location_arbitrator_impl_unittest.cc:118: cell_ = new MockLocationProvider; StrictMock here and elsewhere in this test that mock objects are created. It's vital that we pay attention to how the arbitrator interacts with these objects. It doesn't look like the existing tests cover this aspect, so we'll need to backfill a few tests here.
https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:125: // All other variables must be delcared after |at_exit_| to ensure they On 2016/08/15 21:34:54, Kevin M wrote: > typo: declared > > Also, this comment is already spelled out by the one for at_exit_. Done. https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:89: started = true && provider->StartProvider(enable_high_accuracy_); On 2016/08/15 21:34:54, Kevin M wrote: > "true &&" is a no-op ;) I think you mean "started &&" ? > > Also, do *all* providers need to be started in order for this to work? It seems > to me that a single provider should suffice. Done. https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/loc... device/geolocation/location_arbitrator_impl_unittest.cc:118: cell_ = new MockLocationProvider; On 2016/08/15 21:34:54, Kevin M wrote: > StrictMock here and elsewhere in this test that mock objects are created. It's > vital that we pay attention to how the arbitrator interacts with these objects. > It doesn't look like the existing tests cover this aspect, so we'll need to > backfill a few tests here. To be noted, these are not actually mock objects, but rather fakes. Should I go ahead and just enclose these with StrictMock or wait until an actual refactor? Does StrictMock even do anything when the class it gets passed is not Mock?
The CQ bit was checked by lethalantidote@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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lethalantidote@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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by lethalantidote@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: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by lethalantidote@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: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lethalantidote@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.
lgtm Since this touches browser code outside blimp/, you'll probably want to verify that geolocation still works on a real Chromium build. https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/loc... device/geolocation/location_arbitrator_impl_unittest.cc:118: cell_ = new MockLocationProvider; On 2016/08/15 23:09:52, CJ wrote: > On 2016/08/15 21:34:54, Kevin M wrote: > > StrictMock here and elsewhere in this test that mock objects are created. It's > > vital that we pay attention to how the arbitrator interacts with these > objects. > > It doesn't look like the existing tests cover this aspect, so we'll need to > > backfill a few tests here. > > To be noted, these are not actually mock objects, but rather fakes. Should I go > ahead and just enclose these with StrictMock or wait until an actual refactor? > Does StrictMock even do anything when the class it gets passed is not Mock? Ooh, right. No, StrictMock doesn't do anything for non-mocked classes. I commented on this in another CL. This fake is lightweight enough that it should be a mock. I won't hold up this CL for that cleanup, though. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:89: started = provider->StartProvider(enable_high_accuracy_) || started; super nit: put "started || before provider->StartProvider" to make the logical OR absolutely clear to readers, as a hedge against coders' tendency to skim and gloss over details. ;)
PING - mvanouwerkerk https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/loc... device/geolocation/location_arbitrator_impl_unittest.cc:118: cell_ = new MockLocationProvider; On 2016/08/17 17:44:37, Kevin M wrote: > On 2016/08/15 23:09:52, CJ wrote: > > On 2016/08/15 21:34:54, Kevin M wrote: > > > StrictMock here and elsewhere in this test that mock objects are created. > It's > > > vital that we pay attention to how the arbitrator interacts with these > > objects. > > > It doesn't look like the existing tests cover this aspect, so we'll need to > > > backfill a few tests here. > > > > To be noted, these are not actually mock objects, but rather fakes. Should I > go > > ahead and just enclose these with StrictMock or wait until an actual refactor? > > Does StrictMock even do anything when the class it gets passed is not Mock? > > Ooh, right. No, StrictMock doesn't do anything for non-mocked classes. > > I commented on this in another CL. This fake is lightweight enough that it > should be a mock. I won't hold up this CL for that cleanup, though. Acknowledged. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:89: started = provider->StartProvider(enable_high_accuracy_) || started; On 2016/08/17 17:44:37, Kevin M wrote: > super nit: put "started || before provider->StartProvider" to make the logical > OR absolutely clear to readers, as a hedge against coders' tendency to skim and > gloss over details. ;) Because of short-circuiting, once the first StartProvider returns true and flips started to true, we no longer will call StartProvider, thus to ensure StartProvider gets called, it must be called first.
On 2016/08/17 23:07:11, CJ wrote: > PING - mvanouwerkerk > > https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/loc... > File device/geolocation/location_arbitrator_impl_unittest.cc (right): > > https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/loc... > device/geolocation/location_arbitrator_impl_unittest.cc:118: cell_ = new > MockLocationProvider; > On 2016/08/17 17:44:37, Kevin M wrote: > > On 2016/08/15 23:09:52, CJ wrote: > > > On 2016/08/15 21:34:54, Kevin M wrote: > > > > StrictMock here and elsewhere in this test that mock objects are created. > > It's > > > > vital that we pay attention to how the arbitrator interacts with these > > > objects. > > > > It doesn't look like the existing tests cover this aspect, so we'll need > to > > > > backfill a few tests here. > > > > > > To be noted, these are not actually mock objects, but rather fakes. Should I > > go > > > ahead and just enclose these with StrictMock or wait until an actual > refactor? > > > Does StrictMock even do anything when the class it gets passed is not Mock? > > > > Ooh, right. No, StrictMock doesn't do anything for non-mocked classes. > > > > I commented on this in another CL. This fake is lightweight enough that it > > should be a mock. I won't hold up this CL for that cleanup, though. > > Acknowledged. > > https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... > File device/geolocation/location_arbitrator_impl.cc (right): > > https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... > device/geolocation/location_arbitrator_impl.cc:89: started = > provider->StartProvider(enable_high_accuracy_) || started; > On 2016/08/17 17:44:37, Kevin M wrote: > > super nit: put "started || before provider->StartProvider" to make the logical > > OR absolutely clear to readers, as a hedge against coders' tendency to skim > and > > gloss over details. ;) > > Because of short-circuiting, once the first StartProvider returns true and flips > started to true, we no longer will call StartProvider, thus to ensure > StartProvider gets called, it must be called first. Tested it on Chrome. All unit tests clear, and I put a Chromium build on the test device and had Google tell me my location in a few places. Seemed to work. Let me know if there is any thing more I should do.
Nice cleanup :-) lgtm
Very nice - almost there! https://codereview.chromium.org/2226143002/diff/240001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2226143002/diff/240001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:51: void BlimpLocationProvider::RequestRefresh() { Nothing calls this API except OnPermissionGranted, so how about just moving this code there, and removing this method? What does the delegate's RequestRefresh() actually do, though? Is there any point calling it? Should it be an explicit OnPermissionGranted method as well? https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl.h (right): https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... device/geolocation/geolocation_provider_impl.h:95: // Used to PostTask()s from the geolocation thread to creation thread. s/creation/caller - effectively the caller just expects us to work on a single thread, and this class' use of a secondary thread is irrelevant to the caller. https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:163: provider()->SetArbitratorForTesting(nullptr); Add a comment to explain that we're clearing this to use the default arbitrator here. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:89: started = provider->StartProvider(enable_high_accuracy_) || started; On 2016/08/17 23:07:11, CJ wrote: > On 2016/08/17 17:44:37, Kevin M wrote: > > super nit: put "started || before provider->StartProvider" to make the logical > > OR absolutely clear to readers, as a hedge against coders' tendency to skim > and > > gloss over details. ;) > > Because of short-circuiting, once the first StartProvider returns true and flips > started to true, we no longer will call StartProvider, thus to ensure > StartProvider gets called, it must be called first. Acknowledged. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:223: bool LocationArbitratorImpl::HasPermissionBeenGrantedForTest() const { nit: Move this impl to the equivalent position in this file to match its new position in the class header, i.e. just after the Default URL stuff. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... File device/geolocation/location_provider_android.h (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_provider_android.h:34: void RequestRefresh(); Not sure what you intended here but: (1) This is private, so no code outside this class can call it to provide a hint. (2) Nothing inside the class calls it. (3) It doesn't do anything, even if they did - pretty sure the Android impl only override'd it because it had to! https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/moc... File device/geolocation/mock_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/moc... device/geolocation/mock_location_provider.h:26: bool IsProviderStarted() const; nit: This can be an in-line "trivial getter", i.e: bool is_started() const { return state_ != STOPPED; } https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/moc... device/geolocation/mock_location_provider.h:39: scoped_refptr<base::SingleThreadTaskRunner> provider_task_runner_; These three members should be private, with trivial getters for any of them that tests need to be able to access. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/net... File device/geolocation/network_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/net... device/geolocation/network_location_provider.h:82: void RequestRefresh(); This comment implies that external callers provide that hint, which they can't - this is now an implementation detail and it's called to ensure we get frash location data when permission is granted or the wi-fi state changes. It's also worth considering whether the two tests in RequestRefresh could just be folded into RequestPosition().
https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:33: const LocationUpdateCallback& callback, Looks like we should get rid of this parameter, since caller should use SetUpdateCallback() to set the arbitrator_update_callback. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:155: provider_update_callback_ = callback; This doesn't look right - you should be (re)assigning the abirtrator_update_callback_ here. I'm surprised that none of the tests caught this, since it'll mean that the callback gets called with the LocationProvider pointer set to the wrong value. I'm also surprised that things compile as-is, since OnLocationUpdate is still calling arbitrator_update_callback_ with a single parameter (i.e. it's failing to pass the LocationProvider parameter!) https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.h:45: typedef base::Callback<void(const Geoposition&)> LocationUpdateCallback; You can kill this type, since the arbitrator will now dispatch updates via the LocationProviderUpdateCallback the caller provides.
On 2016/08/19 01:56:31, Wez wrote: > https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... > File device/geolocation/location_arbitrator_impl.cc (right): > > https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... > device/geolocation/location_arbitrator_impl.cc:33: const LocationUpdateCallback& > callback, > Looks like we should get rid of this parameter, since caller should use > SetUpdateCallback() to set the arbitrator_update_callback. > > https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... > device/geolocation/location_arbitrator_impl.cc:155: provider_update_callback_ = > callback; > This doesn't look right - you should be (re)assigning the > abirtrator_update_callback_ here. > > I'm surprised that none of the tests caught this, since it'll mean that the > callback gets called with the LocationProvider pointer set to the wrong value. > > I'm also surprised that things compile as-is, since OnLocationUpdate is still > calling arbitrator_update_callback_ with a single parameter (i.e. it's failing > to pass the LocationProvider parameter!) > > https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... > File device/geolocation/location_arbitrator_impl.h (right): > > https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... > device/geolocation/location_arbitrator_impl.h:45: typedef > base::Callback<void(const Geoposition&)> LocationUpdateCallback; > You can kill this type, since the arbitrator will now dispatch updates via the > LocationProviderUpdateCallback the caller provides. Ah, looks like the type of the arbitrator_update_callback hadn't been switched over, which explains why things still compile.
https://codereview.chromium.org/2226143002/diff/240001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2226143002/diff/240001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:51: void BlimpLocationProvider::RequestRefresh() { On 2016/08/19 01:33:11, Wez wrote: > Nothing calls this API except OnPermissionGranted, so how about just moving this > code there, and removing this method? > > What does the delegate's RequestRefresh() actually do, though? Is there any > point calling it? Should it be an explicit OnPermissionGranted method as well? In most cases RequestRefresh() is only called by OnPermissionGranted or private classes, so for this case I think its fine to just move it to OnPermissionGranted. As for changing the delegate, right now I don't see anything that should keep it as RequestRefresh either. If we do this though, do we want to change the message too? I'm reluctant because that is what we are doing on OnPermissionGranted(), requesting a refresh, but it also makes sense to do it since the client right now processes it into a OnPermissionGranted call anyway. What do you think? https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl.h (right): https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... device/geolocation/geolocation_provider_impl.h:95: // Used to PostTask()s from the geolocation thread to creation thread. On 2016/08/19 01:33:11, Wez wrote: > s/creation/caller - effectively the caller just expects us to work on a single > thread, and this class' use of a secondary thread is irrelevant to the caller. Done. https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:163: provider()->SetArbitratorForTesting(nullptr); On 2016/08/19 01:33:11, Wez wrote: > Add a comment to explain that we're clearing this to use the default arbitrator > here. Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:33: const LocationUpdateCallback& callback, On 2016/08/19 01:56:30, Wez wrote: > Looks like we should get rid of this parameter, since caller should use > SetUpdateCallback() to set the arbitrator_update_callback. Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:155: provider_update_callback_ = callback; On 2016/08/19 01:56:30, Wez wrote: > This doesn't look right - you should be (re)assigning the > abirtrator_update_callback_ here. > > I'm surprised that none of the tests caught this, since it'll mean that the > callback gets called with the LocationProvider pointer set to the wrong value. > > I'm also surprised that things compile as-is, since OnLocationUpdate is still > calling arbitrator_update_callback_ with a single parameter (i.e. it's failing > to pass the LocationProvider parameter!) Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:223: bool LocationArbitratorImpl::HasPermissionBeenGrantedForTest() const { On 2016/08/19 01:33:11, Wez wrote: > nit: Move this impl to the equivalent position in this file to match its new > position in the class header, i.e. just after the Default URL stuff. Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.h:45: typedef base::Callback<void(const Geoposition&)> LocationUpdateCallback; On 2016/08/19 01:56:31, Wez wrote: > You can kill this type, since the arbitrator will now dispatch updates via the > LocationProviderUpdateCallback the caller provides. Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... File device/geolocation/location_provider_android.h (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_provider_android.h:34: void RequestRefresh(); On 2016/08/19 01:33:12, Wez wrote: > Not sure what you intended here but: > (1) This is private, so no code outside this class can call it to provide a > hint. > (2) Nothing inside the class calls it. > (3) It doesn't do anything, even if they did - pretty sure the Android impl only > override'd it because it had to! Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/moc... File device/geolocation/mock_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/moc... device/geolocation/mock_location_provider.h:26: bool IsProviderStarted() const; On 2016/08/19 01:33:12, Wez wrote: > nit: This can be an in-line "trivial getter", i.e: > > bool is_started() const { return state_ != STOPPED; } Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/moc... device/geolocation/mock_location_provider.h:39: scoped_refptr<base::SingleThreadTaskRunner> provider_task_runner_; On 2016/08/19 01:33:12, Wez wrote: > These three members should be private, with trivial getters for any of them that > tests need to be able to access. Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/net... File device/geolocation/network_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/net... device/geolocation/network_location_provider.h:82: void RequestRefresh(); On 2016/08/19 01:33:12, Wez wrote: > This comment implies that external callers provide that hint, which they can't - > this is now an implementation detail and it's called to ensure we get frash > location data when permission is granted or the wi-fi state changes. > > It's also worth considering whether the two tests in RequestRefresh could just > be folded into RequestPosition(). Done. Which tests are you talking about? (Not seeing them).
https://codereview.chromium.org/2226143002/diff/240001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2226143002/diff/240001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:51: void BlimpLocationProvider::RequestRefresh() { On 2016/08/19 01:33:11, Wez wrote: > Nothing calls this API except OnPermissionGranted, so how about just moving this > code there, and removing this method? > > What does the delegate's RequestRefresh() actually do, though? Is there any > point calling it? Should it be an explicit OnPermissionGranted method as well? In most cases RequestRefresh() is only called by OnPermissionGranted or private classes, so for this case I think its fine to just move it to OnPermissionGranted. As for changing the delegate, right now I don't see anything that should keep it as RequestRefresh either. If we do this though, do we want to change the message too? I'm reluctant because that is what we are doing on OnPermissionGranted(), requesting a refresh, but it also makes sense to do it since the client right now processes it into a OnPermissionGranted call anyway. What do you think? https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl.h (right): https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... device/geolocation/geolocation_provider_impl.h:95: // Used to PostTask()s from the geolocation thread to creation thread. On 2016/08/19 01:33:11, Wez wrote: > s/creation/caller - effectively the caller just expects us to work on a single > thread, and this class' use of a secondary thread is irrelevant to the caller. Done. https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2226143002/diff/240001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:163: provider()->SetArbitratorForTesting(nullptr); On 2016/08/19 01:33:11, Wez wrote: > Add a comment to explain that we're clearing this to use the default arbitrator > here. Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl.cc (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:33: const LocationUpdateCallback& callback, On 2016/08/19 01:56:30, Wez wrote: > Looks like we should get rid of this parameter, since caller should use > SetUpdateCallback() to set the arbitrator_update_callback. Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:155: provider_update_callback_ = callback; On 2016/08/19 01:56:30, Wez wrote: > This doesn't look right - you should be (re)assigning the > abirtrator_update_callback_ here. > > I'm surprised that none of the tests caught this, since it'll mean that the > callback gets called with the LocationProvider pointer set to the wrong value. > > I'm also surprised that things compile as-is, since OnLocationUpdate is still > calling arbitrator_update_callback_ with a single parameter (i.e. it's failing > to pass the LocationProvider parameter!) Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.cc:223: bool LocationArbitratorImpl::HasPermissionBeenGrantedForTest() const { On 2016/08/19 01:33:11, Wez wrote: > nit: Move this impl to the equivalent position in this file to match its new > position in the class header, i.e. just after the Default URL stuff. Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl.h (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_arbitrator_impl.h:45: typedef base::Callback<void(const Geoposition&)> LocationUpdateCallback; On 2016/08/19 01:56:31, Wez wrote: > You can kill this type, since the arbitrator will now dispatch updates via the > LocationProviderUpdateCallback the caller provides. Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... File device/geolocation/location_provider_android.h (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/loc... device/geolocation/location_provider_android.h:34: void RequestRefresh(); On 2016/08/19 01:33:12, Wez wrote: > Not sure what you intended here but: > (1) This is private, so no code outside this class can call it to provide a > hint. > (2) Nothing inside the class calls it. > (3) It doesn't do anything, even if they did - pretty sure the Android impl only > override'd it because it had to! Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/moc... File device/geolocation/mock_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/moc... device/geolocation/mock_location_provider.h:26: bool IsProviderStarted() const; On 2016/08/19 01:33:12, Wez wrote: > nit: This can be an in-line "trivial getter", i.e: > > bool is_started() const { return state_ != STOPPED; } Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/moc... device/geolocation/mock_location_provider.h:39: scoped_refptr<base::SingleThreadTaskRunner> provider_task_runner_; On 2016/08/19 01:33:12, Wez wrote: > These three members should be private, with trivial getters for any of them that > tests need to be able to access. Done. https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/net... File device/geolocation/network_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/net... device/geolocation/network_location_provider.h:82: void RequestRefresh(); On 2016/08/19 01:33:12, Wez wrote: > This comment implies that external callers provide that hint, which they can't - > this is now an implementation detail and it's called to ensure we get frash > location data when permission is granted or the wi-fi state changes. > > It's also worth considering whether the two tests in RequestRefresh could just > be folded into RequestPosition(). Done. Which tests are you talking about? (Not seeing them).
On 2016/08/22 17:41:34, CJ wrote: Ping Wez. I'd like to get this in very soon :)
Almost! https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/net... File device/geolocation/network_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/net... device/geolocation/network_location_provider.h:82: void RequestRefresh(); On 2016/08/22 17:41:31, CJ wrote: > On 2016/08/19 01:33:12, Wez wrote: > > This comment implies that external callers provide that hint, which they can't > - > > this is now an implementation detail and it's called to ensure we get frash > > location data when permission is granted or the wi-fi state changes. > > > > It's also worth considering whether the two tests in RequestRefresh could just > > be folded into RequestPosition(). > > Done. Which tests are you talking about? (Not seeing them). All that RequestRefresh() does is: if (!weak_factory_.HasWeakPtrs() || is_wifi_data_complete_) { RequestPosition() } So can we just move that if() into RequestPosition() and get rid of RequestRefresh()? Would that break anything? https://codereview.chromium.org/2226143002/diff/360001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/360001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:29: virtual void OnPermissionGranted() = 0; nit: I'd suggest leaving this as RequestRefresh(), since the delegate doesn't have any notion of permissions - only the LocationProvider cares about permissions being granted, and the call it makes to the Delegate really is just to get it to refresh the location ASAP (I think ;) https://codereview.chromium.org/2226143002/diff/360001/device/geolocation/moc... File device/geolocation/mock_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/360001/device/geolocation/moc... device/geolocation/mock_location_provider.h:27: bool is_permission_granted() { return is_permission_granted_; } nit: This should also be a const getter https://codereview.chromium.org/2226143002/diff/360001/device/geolocation/moc... device/geolocation/mock_location_provider.h:28: const Geoposition& position() { return position_; } As should this.
https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/net... File device/geolocation/network_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/340001/device/geolocation/net... device/geolocation/network_location_provider.h:82: void RequestRefresh(); On 2016/08/24 04:13:17, Wez wrote: > On 2016/08/22 17:41:31, CJ wrote: > > On 2016/08/19 01:33:12, Wez wrote: > > > This comment implies that external callers provide that hint, which they > can't > > - > > > this is now an implementation detail and it's called to ensure we get frash > > > location data when permission is granted or the wi-fi state changes. > > > > > > It's also worth considering whether the two tests in RequestRefresh could > just > > > be folded into RequestPosition(). > > > > Done. Which tests are you talking about? (Not seeing them). > > All that RequestRefresh() does is: > > if (!weak_factory_.HasWeakPtrs() || is_wifi_data_complete_) { > RequestPosition() > } > > So can we just move that if() into RequestPosition() and get rid of > RequestRefresh()? Would that break anything? Doesnt seem to. https://codereview.chromium.org/2226143002/diff/360001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/360001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:29: virtual void OnPermissionGranted() = 0; On 2016/08/24 04:13:17, Wez wrote: > nit: I'd suggest leaving this as RequestRefresh(), since the delegate doesn't > have any notion of permissions - only the LocationProvider cares about > permissions being granted, and the call it makes to the Delegate really is just > to get it to refresh the location ASAP (I think ;) Done. https://codereview.chromium.org/2226143002/diff/360001/device/geolocation/moc... File device/geolocation/mock_location_provider.h (right): https://codereview.chromium.org/2226143002/diff/360001/device/geolocation/moc... device/geolocation/mock_location_provider.h:27: bool is_permission_granted() { return is_permission_granted_; } On 2016/08/24 04:13:17, Wez wrote: > nit: This should also be a const getter Done. https://codereview.chromium.org/2226143002/diff/360001/device/geolocation/moc... device/geolocation/mock_location_provider.h:28: const Geoposition& position() { return position_; } On 2016/08/24 04:13:17, Wez wrote: > As should this. Done.
https://codereview.chromium.org/2226143002/diff/380001/device/geolocation/net... File device/geolocation/network_location_provider.cc (right): https://codereview.chromium.org/2226143002/diff/380001/device/geolocation/net... device/geolocation/network_location_provider.cc:221: if (!weak_factory_.HasWeakPtrs() || is_wifi_data_complete_) { Let's use an early-exit for this (like |is_new_data_available_| below) rather than indent the whole rest of the function! https://codereview.chromium.org/2226143002/diff/380001/device/geolocation/net... device/geolocation/network_location_provider.cc:222: DCHECK(CalledOnValidThread()); This DCHECK ensures we're called on the correct thread, so belongs at the start of the function.
https://codereview.chromium.org/2226143002/diff/380001/device/geolocation/net... File device/geolocation/network_location_provider.cc (right): https://codereview.chromium.org/2226143002/diff/380001/device/geolocation/net... device/geolocation/network_location_provider.cc:221: if (!weak_factory_.HasWeakPtrs() || is_wifi_data_complete_) { On 2016/08/24 23:20:36, Wez wrote: > Let's use an early-exit for this (like |is_new_data_available_| below) rather > than indent the whole rest of the function! Done. https://codereview.chromium.org/2226143002/diff/380001/device/geolocation/net... device/geolocation/network_location_provider.cc:222: DCHECK(CalledOnValidThread()); On 2016/08/24 23:20:36, Wez wrote: > This DCHECK ensures we're called on the correct thread, so belongs at the start > of the function. Done.
lgtm
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2226143002/#ps400001 (title: "In response to Wez's #82 comments.")
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org, wez@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2226143002/#ps420001 (title: "Merge branch 'master' into lai")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org, wez@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2226143002/#ps440001 (title: "Merge branch 'master' into lai")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lethalantidote@chromium.org
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org, wez@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2226143002/#ps460001 (title: "Merge branch 'master' into lai")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lethalantidote@chromium.org
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lethalantidote@chromium.org
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lethalantidote@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Gets rid of the LocationArbitrator interface, in preference for LocationProvider. This CL reorganizes classes that derived from LocationArbitrator to instead derive from LocationProvider. There was a quite a bit of overlap between the two interfaces, and merging the two in favor of LocationProvider makes later work with Blimp much easier. Other work included: *Addresses TODO to remove CreateArbitrator in favor of SetArbitratorForTest BUG=614486, 637869 ========== to ========== Gets rid of the LocationArbitrator interface, in preference for LocationProvider. This CL reorganizes classes that derived from LocationArbitrator to instead derive from LocationProvider. There was a quite a bit of overlap between the two interfaces, and merging the two in favor of LocationProvider makes later work with Blimp much easier. Other work included: *Addresses TODO to remove CreateArbitrator in favor of SetArbitratorForTest BUG=614486, 637869 ==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== Gets rid of the LocationArbitrator interface, in preference for LocationProvider. This CL reorganizes classes that derived from LocationArbitrator to instead derive from LocationProvider. There was a quite a bit of overlap between the two interfaces, and merging the two in favor of LocationProvider makes later work with Blimp much easier. Other work included: *Addresses TODO to remove CreateArbitrator in favor of SetArbitratorForTest BUG=614486, 637869 ========== to ========== Gets rid of the LocationArbitrator interface, in preference for LocationProvider. This CL reorganizes classes that derived from LocationArbitrator to instead derive from LocationProvider. There was a quite a bit of overlap between the two interfaces, and merging the two in favor of LocationProvider makes later work with Blimp much easier. Other work included: *Addresses TODO to remove CreateArbitrator in favor of SetArbitratorForTest BUG=614486, 637869 Committed: https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9 Cr-Commit-Position: refs/heads/master@{#414735} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/d04d18d7205ad8f2ea24bfa956a833193c6531f9 Cr-Commit-Position: refs/heads/master@{#414735} |
