Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2161223003:
Adds GeolocationFeature for Blimp Geolocation project. (Closed)
|
Created:
4 years, 5 months ago by CJ Modified:
4 years, 3 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@engine_feature_prep Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds GeolocationFeature for Blimp Geolocation project.
This change provides the client-side classes needed to receive and send
geolocation messages. GeolocationFeature sends messages from the client
that either contain a geolocation coordinate or error message. The
GeolocationFeature class handles incoming GeoloationSetInterestLevelMessages
and RequestRefreshMessages.
Other items included:
* Changes device/geolocation/mock_location_provider to use fake_location_provider name
* Moves blimp/client/feature/mock_location_provider into device/geolocation.
BUG=614486
Committed: https://crrev.com/7a376a1a71743b1b50c6fb9c206ffbb0ca8c8ba2
Cr-Commit-Position: refs/heads/master@{#414797}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Addresses kmarshall's #8 comments and adds unittest #
Total comments: 11
Patch Set 3 : Addresses Wez's #10 comments #Patch Set 4 : Addresses Wez's #12 comment #
Total comments: 26
Patch Set 5 : Addresses kmarshall's #14 comments #
Total comments: 22
Patch Set 6 : Addresses kmarshall's #16 comments #
Total comments: 20
Patch Set 7 : Addresses Wez's #18 feedback #
Total comments: 4
Patch Set 8 : Addresses Wez's #21 comments #Patch Set 9 : Adds deps for device/geolocation. #Patch Set 10 : Fixes BUILD to include //device/geolocation #
Total comments: 10
Patch Set 11 : Fixes BUILD.gn #Patch Set 12 : Addresses kmarshall's #33 comments #Patch Set 13 : Adding more deps to BUILD for device/geolocation. #
Total comments: 7
Patch Set 14 : Addresses kmarshall's #49 comments #Patch Set 15 : Addresses Wez's #50 comments and adds DEP file #Patch Set 16 : Update BUILD. #Patch Set 17 : Makes UnexpectedMessage test work outside of debug #
Total comments: 34
Patch Set 18 : Addresses kmarshall's #66 comments #
Total comments: 2
Patch Set 19 : Addresses kmarshall's #68 comments #Patch Set 20 : Merge branch 'lai' into client_feature #Patch Set 21 : Moves geolocation files to blimp/client/core/geolocation #Patch Set 22 : Clean up. #
Total comments: 6
Patch Set 23 : Addresses nyquist's #79 comments #
Total comments: 43
Patch Set 24 : In response to Wez's #81 comments. #Patch Set 25 : Addresses kmarshall's #82 comments. #
Total comments: 8
Patch Set 26 : Addressses Wez's #94 comments. #
Total comments: 3
Patch Set 27 : Addresses kmarshall's #102 comment. #
Total comments: 4
Patch Set 28 : Addresses Wez's #105 comments. #Patch Set 29 : Merge branch 'master' into client_feature #Patch Set 30 : Merge branch 'master' into client_feature #Patch Set 31 : More merging. #Depends on Patchset: Messages
Total messages: 118 (65 generated)
Description was changed from ========== Adds GeolocationFeature for Blimp Geolocation project. This change provides the client-side classes needed to receive and send geolocation messages. GeolocationFeature sends messages from the client that either contain a geolocation coordinate or error message. The GeolocationFeature class handles incoming GeoloationSetInterestLevelMessages and RequestRefreshMessages. Unittests forthcoming with first CL response. BUG=614486 ========== to ========== Adds GeolocationFeature for Blimp Geolocation project. This change provides the client-side classes needed to receive and send geolocation messages. GeolocationFeature sends messages from the client that either contain a geolocation coordinate or error message. The GeolocationFeature class handles incoming GeoloationSetInterestLevelMessages and RequestRefreshMessages. Unittests forthcoming with first CL response. BUG=614486 ==========
lethalantidote@chromium.org changed reviewers: + kmarshall@chromium.org, wez@chromium.org
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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) 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_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_...)
https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.cc:80: // This case shouldn't be hit. Error? Use NOTREACHED() instead of a comment. https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.cc:103: if (set_level_message.has_level()) { If it doesn't have a level, doesn't this mean that the client has violated the protocol? https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.cc:114: DLOG(FATAL) << "Engine sent unexpected message type."; DLOG only crashes on debug builds. We should fail the connection for release builds, ie. by invoking |callback| with an error code. https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... File blimp/client/feature/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:17: // Handles all incoming and outgoing protobu messages of type typo This is a literal description of how the class behaves, but I think a more succinct, plain-English description might suffice. https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:25: virtual ~GeolocationFeatureDelegate() {} Don't need a virtual dtor if the delegate object isn't owned/deleted here. https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:26: virtual void OnGeolocationInterestUpdate( Document each method and add newlines https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:27: GeolocationSetInterestLevelMessage::Level level); Make these methods pure virtual. https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:39: // Sets a GeolocationFeatureDelegate to be notified of all geolocation "messages" probably not so much... how about "events", or "updates"? https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:43: void OnLocationUpdate(const content::Geoposition& position); Add comment https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:50: void SendGeolocationPositionMessage(const content::Geoposition& position); Add comment https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:51: void SendGeolocationErrorMessage(const content::Geoposition& position); Add comment https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:53: GeolocationFeatureDelegate* delegate_; = nullptr;
https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.cc:80: // This case shouldn't be hit. Error? On 2016/07/20 17:30:15, Kevin M wrote: > Use NOTREACHED() instead of a comment. Cool thanks. Are using comments to ask questions about code like a thing, or should I avoid doing that in the future? https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.cc:103: if (set_level_message.has_level()) { On 2016/07/20 17:30:15, Kevin M wrote: > If it doesn't have a level, doesn't this mean that the client has violated the > protocol? True. I'm thinking the delegate should handle this check. What do you think? https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.cc:114: DLOG(FATAL) << "Engine sent unexpected message type."; On 2016/07/20 17:30:15, Kevin M wrote: > DLOG only crashes on debug builds. We should fail the connection for release > builds, ie. by invoking |callback| with an error code. Done. https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... File blimp/client/feature/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:17: // Handles all incoming and outgoing protobu messages of type On 2016/07/20 17:30:15, Kevin M wrote: > typo > > This is a literal description of how the class behaves, but I think a more > succinct, plain-English description might suffice. Done. https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:25: virtual ~GeolocationFeatureDelegate() {} On 2016/07/20 17:30:16, Kevin M wrote: > Don't need a virtual dtor if the delegate object isn't owned/deleted here. Done. https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:26: virtual void OnGeolocationInterestUpdate( On 2016/07/20 17:30:16, Kevin M wrote: > Document each method and add newlines Done. https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:27: GeolocationSetInterestLevelMessage::Level level); On 2016/07/20 17:30:15, Kevin M wrote: > Make these methods pure virtual. Done. https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:39: // Sets a GeolocationFeatureDelegate to be notified of all geolocation On 2016/07/20 17:30:16, Kevin M wrote: > "messages" probably not so much... how about "events", or "updates"? Done. https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:43: void OnLocationUpdate(const content::Geoposition& position); On 2016/07/20 17:30:15, Kevin M wrote: > Add comment Done. https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:50: void SendGeolocationPositionMessage(const content::Geoposition& position); On 2016/07/20 17:30:15, Kevin M wrote: > Add comment Done. https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:51: void SendGeolocationErrorMessage(const content::Geoposition& position); On 2016/07/20 17:30:16, Kevin M wrote: > Add comment Done. https://codereview.chromium.org/2161223003/diff/1/blimp/client/feature/geoloc... blimp/client/feature/geolocation_feature.h:53: GeolocationFeatureDelegate* delegate_; On 2016/07/20 17:30:16, Kevin M wrote: > = nullptr; Done.
https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.cc:81: NOTREACHED() << "ERROR_CODE_NONE GIVEN."; nit: No need for the additional text; NOTREACHED() documents that this shouldn't be reached https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.cc:94: DCHECK(!callback.is_null()); As discussed in the Engine*Feature CL, the API does not require a non-null |callback|. https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.h:22: class GeolocationFeatureDelegate { Why does the GeolocationFeature need its own Delegate interface, rather than just working with a LocationProvider directly? https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.h:42: void SetDelegate(GeolocationFeatureDelegate* delegate); You're passing in a bare-pointer, so be clear about the ownership/lifetime expectations of |delegate| https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.h:58: void SendGeolocationErrorMessage(const content::Geoposition& position); nit: I'd suggest having this accept the error_code field from a Geoposition, rather than a whole Geoposition, for clarity (otherwise it's not clear what the function expects the other fields to be!)
https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.cc:81: NOTREACHED() << "ERROR_CODE_NONE GIVEN."; On 2016/07/22 01:35:08, Wez wrote: > nit: No need for the additional text; NOTREACHED() documents that this shouldn't > be reached Done. https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.cc:94: DCHECK(!callback.is_null()); On 2016/07/22 01:35:08, Wez wrote: > As discussed in the Engine*Feature CL, the API does not require a non-null > |callback|. Done. https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.h:22: class GeolocationFeatureDelegate { On 2016/07/22 01:35:08, Wez wrote: > Why does the GeolocationFeature need its own Delegate interface, rather than > just working with a LocationProvider directly? At this point in time it is not planned to work with a LocationProvider directly. The original plan was to have some class to handle bridging the gap between the GeolocationProviderImpl and the GeolocationFeature. If it makes sense for that to be a LocationProvider, I'll be happy to refactor it to be so. https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.h:42: void SetDelegate(GeolocationFeatureDelegate* delegate); On 2016/07/22 01:35:08, Wez wrote: > You're passing in a bare-pointer, so be clear about the ownership/lifetime > expectations of |delegate| Made it into a unique_ptr handled by the GeolocationFeature https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.h:58: void SendGeolocationErrorMessage(const content::Geoposition& position); On 2016/07/22 01:35:08, Wez wrote: > nit: I'd suggest having this accept the error_code field from a Geoposition, > rather than a whole Geoposition, for clarity (otherwise it's not clear what the > function expects the other fields to be!) Done.
Re my suggestion to use LocationProvider - if there's a specific reason not to use that interface here then that's fine, but if not then it seems a waste to introduce another intermediate Delegate! https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.h:22: class GeolocationFeatureDelegate { On 2016/07/22 19:22:35, CJ wrote: > On 2016/07/22 01:35:08, Wez wrote: > > Why does the GeolocationFeature need its own Delegate interface, rather than > > just working with a LocationProvider directly? > > At this point in time it is not planned to work with a LocationProvider > directly. The original plan was to have some class to handle bridging the gap > between the GeolocationProviderImpl and the GeolocationFeature. If it makes > sense for that to be a LocationProvider, I'll be happy to refactor it to be so. I would recommend having GeolocationFeature instantiated with a LocationProvider to use, rather than adding another intermediate layer of glue, I think - the glue layer would be so trivial as to not really be worth it, IMO! Using LocationProvider would also mean that you could move OnLocationUpdate to be an implementation detail (you'd Bind() it to create the callback to pass to the LocationProvider), and you can remove SetDelegate and just pass the LocationProvider as a constructor parameter.
On 2016/07/22 22:14:51, Wez wrote: > Re my suggestion to use LocationProvider - if there's a specific reason not to > use that interface here then that's fine, but if not then it seems a waste to > introduce another intermediate Delegate! > > https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... > File blimp/client/feature/geolocation_feature.h (right): > > https://codereview.chromium.org/2161223003/diff/20001/blimp/client/feature/ge... > blimp/client/feature/geolocation_feature.h:22: class GeolocationFeatureDelegate > { > On 2016/07/22 19:22:35, CJ wrote: > > On 2016/07/22 01:35:08, Wez wrote: > > > Why does the GeolocationFeature need its own Delegate interface, rather than > > > just working with a LocationProvider directly? > > > > At this point in time it is not planned to work with a LocationProvider > > directly. The original plan was to have some class to handle bridging the gap > > between the GeolocationProviderImpl and the GeolocationFeature. If it makes > > sense for that to be a LocationProvider, I'll be happy to refactor it to be > so. > > I would recommend having GeolocationFeature instantiated with a LocationProvider > to use, rather than adding another intermediate layer of glue, I think - the > glue layer would be so trivial as to not really be worth it, IMO! > > Using LocationProvider would also mean that you could move OnLocationUpdate to > be an implementation detail (you'd Bind() it to create the callback to pass to > the LocationProvider), and you can remove SetDelegate and just pass the > LocationProvider as a constructor parameter. Can I do that in a second CL? Right now I dont know what this LocationProvider will exactly look like, and I rather refactor this to use a LocationProvider once I have that LocationProvider fleshed out.
https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.cc:55: case GeolocationMessage::TYPE_NOT_SET: Break visibly on debug builds: DLOG(FATAL) << "Unsupported message type."; https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.h:54: std::unique_ptr<content::LocationProvider> location_provider_; Comment on the role of location_provider_? https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:26: void SendMockSetInterestLevelMessage( If you make this a protected member of the test class, you can just call feature_ directly instead of taking *processor. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:42: void SendMockUnexpectedMessage(BlimpMessageProcessor* processor) { This only has one caller; no need to have a helper function for this. Ditto for SendMockRequestRefreshMessage https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:70: arg.geolocation().coordinates().latitude() == -42.0 && Move these into kConstants. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:97: // These are a raw pointers to classes that are "these are raw pointers". Remove newline below out_processor_ to make it clear that this comment applies to both fields. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:102: std::unique_ptr<GeolocationFeature> feature_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:102: std::unique_ptr<GeolocationFeature> feature_; Add newline above https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:102: std::unique_ptr<GeolocationFeature> feature_; Does this need to be heap-allocated? Can't it just be a standalone GeolocationFeature? https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:119: EXPECT_CALL(*location_provider_, StartProvider(_)).Times(0); Use StrictMock, then you don't have to specify Times(0) or Times(1) anywhere. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:158: content::Geoposition position; add newline above, remove newline below https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:162: position.error_message = "Position unavailable"; IMO checking for error message equality is too brittle, for the same reason as checking log output. Just checking the code should suffice. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/mo... File blimp/client/feature/mock_location_provider.h (right): https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/mo... blimp/client/feature/mock_location_provider.h:29: LocationProviderUpdateCallback callback_; Move the callback outside the mock class; turn SetUpdateCallback into a MOCK_METHOD, and use SaveArg<0>(&callback) to retain the callback for later invocation.
https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.cc:55: case GeolocationMessage::TYPE_NOT_SET: On 2016/07/28 21:32:42, Kevin M wrote: > Break visibly on debug builds: > > DLOG(FATAL) << "Unsupported message type."; Done. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.h:54: std::unique_ptr<content::LocationProvider> location_provider_; On 2016/07/28 21:32:42, Kevin M wrote: > Comment on the role of location_provider_? Done. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:26: void SendMockSetInterestLevelMessage( On 2016/07/28 21:32:43, Kevin M wrote: > If you make this a protected member of the test class, you can just call > feature_ directly instead of taking *processor. Done. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:42: void SendMockUnexpectedMessage(BlimpMessageProcessor* processor) { On 2016/07/28 21:32:43, Kevin M wrote: > This only has one caller; no need to have a helper function for this. > > Ditto for SendMockRequestRefreshMessage Done. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:70: arg.geolocation().coordinates().latitude() == -42.0 && On 2016/07/28 21:32:42, Kevin M wrote: > Move these into kConstants. Done. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:97: // These are a raw pointers to classes that are On 2016/07/28 21:32:43, Kevin M wrote: > "these are raw pointers". > > Remove newline below out_processor_ to make it clear that this comment applies > to both fields. Done. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:102: std::unique_ptr<GeolocationFeature> feature_; On 2016/07/28 21:32:42, Kevin M wrote: > Does this need to be heap-allocated? Can't it just be a standalone > GeolocationFeature? Done. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:102: std::unique_ptr<GeolocationFeature> feature_; On 2016/07/28 21:32:43, Kevin M wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:102: std::unique_ptr<GeolocationFeature> feature_; On 2016/07/28 21:32:43, Kevin M wrote: > Add newline above Done. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:119: EXPECT_CALL(*location_provider_, StartProvider(_)).Times(0); On 2016/07/28 21:32:43, Kevin M wrote: > Use StrictMock, then you don't have to specify Times(0) or Times(1) anywhere. Done. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:158: content::Geoposition position; On 2016/07/28 21:32:42, Kevin M wrote: > add newline above, remove newline below Done. https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:162: position.error_message = "Position unavailable"; On 2016/07/28 21:32:43, Kevin M wrote: > IMO checking for error message equality is too brittle, for the same reason as > checking log output. Just checking the code should suffice. As for the DLOG FATAL, do we just want to make sure nothing gets called? https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/mo... File blimp/client/feature/mock_location_provider.h (right): https://codereview.chromium.org/2161223003/diff/60001/blimp/client/feature/mo... blimp/client/feature/mock_location_provider.h:29: LocationProviderUpdateCallback callback_; On 2016/07/28 21:32:43, Kevin M wrote: > Move the callback outside the mock class; turn SetUpdateCallback into a > MOCK_METHOD, and use SaveArg<0>(&callback) to retain the callback for later > invocation. Done.
Looking good, just some minor comments. https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.cc:13: #include "blimp/common/proto/geolocation.pb.h" Already included in .h https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.cc:40: DCHECK(location_provider_); You can DCHECK location_provider_ in the ctor, but once you've done that, you're guaranteed that it's non-null, so you can remove the DCHECK from other call sites. https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.cc:45: const GeolocationSetInterestLevelMessage& set_level_message = You can inline this and remove the braces https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.h:22: GeolocationFeature( "explicit" - also, try running git cl lint https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:55: location_provider_ = new StrictMock<MockLocationProvider>(); This works but the ownership policy isn't as robustly enforced as if you did: auto location_provider = MakeUnique<StrictMock<MockLocation>>(); location_provider_ = location_provider.get(); feature_ = MakeUnique<GeolocationFeature>(std::move(location_provider)); The concern with assigning raw pointers and then using WrapUnique is that there could be a leak if an early exit block or conditional was added in between the "new" and "WrapUnique" operations. Binding heap allocated data to unique_ptrs ASAP addresses this. https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:58: feature_ = new GeolocationFeature(base::WrapUnique(location_provider_)); Doesn't this leak? https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:65: delete location_provider_; Use std::unique_ptr for these two fields, and remove the initializer from the ctor initialization list. https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:85: // These are raw pointers to classes that are nit: premature linewrap https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:97: TEST_F(GeolocationFeatureTest, UpdateInterestLevelReceived) { Declaring an testing::InSequence object would be useful here, since ordering matters. https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:120: EXPECT_DFATAL(feature_->ProcessMessage(std::move(message), cb.callback()), This works??? Cool, I should use it. https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/mo... File blimp/client/feature/mock_location_provider.h (right): https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/mo... blimp/client/feature/mock_location_provider.h:8: #include "device/geolocation/geoposition.h" nitty nit, you can use prototypes here
https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.cc:13: #include "blimp/common/proto/geolocation.pb.h" On 2016/08/01 20:51:04, Kevin M wrote: > Already included in .h Done. https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.cc:40: DCHECK(location_provider_); On 2016/08/01 20:51:04, Kevin M wrote: > You can DCHECK location_provider_ in the ctor, but once you've done that, you're > guaranteed that it's non-null, so you can remove the DCHECK from other call > sites. Done. https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.cc:45: const GeolocationSetInterestLevelMessage& set_level_message = On 2016/08/01 20:51:04, Kevin M wrote: > You can inline this and remove the braces What does "inline" mean in this context? https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature.h:22: GeolocationFeature( On 2016/08/01 20:51:04, Kevin M wrote: > "explicit" - also, try running git cl lint git cl lint has nothing to say <<.. (Total errors found: 0) Maybe it somehow missed that one? Adding it. https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... File blimp/client/feature/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:55: location_provider_ = new StrictMock<MockLocationProvider>(); On 2016/08/01 20:51:04, Kevin M wrote: > This works but the ownership policy isn't as robustly enforced as if you did: > > auto location_provider = MakeUnique<StrictMock<MockLocation>>(); > location_provider_ = location_provider.get(); > feature_ = MakeUnique<GeolocationFeature>(std::move(location_provider)); > > The concern with assigning raw pointers and then using WrapUnique is that there > could be a leak if an early exit block or conditional was added in between the > "new" and "WrapUnique" operations. Binding heap allocated data to unique_ptrs > ASAP addresses this. Done. https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:58: feature_ = new GeolocationFeature(base::WrapUnique(location_provider_)); On 2016/08/01 20:51:05, Kevin M wrote: > Doesn't this leak? Is it still a concern if this is now a unique_ptr? https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:65: delete location_provider_; On 2016/08/01 20:51:05, Kevin M wrote: > Use std::unique_ptr for these two fields, and remove the initializer from the > ctor initialization list. Done. https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:85: // These are raw pointers to classes that are On 2016/08/01 20:51:04, Kevin M wrote: > nit: premature linewrap Done. https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:97: TEST_F(GeolocationFeatureTest, UpdateInterestLevelReceived) { On 2016/08/01 20:51:04, Kevin M wrote: > Declaring an testing::InSequence object would be useful here, since ordering > matters. Done. https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/ge... blimp/client/feature/geolocation_feature_unittest.cc:120: EXPECT_DFATAL(feature_->ProcessMessage(std::move(message), cb.callback()), On 2016/08/01 20:51:05, Kevin M wrote: > This works??? Cool, I should use it. As far as I can see it does. https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/mo... File blimp/client/feature/mock_location_provider.h (right): https://codereview.chromium.org/2161223003/diff/80001/blimp/client/feature/mo... blimp/client/feature/mock_location_provider.h:8: #include "device/geolocation/geoposition.h" On 2016/08/01 20:51:05, Kevin M wrote: > nitty nit, you can use prototypes here What do you mean by prototypes in this context?
https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:22: DCHECK(location_provider); nit: Strictly you don't need this, since you de-reference location_provider_ below. https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:23: location_provider_ = std::move(location_provider); nit: Move this std::move() to be a member-initialization. https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:55: DLOG(FATAL) << "Unsupported message type."; nit: This should be "Invalid message type." - "Unsupported" implies that we just haven't implemented it yet! However, I'd personally suggest omitting the message entirely, since only Debug builds will fire this, and the developer can see what triggered the failure from the stack + code easily enough. https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:59: callback.Run(net::OK); nit: Consider using ScopedTaskRunner for this. As with the EngineGeolocationFeature, remember that it is valid to supply ProcessMessage() with a null |callback| - ScopedTaskRunner will implicitly deal with that case for you, or you can explicitly add an is_null() check before the Run(), if you want. https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:68: SendGeolocationErrorMessage(position.error_code, position.error_message); If you change the signature of SendGeolocationErrorMessage() to take a GeolocationErrorMessage::ErrorCode as parameter then you can have this method use a switch, and get rid of the NOTREACHED() case in SendGeolocationErrorMessage. Downside is that you'll have three cases all calling SendGeolocationErrorMessage(...) in here, but then you'll only have one set_error_code() call inside that method, so overall the same # of lines, I think. https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:104: net::CompletionCallback()); nit: Perhaps we should register a completion callback and only send a new position update if the network is ready for it? https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.h:23: std::unique_ptr<device::LocationProvider> location_provider); nit: Add a comment to explain how |location_provider| is used, e.g. to make clear that it is the _only_ LocationProvider that this class will consider - this class won't e.g. create "network" location providers to gather backup information from. https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.h:41: void OnGeolocationInterestUpdate( nit: It's a little strange to have two On*() methods but going in different directions - this one is really about *handling* an incoming InterestUpdate message, right? So consider calling it HandleSetInterestLevelMessage, or simply SetInterestLevel. https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:148: TEST_F(GeolocationFeatureTest, ErrorUpdateSendsCorrectMessage) { Add a test-case for the Feature handling a null ProcessMessage() callback. :D
Looking good :D
https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:22: DCHECK(location_provider); On 2016/08/02 01:18:10, Wez wrote: > nit: Strictly you don't need this, since you de-reference location_provider_ > below. Done. https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:23: location_provider_ = std::move(location_provider); On 2016/08/02 01:18:10, Wez wrote: > nit: Move this std::move() to be a member-initialization. Done. https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:55: DLOG(FATAL) << "Unsupported message type."; On 2016/08/02 01:18:10, Wez wrote: > nit: This should be "Invalid message type." - "Unsupported" implies that we just > haven't implemented it yet! However, I'd personally suggest omitting the > message entirely, since only Debug builds will fire this, and the developer can > see what triggered the failure from the stack + code easily enough. Done. https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:59: callback.Run(net::OK); On 2016/08/02 01:18:11, Wez wrote: > nit: Consider using ScopedTaskRunner for this. > > As with the EngineGeolocationFeature, remember that it is valid to supply > ProcessMessage() with a null |callback| - ScopedTaskRunner will implicitly deal > with that case for you, or you can explicitly add an is_null() check before the > Run(), if you want. Done. https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:68: SendGeolocationErrorMessage(position.error_code, position.error_message); On 2016/08/02 01:18:10, Wez wrote: > If you change the signature of SendGeolocationErrorMessage() to take a > GeolocationErrorMessage::ErrorCode as parameter then you can have this method > use a switch, and get rid of the NOTREACHED() case in > SendGeolocationErrorMessage. > > Downside is that you'll have three cases all calling > SendGeolocationErrorMessage(...) in here, but then you'll only have one > set_error_code() call inside that method, so overall the same # of lines, I > think. Done. https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:104: net::CompletionCallback()); On 2016/08/02 01:18:10, Wez wrote: > nit: Perhaps we should register a completion callback and only send a new > position update if the network is ready for it? How does one know if the network is ready? https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.h:23: std::unique_ptr<device::LocationProvider> location_provider); On 2016/08/02 01:18:11, Wez wrote: > nit: Add a comment to explain how |location_provider| is used, e.g. to make > clear that it is the _only_ LocationProvider that this class will consider - > this class won't e.g. create "network" location providers to gather backup > information from. Done. https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.h:41: void OnGeolocationInterestUpdate( On 2016/08/02 01:18:11, Wez wrote: > nit: It's a little strange to have two On*() methods but going in different > directions - this one is really about *handling* an incoming InterestUpdate > message, right? So consider calling it HandleSetInterestLevelMessage, or simply > SetInterestLevel. Done. https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:148: TEST_F(GeolocationFeatureTest, ErrorUpdateSendsCorrectMessage) { On 2016/08/02 01:18:11, Wez wrote: > Add a test-case for the Feature handling a null ProcessMessage() callback. :D Done.
LGTM w/ nits! https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:104: net::CompletionCallback()); On 2016/08/03 20:06:03, CJ wrote: > On 2016/08/02 01:18:10, Wez wrote: > > nit: Perhaps we should register a completion callback and only send a new > > position update if the network is ready for it? > > How does one know if the network is ready? By passing a non-null CompletionCallback and setting a Boolean in the GelocationFeature - then when the callback fires you clear the Boolean. While the Bool is set you never send out any further updates. That way if the outgoing MessageProcessor is slow, you won't flood it with data. :) https://codereview.chromium.org/2161223003/diff/120001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/120001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:56: result = net::ERR_UNEXPECTED; nit: Add an explicit break; here, even though nothing comes after it - otherwise someone might add a new case here later and get a nasty surprise! https://codereview.chromium.org/2161223003/diff/120001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:58: if (!callback.is_null()) { nit: Precede this check with a blank line, to separate it from the switch, since the two blocks of code represent different stages of processing the request
https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/100001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:104: net::CompletionCallback()); On 2016/08/09 01:40:33, Wez wrote: > On 2016/08/03 20:06:03, CJ wrote: > > On 2016/08/02 01:18:10, Wez wrote: > > > nit: Perhaps we should register a completion callback and only send a new > > > position update if the network is ready for it? > > > > How does one know if the network is ready? > > By passing a non-null CompletionCallback and setting a Boolean in the > GelocationFeature - then when the callback fires you clear the Boolean. While > the Bool is set you never send out any further updates. That way if the > outgoing MessageProcessor is slow, you won't flood it with data. :) Done. https://codereview.chromium.org/2161223003/diff/120001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/120001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:56: result = net::ERR_UNEXPECTED; On 2016/08/09 01:40:33, Wez wrote: > nit: Add an explicit break; here, even though nothing comes after it - otherwise > someone might add a new case here later and get a nasty surprise! Done. https://codereview.chromium.org/2161223003/diff/120001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:58: if (!callback.is_null()) { On 2016/08/09 01:40:33, Wez wrote: > nit: Precede this check with a blank line, to separate it from the switch, since > the two blocks of code represent different stages of processing the request 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_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...) linux_chromium_clobber_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
lethalantidote@chromium.org changed reviewers: + mcasas@chromium.org
+ mcasas@chromium.org for DEPS
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...) 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...) 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_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_...)
https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:48: SetInterestLevel(set_level_message.level()); Can just inline line 47 here https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:57: DLOG(FATAL) << "Invalid message type."; output the type_case in the log https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:146: can_send_message_ = true; Should we send a new location if a Send was previously ignored? https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:74: protected: Please revisit visibility. Not quite clear why some parts are public and some are protected. What about making them all public?
On 2016/08/09 21:56:37, CJ wrote: > + mailto:mcasas@chromium.org for DEPS lgtm consider putting a specific deps in feature/ with just the one geolocation line?
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...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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_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_...)
https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:48: SetInterestLevel(set_level_message.level()); On 2016/08/09 22:08:55, Kevin M wrote: > Can just inline line 47 here Done. https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:57: DLOG(FATAL) << "Invalid message type."; On 2016/08/09 22:08:55, Kevin M wrote: > output the type_case in the log Done. https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:146: can_send_message_ = true; On 2016/08/09 22:08:55, Kevin M wrote: > Should we send a new location if a Send was previously ignored? Done. https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:74: protected: On 2016/08/09 22:08:55, Kevin M wrote: > Please revisit visibility. Not quite clear why some parts are public and some > are protected. What about making them all public? Did it, but don't we usually like things to be more private than not?
On 2016/08/09 22:10:31, mcasas wrote: > On 2016/08/09 21:56:37, CJ wrote: > > + mailto:mcasas@chromium.org for DEPS > > lgtm > consider putting a specific deps in > feature/ with just the one geolocation > line? I'm pretty new to DEP files, but since there isn't a seperate BUILD.gn file for feature/, would making a seperate DEPS make sense to do?
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...) 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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:74: protected: On 2016/08/10 00:13:49, CJ wrote: > On 2016/08/09 22:08:55, Kevin M wrote: > > Please revisit visibility. Not quite clear why some parts are public and some > > are protected. What about making them all public? > > Did it, but don't we usually like things to be more private than not? In general, the visibility should make good semantic sense and be consistent. In this patch, both RunCallback() and SendMockSetInterestLevelMessage() are called by test subclasses, but one is public and one is protected. My preference would be to have them both become protected. https://codereview.chromium.org/2161223003/diff/240001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/240001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:149: OnLocationUpdate(location_provider_.get(), position); Won't this just update endlessly? https://codereview.chromium.org/2161223003/diff/240001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/240001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.h:22: // The location_provider passed in will be the only location_provider the nit: Surround parameter names with pipes |location_provider|
https://codereview.chromium.org/2161223003/diff/240001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/240001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:149: OnLocationUpdate(location_provider_.get(), position); On 2016/08/10 23:23:34, Kevin M wrote: > Won't this just update endlessly? Right; we only want to send a new update following a send completing if it's time to send one - to do that we'd need a second bool need_to_send_position, which gets set if SendGeolocation*Message() is called when can_send_message_ is false, and only send a location here if that is still set. However, we could probably just set can_send_message = true here and let the location be sent the next time the LocationProvider calls us back. https://codereview.chromium.org/2161223003/diff/240001/blimp/client/feature/m... File blimp/client/feature/mock_location_provider.h (right): https://codereview.chromium.org/2161223003/diff/240001/blimp/client/feature/m... blimp/client/feature/mock_location_provider.h:15: class MockLocationProvider : public device::LocationProvider { This mock should be defined in the device/ directly, alongside the fake LocationProvider (which is currently called MockLocationProvider... );
Description was changed from ========== Adds GeolocationFeature for Blimp Geolocation project. This change provides the client-side classes needed to receive and send geolocation messages. GeolocationFeature sends messages from the client that either contain a geolocation coordinate or error message. The GeolocationFeature class handles incoming GeoloationSetInterestLevelMessages and RequestRefreshMessages. Unittests forthcoming with first CL response. BUG=614486 ========== to ========== Adds GeolocationFeature for Blimp Geolocation project. This change provides the client-side classes needed to receive and send geolocation messages. GeolocationFeature sends messages from the client that either contain a geolocation coordinate or error message. The GeolocationFeature class handles incoming GeoloationSetInterestLevelMessages and RequestRefreshMessages. Other items included: * Changes device/geolocation/mock_location_provider to use fake_location_provider name * Moves blimp/client/feature/mock_location_provider into device/geolocation. BUG=614486 ==========
https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/180001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:74: protected: On 2016/08/10 23:23:34, Kevin M wrote: > On 2016/08/10 00:13:49, CJ wrote: > > On 2016/08/09 22:08:55, Kevin M wrote: > > > Please revisit visibility. Not quite clear why some parts are public and > some > > > are protected. What about making them all public? > > > > Did it, but don't we usually like things to be more private than not? > > In general, the visibility should make good semantic sense and be consistent. In > this patch, both RunCallback() and SendMockSetInterestLevelMessage() are called > by test subclasses, but one is public and one is protected. My preference would > be to have them both become protected. Done. https://codereview.chromium.org/2161223003/diff/240001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/240001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:149: OnLocationUpdate(location_provider_.get(), position); On 2016/08/12 00:39:42, Wez wrote: > On 2016/08/10 23:23:34, Kevin M wrote: > > Won't this just update endlessly? > > Right; we only want to send a new update following a send completing if it's > time to send one - to do that we'd need a second bool need_to_send_position, > which gets set if SendGeolocation*Message() is called when can_send_message_ is > false, and only send a location here if that is still set. > > However, we could probably just set can_send_message = true here and let the > location be sent the next time the LocationProvider calls us back. Yeah, I wasn't sure what we are doing in the error case, if we wanted to try again right away with the current best position. I think just keeping it simple and letting the LocationProvider decide it is ready to try again would be best. https://codereview.chromium.org/2161223003/diff/240001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/240001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.h:22: // The location_provider passed in will be the only location_provider the On 2016/08/10 23:23:34, Kevin M wrote: > nit: Surround parameter names with pipes > > |location_provider| Done. https://codereview.chromium.org/2161223003/diff/240001/blimp/client/feature/m... File blimp/client/feature/mock_location_provider.h (right): https://codereview.chromium.org/2161223003/diff/240001/blimp/client/feature/m... blimp/client/feature/mock_location_provider.h:15: class MockLocationProvider : public device::LocationProvider { On 2016/08/12 00:39:42, Wez wrote: > This mock should be defined in the device/ directly, alongside the fake > LocationProvider (which is currently called MockLocationProvider... ); Ah yes, I was going to do that in the refactor CL but the name conflict put it on the back burner. Changing the current device/geolocation/MockLocationProvider to be FakeLocationProvider.
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...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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-...) 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_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_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: This issue passed the CQ dry run.
https://codereview.chromium.org/2161223003/diff/320001/blimp/client/BUILD.gn File blimp/client/BUILD.gn (right): https://codereview.chromium.org/2161223003/diff/320001/blimp/client/BUILD.gn#... blimp/client/BUILD.gn:113: "//device/geolocation:device_geolocation", Hm, this target should probably be named "geolocation", which becomes the more succinct target "//device/geolocation". Not really in your CL scope, but just pointing this out as another point for potential cleanup. https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/DEPS File blimp/client/feature/DEPS (left): https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/D... blimp/client/feature/DEPS:3: "+google_apis", Any reason why this was removed? https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:45: case GeolocationMessage::kSetInterestLevel: { No variables defined in this case block; no need to use curly braces https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:106: GeolocationCoordinatesMessage* coordinates = nit: move this to the top of the cluster of coordinates setters https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:142: void GeolocationFeature::OnSendComplete(int result) { What about sending an update that was previously suppressed? So we'd have: 1. message sent. 2. update received, suppressed. 3. update received, suppressed. 4. message sent ACK 5. location from #3 sent. https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.h:22: // |location_provider| will be the only LocationProvider the Nit: I think this is already implied by the fact that we don't have post-construction setters for this type. https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.h:52: const GeolocationErrorMessage::ErrorCode& error_code, ErrorCode is a POD type, so should be passed as-is and not as a reference. https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.h:67: // True if a message is not in the process of being sent. I think the meaning of this variable, and how it is used, would be easier to comprehend if we inverted the semantics. For example, it could be "send_active_" which starts out as false, becomes true when a message is being sent, then false again once the message is delivered. https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:66: auto out_processor = base::MakeUnique<MockBlimpMessageProcessor>(); StrictMock? Also remove Times(1) https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:166: ON_CALL(*out_processor_, MockableProcessMessage(_, _)) Use EXPECT_CALL with a specific Times() https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:194: TEST_F(GeolocationFeatureTest, NoRepeatSendsWithMessagePending) { Also verify that we can send messages again after acknowledgement. https://codereview.chromium.org/2161223003/diff/320001/device/geolocation/fak... File device/geolocation/fake_location_provider.cc (right): https://codereview.chromium.org/2161223003/diff/320001/device/geolocation/fak... device/geolocation/fake_location_provider.cc:57: // Fake location provider that automatically calls back its client at most Is this class used anywhere in Chromium? https://codereview.chromium.org/2161223003/diff/320001/device/geolocation/fak... device/geolocation/fake_location_provider.cc:108: LocationProvider* NewFakeLocationProvider() { 1. These helper methods are very unusual. Generally speaking, picking and naming sensible constructor parameters are the way to go. If a nullable return value is desired, then a static Create method can be useful, but that doesn't appear to be the case here. 2. I don't see any callers for NewMockLocationProvider, and NewAutoSuccessMockLocationProvider in the current codebase. I recommend deleting these helper methods and replacing their callers (if they exist) with direct calls to the constructor. https://codereview.chromium.org/2161223003/diff/320001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/320001/device/geolocation/loc... device/geolocation/location_arbitrator_impl_unittest.cc:64: void SetReferencePosition(FakeLocationProvider* provider) { Cleanup todo: Populate a Geoposition in the class from kConstants and then just copy that with "=" versus using a helper function.
https://codereview.chromium.org/2161223003/diff/320001/blimp/client/BUILD.gn File blimp/client/BUILD.gn (right): https://codereview.chromium.org/2161223003/diff/320001/blimp/client/BUILD.gn#... blimp/client/BUILD.gn:113: "//device/geolocation:device_geolocation", On 2016/08/15 20:35:40, Kevin M wrote: > Hm, this target should probably be named "geolocation", which becomes the more > succinct target "//device/geolocation". Not really in your CL scope, but just > pointing this out as another point for potential cleanup. yeah I agree. Not sure why it was made like that. https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/DEPS File blimp/client/feature/DEPS (left): https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/D... blimp/client/feature/DEPS:3: "+google_apis", On 2016/08/15 20:35:40, Kevin M wrote: > Any reason why this was removed? Done. https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:45: case GeolocationMessage::kSetInterestLevel: { On 2016/08/15 20:35:40, Kevin M wrote: > No variables defined in this case block; no need to use curly braces Done. https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:106: GeolocationCoordinatesMessage* coordinates = On 2016/08/15 20:35:40, Kevin M wrote: > nit: move this to the top of the cluster of coordinates setters Done. https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.cc:142: void GeolocationFeature::OnSendComplete(int result) { On 2016/08/15 20:35:40, Kevin M wrote: > What about sending an update that was previously suppressed? > > So we'd have: > 1. message sent. > 2. update received, suppressed. > 3. update received, suppressed. > 4. message sent ACK > 5. location from #3 sent. Done. https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.h:22: // |location_provider| will be the only LocationProvider the On 2016/08/15 20:35:40, Kevin M wrote: > Nit: I think this is already implied by the fact that we don't have > post-construction setters for this type. Done. https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.h:52: const GeolocationErrorMessage::ErrorCode& error_code, On 2016/08/15 20:35:40, Kevin M wrote: > ErrorCode is a POD type, so should be passed as-is and not as a reference. POD? https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature.h:67: // True if a message is not in the process of being sent. On 2016/08/15 20:35:40, Kevin M wrote: > I think the meaning of this variable, and how it is used, would be easier to > comprehend if we inverted the semantics. For example, it could be "send_active_" > which starts out as false, becomes true when a message is being sent, then false > again once the message is delivered. Done. https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:66: auto out_processor = base::MakeUnique<MockBlimpMessageProcessor>(); On 2016/08/15 20:35:40, Kevin M wrote: > StrictMock? > > Also remove Times(1) Done. https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:166: ON_CALL(*out_processor_, MockableProcessMessage(_, _)) On 2016/08/15 20:35:40, Kevin M wrote: > Use EXPECT_CALL with a specific Times() Are you saying to make this ON_CALL and the next 3 EXPECT_CALL's into one call? https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:194: TEST_F(GeolocationFeatureTest, NoRepeatSendsWithMessagePending) { On 2016/08/15 20:35:40, Kevin M wrote: > Also verify that we can send messages again after acknowledgement. I can't think of a way to do that without a delay call, which could create a flaky test. Do you have any suggestions, or is that just the way things are done? https://codereview.chromium.org/2161223003/diff/320001/device/geolocation/fak... File device/geolocation/fake_location_provider.cc (right): https://codereview.chromium.org/2161223003/diff/320001/device/geolocation/fak... device/geolocation/fake_location_provider.cc:57: // Fake location provider that automatically calls back its client at most On 2016/08/15 20:35:40, Kevin M wrote: > Is this class used anywhere in Chromium? It is used in LocationArbitratorImplTest. https://codereview.chromium.org/2161223003/diff/320001/device/geolocation/fak... device/geolocation/fake_location_provider.cc:108: LocationProvider* NewFakeLocationProvider() { On 2016/08/15 20:35:40, Kevin M wrote: > 1. These helper methods are very unusual. Generally speaking, picking and naming > sensible constructor parameters are the way to go. If a nullable return value is > desired, then a static Create method can be useful, but that doesn't appear to > be the case here. > 2. I don't see any callers for NewMockLocationProvider, and > NewAutoSuccessMockLocationProvider in the current codebase. > > I recommend deleting these helper methods and replacing their callers (if they > exist) with direct calls to the constructor. Done. https://codereview.chromium.org/2161223003/diff/320001/device/geolocation/loc... File device/geolocation/location_arbitrator_impl_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/320001/device/geolocation/loc... device/geolocation/location_arbitrator_impl_unittest.cc:64: void SetReferencePosition(FakeLocationProvider* provider) { On 2016/08/15 20:35:40, Kevin M wrote: > Cleanup todo: > > Populate a Geoposition in the class from kConstants and then just copy that with > "=" versus using a helper function. Acknowledged.
https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:166: ON_CALL(*out_processor_, MockableProcessMessage(_, _)) On 2016/08/15 21:59:32, CJ wrote: > On 2016/08/15 20:35:40, Kevin M wrote: > > Use EXPECT_CALL with a specific Times() > > Are you saying to make this ON_CALL and the next 3 EXPECT_CALL's into one call? I'm saying don't use ON_CALL; instead, be precise about the number of times you expect the method to be called. Using EXPECT_CALL and .Times() :) https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:194: TEST_F(GeolocationFeatureTest, NoRepeatSendsWithMessagePending) { On 2016/08/15 21:59:32, CJ wrote: > On 2016/08/15 20:35:40, Kevin M wrote: > > Also verify that we can send messages again after acknowledgement. > > I can't think of a way to do that without a delay call, which could create a > flaky test. Do you have any suggestions, or is that just the way things are > done? Right, we don't like delays in our tests, but there are other ways we can do this. Use testing::Invoke<1>(net::OK) or testing::SaveArg<1> to save the callback for later running. https://codereview.chromium.org/2161223003/diff/320001/device/geolocation/fak... File device/geolocation/fake_location_provider.cc (right): https://codereview.chromium.org/2161223003/diff/320001/device/geolocation/fak... device/geolocation/fake_location_provider.cc:108: LocationProvider* NewFakeLocationProvider() { On 2016/08/15 21:59:32, CJ wrote: > On 2016/08/15 20:35:40, Kevin M wrote: > > 1. These helper methods are very unusual. Generally speaking, picking and > naming > > sensible constructor parameters are the way to go. If a nullable return value > is > > desired, then a static Create method can be useful, but that doesn't appear to > > be the case here. > > 2. I don't see any callers for NewMockLocationProvider, and > > NewAutoSuccessMockLocationProvider in the current codebase. > > > > I recommend deleting these helper methods and replacing their callers (if they > > exist) with direct calls to the constructor. > > Done. They're still here... :) https://codereview.chromium.org/2161223003/diff/340001/device/geolocation/fak... File device/geolocation/fake_location_provider.cc (right): https://codereview.chromium.org/2161223003/diff/340001/device/geolocation/fak... device/geolocation/fake_location_provider.cc:8: #include "device/geolocation/fake_location_provider.h" It looks like location_arbitrator_impl_unittest is the only consumer of FakeLocationProvider. Maybe we should convert the existing cases to use Mock instead. The code here isn't very complex. Add it to the cleanup queue?
https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... File blimp/client/feature/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:166: ON_CALL(*out_processor_, MockableProcessMessage(_, _)) On 2016/08/17 00:55:01, Kevin M wrote: > On 2016/08/15 21:59:32, CJ wrote: > > On 2016/08/15 20:35:40, Kevin M wrote: > > > Use EXPECT_CALL with a specific Times() > > > > Are you saying to make this ON_CALL and the next 3 EXPECT_CALL's into one > call? > > I'm saying don't use ON_CALL; instead, be precise about the number of times you > expect the method to be called. Using EXPECT_CALL and .Times() :) I'm using ON_CALL to specify the behavior of the next 3 EXPECT_CALL's, and since |out_processor_| is a StrictMock, this would make it be precise no? I would just have it rolled into one, but the expected arguments are different in each of the 3 calls. Would it be better than to specify Invoke behavior in each of the EXPECT_CALL's? https://codereview.chromium.org/2161223003/diff/320001/blimp/client/feature/g... blimp/client/feature/geolocation_feature_unittest.cc:194: TEST_F(GeolocationFeatureTest, NoRepeatSendsWithMessagePending) { On 2016/08/17 00:55:02, Kevin M wrote: > On 2016/08/15 21:59:32, CJ wrote: > > On 2016/08/15 20:35:40, Kevin M wrote: > > > Also verify that we can send messages again after acknowledgement. > > > > I can't think of a way to do that without a delay call, which could create a > > flaky test. Do you have any suggestions, or is that just the way things are > > done? > > Right, we don't like delays in our tests, but there are other ways we can do > this. Use testing::Invoke<1>(net::OK) or testing::SaveArg<1> to save the > callback for later running. Done. https://codereview.chromium.org/2161223003/diff/320001/device/geolocation/fak... File device/geolocation/fake_location_provider.cc (right): https://codereview.chromium.org/2161223003/diff/320001/device/geolocation/fak... device/geolocation/fake_location_provider.cc:108: LocationProvider* NewFakeLocationProvider() { On 2016/08/17 00:55:02, Kevin M wrote: > On 2016/08/15 21:59:32, CJ wrote: > > On 2016/08/15 20:35:40, Kevin M wrote: > > > 1. These helper methods are very unusual. Generally speaking, picking and > > naming > > > sensible constructor parameters are the way to go. If a nullable return > value > > is > > > desired, then a static Create method can be useful, but that doesn't appear > to > > > be the case here. > > > 2. I don't see any callers for NewMockLocationProvider, and > > > NewAutoSuccessMockLocationProvider in the current codebase. > > > > > > I recommend deleting these helper methods and replacing their callers (if > they > > > exist) with direct calls to the constructor. > > > > Done. > > They're still here... :) Still there? Are you saying you want the class AutoFakeLocationProvider also removed? https://codereview.chromium.org/2161223003/diff/340001/device/geolocation/fak... File device/geolocation/fake_location_provider.cc (right): https://codereview.chromium.org/2161223003/diff/340001/device/geolocation/fak... device/geolocation/fake_location_provider.cc:8: #include "device/geolocation/fake_location_provider.h" On 2016/08/17 00:55:02, Kevin M wrote: > It looks like location_arbitrator_impl_unittest is the only consumer of > FakeLocationProvider. Maybe we should convert the existing cases to use Mock > instead. The code here isn't very complex. Add it to the cleanup queue? Adding a cleanup TODO.
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...
khushalsagar@chromium.org changed reviewers: + nyquist@chromium.org
+nyquist, are we still adding code to feature?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/18 01:13:23, Khushal wrote: > +nyquist, are we still adding code to feature? Hi! If it is any help, I'm moving all geolocation things in it's own folder in this CL: https://codereview.chromium.org/2249283003/. I could easily reroute things in that CL to put that folder in the correct place. Please let me know!
On 2016/08/18 01:13:23, Khushal wrote: > +nyquist, are we still adding code to feature? Hi! If it is any help, I'm moving all geolocation things in it's own folder in this CL: https://codereview.chromium.org/2249283003/. I could easily reroute things in that CL to put that folder in the correct place. Please let me know!
On 2016/08/18 17:21:46, CJ wrote: > On 2016/08/18 01:13:23, Khushal wrote: > > +nyquist, are we still adding code to feature? > > Hi! If it is any help, I'm moving all geolocation things in it's own folder in > this CL: https://codereview.chromium.org/2249283003/. I could easily reroute > things in that CL to put that folder in the correct place. Please let me know! Things have now be restructures to allow for a linear dependency chain between this and the two other CL's concerning this area of work. Also, geolocation has been moved to blimp/client/core/geolocation, as requested.
Just a couple of tiny things about the BUILD.gn files https://codereview.chromium.org/2161223003/diff/420001/blimp/client/core/geol... File blimp/client/core/geolocation/BUILD.gn (right): https://codereview.chromium.org/2161223003/diff/420001/blimp/client/core/geol... blimp/client/core/geolocation/BUILD.gn:12: "//blimp/client/*", For now it looks like this could in fact be //blimp/client/core/* or even better just literally //blimp/client/core ? https://codereview.chromium.org/2161223003/diff/420001/blimp/client/core/geol... blimp/client/core/geolocation/BUILD.gn:24: "//blimp/net", This seems to be exposed in your geolocation_feature.h, so it should be public_deps. https://codereview.chromium.org/2161223003/diff/420001/blimp/client/core/geol... blimp/client/core/geolocation/BUILD.gn:27: "//device/geolocation:device_geolocation", public_deps
https://codereview.chromium.org/2161223003/diff/420001/blimp/client/core/geol... File blimp/client/core/geolocation/BUILD.gn (right): https://codereview.chromium.org/2161223003/diff/420001/blimp/client/core/geol... blimp/client/core/geolocation/BUILD.gn:12: "//blimp/client/*", On 2016/08/18 22:12:30, nyquist wrote: > For now it looks like this could in fact be //blimp/client/core/* or even better > just literally //blimp/client/core ? Done. https://codereview.chromium.org/2161223003/diff/420001/blimp/client/core/geol... blimp/client/core/geolocation/BUILD.gn:24: "//blimp/net", On 2016/08/18 22:12:30, nyquist wrote: > This seems to be exposed in your geolocation_feature.h, so it should be > public_deps. Done. https://codereview.chromium.org/2161223003/diff/420001/blimp/client/core/geol... blimp/client/core/geolocation/BUILD.gn:27: "//device/geolocation:device_geolocation", On 2016/08/18 22:12:30, nyquist wrote: > public_deps Done.
Still some cleanup to do on the test side, but the implementation is looking great at this point. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... File blimp/client/core/geolocation/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature.cc:65: const device::Geoposition& position) { nit: DCHECK_EQ(location_provider_, location_provider); https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... File blimp/client/core/geolocation/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature.h:40: // Communicates to the client a change of interest level from the engine. nit: This wording is very similar to the preceding "Sends a..." - whereas the preceding API triggers an "outgoing" Client->Engine message, this is effectively a message-handler. I'd suggest a comment like "Handles a request from the Engine to change the level of Geolocation information reported to it." or similar. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature.h:44: // geoposition and sends it to the engine. nit: "Creates ... and sends ..." is redundant - suggest just "Sends ..." https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature.h:53: // Callback function when message finishes sending. nit: Grammar; I think you mean "Called when a message send completes"? https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature.h:62: // Run when message is finished sending. Not clear from the context who is sending the message (i.e. outgoing or incoming..?) - is this just a place-holder for Bind(..., OnSendComplete)? If so then I'd suggest calling it on_send_complete_callback_. However, is there any reason to store this at all? Could we just Bind() it at the call-site and avoid the need for a member? https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature.h:66: bool sending_message_ = false; nit: Suggest am_sending_message_ In general you'll see that bools members have very verbose names that make their role very explicit, and they often start with is_, am_, etc, to make clear that they are just true/false values when we refer to them. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... File blimp/client/core/geolocation/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature_unittest.cc:86: void GetPosition(device::Geoposition* position) { *position = position_; } What is this for? https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature_unittest.cc:105: const net::CompletionCallback& callback) { nit: Suggest calling this ReportProcessMessageSuccess https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature_unittest.cc:110: const net::CompletionCallback& callback) { This doesn't seem to be used anywhere? https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature_unittest.cc:115: callback.Run(net::OK); Nor does this? https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... File device/geolocation/fake_location_provider.cc (left): https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... device/geolocation/fake_location_provider.cc:112: LocationProvider* NewMockLocationProvider() { Looks like you've removed these from the .cc but renamed them in the .h? https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... File device/geolocation/fake_location_provider.cc (right): https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... device/geolocation/fake_location_provider.cc:66: class AutoFakeLocationProvider : public FakeLocationProvider { If you've removed the New*() methods below, surely there is no way to create this class any more? https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... File device/geolocation/fake_location_provider.h (right): https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... device/geolocation/fake_location_provider.h:29: bool IsProviderStarted() const; nit: See comments in other CL re making this a simple-getter, and hiding the member variables to be private. https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... device/geolocation/fake_location_provider.h:38: Geoposition position_; nit: These PoD members can be in-line initialized. https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... device/geolocation/fake_location_provider.h:61: LocationProvider* NewAutoSuccessFakeNetworkLocationProvider(); Do any call-sites actually use these? I'd assume not, since you haven't had to update anywhere - so can we just remove them? https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/moc... File device/geolocation/mock_location_provider.h (right): https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/moc... device/geolocation/mock_location_provider.h:24: MOCK_METHOD0(RequestRefresh, void()); Didn't we remove RequestRefresh from the LocationProvider API?
https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... File blimp/client/core/geolocation/BUILD.gn (right): https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/BUILD.gn:55: "//device/geolocation:unittests", Remove this entry. Including other unit test groups clutters up the dependency graph. They'll be run on the waterfall w/o us needing to include them. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... File blimp/client/core/geolocation/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature_unittest.cc:22: using testing::Assign; not used https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature_unittest.cc:25: using testing::Return; not used https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature_unittest.cc:181: ON_CALL(*out_processor_, MockableProcessMessage(_, _)) Wow, this works. Apparently EXPECT_CALL and ON_CALL can be used together - EXPECT_CALL specifies which calls are made, and ON_CALL specifies the actions.
https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... File blimp/client/core/geolocation/geolocation_feature.cc (right): https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature.cc:65: const device::Geoposition& position) { On 2016/08/19 18:50:25, Wez wrote: > nit: DCHECK_EQ(location_provider_, location_provider); Done. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... File blimp/client/core/geolocation/geolocation_feature.h (right): https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature.h:40: // Communicates to the client a change of interest level from the engine. On 2016/08/19 18:50:25, Wez wrote: > nit: This wording is very similar to the preceding "Sends a..." - whereas the > preceding API triggers an "outgoing" Client->Engine message, this is effectively > a message-handler. > > I'd suggest a comment like "Handles a request from the Engine to change the > level of Geolocation information reported to it." or similar. Done. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature.h:44: // geoposition and sends it to the engine. On 2016/08/19 18:50:25, Wez wrote: > nit: "Creates ... and sends ..." is redundant - suggest just "Sends ..." Done. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature.h:53: // Callback function when message finishes sending. On 2016/08/19 18:50:25, Wez wrote: > nit: Grammar; I think you mean "Called when a message send completes"? Done. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature.h:62: // Run when message is finished sending. On 2016/08/19 18:50:25, Wez wrote: > Not clear from the context who is sending the message (i.e. outgoing or > incoming..?) - is this just a place-holder for Bind(..., OnSendComplete)? If so > then I'd suggest calling it on_send_complete_callback_. > > However, is there any reason to store this at all? Could we just Bind() it at > the call-site and avoid the need for a member? Good point. Not stored now. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature.h:66: bool sending_message_ = false; On 2016/08/19 18:50:25, Wez wrote: > nit: Suggest am_sending_message_ > > In general you'll see that bools members have very verbose names that make their > role very explicit, and they often start with is_, am_, etc, to make clear that > they are just true/false values when we refer to them. Cool thanks, fixed. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... File blimp/client/core/geolocation/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature_unittest.cc:86: void GetPosition(device::Geoposition* position) { *position = position_; } On 2016/08/19 18:50:25, Wez wrote: > What is this for? Seems like poor merging to me. Removed. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature_unittest.cc:105: const net::CompletionCallback& callback) { On 2016/08/19 18:50:25, Wez wrote: > nit: Suggest calling this ReportProcessMessageSuccess Done. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature_unittest.cc:110: const net::CompletionCallback& callback) { On 2016/08/19 18:50:25, Wez wrote: > This doesn't seem to be used anywhere? Gone. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature_unittest.cc:115: callback.Run(net::OK); On 2016/08/19 18:50:25, Wez wrote: > Nor does this? Gone. https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... File device/geolocation/fake_location_provider.cc (left): https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... device/geolocation/fake_location_provider.cc:112: LocationProvider* NewMockLocationProvider() { On 2016/08/19 18:50:25, Wez wrote: > Looks like you've removed these from the .cc but renamed them in the .h? Ah. Sorry, another merge failure on my part. Gone. https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... File device/geolocation/fake_location_provider.cc (right): https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... device/geolocation/fake_location_provider.cc:66: class AutoFakeLocationProvider : public FakeLocationProvider { On 2016/08/19 18:50:25, Wez wrote: > If you've removed the New*() methods below, surely there is no way to create > this class any more? I dont think this class is used anywhere. I'm removing it. https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... File device/geolocation/fake_location_provider.h (right): https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... device/geolocation/fake_location_provider.h:29: bool IsProviderStarted() const; On 2016/08/19 18:50:25, Wez wrote: > nit: See comments in other CL re making this a simple-getter, and hiding the > member variables to be private. Done. Let me know if this is what was intended. https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... device/geolocation/fake_location_provider.h:38: Geoposition position_; On 2016/08/19 18:50:26, Wez wrote: > nit: These PoD members can be in-line initialized. Done. https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... device/geolocation/fake_location_provider.h:61: LocationProvider* NewAutoSuccessFakeNetworkLocationProvider(); On 2016/08/19 18:50:25, Wez wrote: > Do any call-sites actually use these? I'd assume not, since you haven't had to > update anywhere - so can we just remove them? Yeah they are gone now. https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/moc... File device/geolocation/mock_location_provider.h (right): https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/moc... device/geolocation/mock_location_provider.h:24: MOCK_METHOD0(RequestRefresh, void()); On 2016/08/19 18:50:26, Wez wrote: > Didn't we remove RequestRefresh from the LocationProvider API? I think this is more of me learning how to merge right. It's gone now.
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...) 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_clobber_rel_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_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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...
lethalantidote@chromium.org changed reviewers: + rogerta@chromium.org
+ rogerta@ for '+google_apis' in blimp/client/core/geolocaton/DEPS https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... File blimp/client/core/geolocation/BUILD.gn (right): https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/BUILD.gn:55: "//device/geolocation:unittests", On 2016/08/19 22:16:26, Kevin M wrote: > Remove this entry. > > Including other unit test groups clutters up the dependency graph. They'll be > run on the waterfall w/o us needing to include them. It's needed so that device/geolocation/mock_location_provider can be accessible. However, on that note, its probably best if mock_location_provider and the like get their own test_support build rule and I'll link that in instead. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... File blimp/client/core/geolocation/geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature_unittest.cc:22: using testing::Assign; On 2016/08/19 22:16:26, Kevin M wrote: > not used Done. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature_unittest.cc:25: using testing::Return; On 2016/08/19 22:16:26, Kevin M wrote: > not used Done. https://codereview.chromium.org/2161223003/diff/440001/blimp/client/core/geol... blimp/client/core/geolocation/geolocation_feature_unittest.cc:181: ON_CALL(*out_processor_, MockableProcessMessage(_, _)) On 2016/08/19 22:16:26, Kevin M wrote: > Wow, this works. Apparently EXPECT_CALL and ON_CALL can be used together - > EXPECT_CALL specifies which calls are made, and ON_CALL specifies the actions. Acknowledged.
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...) 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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... File device/geolocation/fake_location_provider.cc (right): https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... device/geolocation/fake_location_provider.cc:66: class AutoFakeLocationProvider : public FakeLocationProvider { On 2016/08/19 22:22:34, CJ wrote: > On 2016/08/19 18:50:25, Wez wrote: > > If you've removed the New*() methods below, surely there is no way to create > > this class any more? > > I dont think this class is used anywhere. I'm removing it. My point was that it _can't_ be used from anywhere - it's declared in this .cc file, so no other code could possibly refer to it, and you've removed the uses of it in the .cc file itself already - so you can be 100% sure it's dead code :) https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/moc... File device/geolocation/mock_location_provider.h (right): https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/moc... device/geolocation/mock_location_provider.h:24: MOCK_METHOD0(RequestRefresh, void()); On 2016/08/19 22:22:34, CJ wrote: > On 2016/08/19 18:50:26, Wez wrote: > > Didn't we remove RequestRefresh from the LocationProvider API? > > I think this is more of me learning how to merge right. It's gone now. Acknowledged. https://codereview.chromium.org/2161223003/diff/480001/device/geolocation/fak... File device/geolocation/fake_location_provider.h (right): https://codereview.chromium.org/2161223003/diff/480001/device/geolocation/fak... device/geolocation/fake_location_provider.h:29: bool get_state() const { return state_; } |state_| isn't a bool; do you mean: State get_state() const or bool is_started() const ? https://codereview.chromium.org/2161223003/diff/480001/device/geolocation/fak... device/geolocation/fake_location_provider.h:30: bool get_is_permission_granted() const { return is_permission_granted_; } Simple getters and setters are named along the lines of: Foo foo() const void set_foo(Foo) i.e. no get_ on the getter https://codereview.chromium.org/2161223003/diff/480001/device/geolocation/fak... device/geolocation/fake_location_provider.h:31: const Geoposition& get_position() const { return position_; } Isn't this exactly what GetPosition does? https://codereview.chromium.org/2161223003/diff/480001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/480001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:148: *started = arbitrator()->get_state() != FakeLocationProvider::STOPPED; You've defined get_state() as returning a bool, not a State, I think?
lgtm for '+google_apis' in blimp/client/core/geolocaton/DEPS
https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... File device/geolocation/fake_location_provider.cc (right): https://codereview.chromium.org/2161223003/diff/440001/device/geolocation/fak... device/geolocation/fake_location_provider.cc:66: class AutoFakeLocationProvider : public FakeLocationProvider { On 2016/08/20 01:42:11, Wez wrote: > On 2016/08/19 22:22:34, CJ wrote: > > On 2016/08/19 18:50:25, Wez wrote: > > > If you've removed the New*() methods below, surely there is no way to create > > > this class any more? > > > > I dont think this class is used anywhere. I'm removing it. > > My point was that it _can't_ be used from anywhere - it's declared in this .cc > file, so no other code could possibly refer to it, and you've removed the uses > of it in the .cc file itself already - so you can be 100% sure it's dead code :) No I understood. Sorry poor phrasing by me I guess. https://codereview.chromium.org/2161223003/diff/480001/device/geolocation/fak... File device/geolocation/fake_location_provider.h (right): https://codereview.chromium.org/2161223003/diff/480001/device/geolocation/fak... device/geolocation/fake_location_provider.h:29: bool get_state() const { return state_; } On 2016/08/20 01:42:11, Wez wrote: > |state_| isn't a bool; do you mean: > > State get_state() const > > or > > bool is_started() const > > ? State state(). Done. https://codereview.chromium.org/2161223003/diff/480001/device/geolocation/fak... device/geolocation/fake_location_provider.h:30: bool get_is_permission_granted() const { return is_permission_granted_; } On 2016/08/20 01:42:11, Wez wrote: > Simple getters and setters are named along the lines of: > > Foo foo() const > void set_foo(Foo) > > i.e. no get_ on the getter Done. https://codereview.chromium.org/2161223003/diff/480001/device/geolocation/fak... device/geolocation/fake_location_provider.h:31: const Geoposition& get_position() const { return position_; } On 2016/08/20 01:42:11, Wez wrote: > Isn't this exactly what GetPosition does? Good point. https://codereview.chromium.org/2161223003/diff/480001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/480001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:148: *started = arbitrator()->get_state() != FakeLocationProvider::STOPPED; On 2016/08/20 01:42:11, Wez wrote: > You've defined get_state() as returning a bool, not a State, I think? 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...
lgtm https://codereview.chromium.org/2161223003/diff/500001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/500001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:119: void GetProvidersStarted(bool* started); ! Please clean this up to return a bool instead of taking a bool ptr. O_o
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...)
https://codereview.chromium.org/2161223003/diff/500001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/500001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:119: void GetProvidersStarted(bool* started); On 2016/08/22 19:54:26, Kevin M wrote: > ! Please clean this up to return a bool instead of taking a bool ptr. O_o It seems like it has to be this way due to this funky business with ProvidersStarted(). I tried pulling out all the thread related stuff in that function to see if it is feasible and it looks like it is necessary. If you have any ideas on how to do this in a less gross way, I wanna learn.
https://codereview.chromium.org/2161223003/diff/500001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/500001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:119: void GetProvidersStarted(bool* started); On 2016/08/22 20:40:48, CJ wrote: > On 2016/08/22 19:54:26, Kevin M wrote: > > ! Please clean this up to return a bool instead of taking a bool ptr. O_o > > It seems like it has to be this way due to this funky business with > ProvidersStarted(). I tried pulling out all the thread related stuff in that > function to see if it is feasible and it looks like it is necessary. If you have > any ideas on how to do this in a less gross way, I wanna learn. This is just the Bad Old Way of doing things, from when we just had PostTaskAndReply. With PostTaskAndReplyWithResult you can invoke a base::Callback on the target thread and have the result posted back to your reply callback as the first parameter now.
On 2016/08/22 20:47:55, Wez wrote: > https://codereview.chromium.org/2161223003/diff/500001/device/geolocation/geo... > File device/geolocation/geolocation_provider_impl_unittest.cc (right): > > https://codereview.chromium.org/2161223003/diff/500001/device/geolocation/geo... > device/geolocation/geolocation_provider_impl_unittest.cc:119: void > GetProvidersStarted(bool* started); > On 2016/08/22 20:40:48, CJ wrote: > > On 2016/08/22 19:54:26, Kevin M wrote: > > > ! Please clean this up to return a bool instead of taking a bool ptr. O_o > > > > It seems like it has to be this way due to this funky business with > > ProvidersStarted(). I tried pulling out all the thread related stuff in that > > function to see if it is feasible and it looks like it is necessary. If you > have > > any ideas on how to do this in a less gross way, I wanna learn. > > This is just the Bad Old Way of doing things, from when we just had > PostTaskAndReply. With PostTaskAndReplyWithResult you can invoke a > base::Callback on the target thread and have the result posted back to your > reply callback as the first parameter now. Done. Wez and I talked offline about this, and found that PostTaskAndReplyWithResults may be sort of a neutral change (the callback would just be storing the value to return it), but he did point out that using a member variable for |started| does address the bool pointer issue.
LGTM w/ nits. https://codereview.chromium.org/2161223003/diff/520001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/520001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:132: bool started_; nit: Prefer is_started_ in cases like this. https://codereview.chromium.org/2161223003/diff/520001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:175: base::Bind(&DummyFunction); No need for this line; just pass &DummyFunction directly in place of |callback| below.
https://codereview.chromium.org/2161223003/diff/520001/device/geolocation/geo... File device/geolocation/geolocation_provider_impl_unittest.cc (right): https://codereview.chromium.org/2161223003/diff/520001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:132: bool started_; On 2016/08/24 04:25:50, Wez wrote: > nit: Prefer is_started_ in cases like this. Done. https://codereview.chromium.org/2161223003/diff/520001/device/geolocation/geo... device/geolocation/geolocation_provider_impl_unittest.cc:175: base::Bind(&DummyFunction); On 2016/08/24 04:25:50, Wez wrote: > No need for this line; just pass &DummyFunction directly in place of |callback| > below. Done.
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org, rogerta@chromium.org, kmarshall@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2161223003/#ps560001 (title: "Merge branch 'master' into client_feature")
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 mcasas@chromium.org, rogerta@chromium.org, kmarshall@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2161223003/#ps600001 (title: "More merging.")
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 ========== Adds GeolocationFeature for Blimp Geolocation project. This change provides the client-side classes needed to receive and send geolocation messages. GeolocationFeature sends messages from the client that either contain a geolocation coordinate or error message. The GeolocationFeature class handles incoming GeoloationSetInterestLevelMessages and RequestRefreshMessages. Other items included: * Changes device/geolocation/mock_location_provider to use fake_location_provider name * Moves blimp/client/feature/mock_location_provider into device/geolocation. BUG=614486 ========== to ========== Adds GeolocationFeature for Blimp Geolocation project. This change provides the client-side classes needed to receive and send geolocation messages. GeolocationFeature sends messages from the client that either contain a geolocation coordinate or error message. The GeolocationFeature class handles incoming GeoloationSetInterestLevelMessages and RequestRefreshMessages. Other items included: * Changes device/geolocation/mock_location_provider to use fake_location_provider name * Moves blimp/client/feature/mock_location_provider into device/geolocation. BUG=614486 ==========
Message was sent while issue was closed.
Committed patchset #31 (id:600001)
Message was sent while issue was closed.
Description was changed from ========== Adds GeolocationFeature for Blimp Geolocation project. This change provides the client-side classes needed to receive and send geolocation messages. GeolocationFeature sends messages from the client that either contain a geolocation coordinate or error message. The GeolocationFeature class handles incoming GeoloationSetInterestLevelMessages and RequestRefreshMessages. Other items included: * Changes device/geolocation/mock_location_provider to use fake_location_provider name * Moves blimp/client/feature/mock_location_provider into device/geolocation. BUG=614486 ========== to ========== Adds GeolocationFeature for Blimp Geolocation project. This change provides the client-side classes needed to receive and send geolocation messages. GeolocationFeature sends messages from the client that either contain a geolocation coordinate or error message. The GeolocationFeature class handles incoming GeoloationSetInterestLevelMessages and RequestRefreshMessages. Other items included: * Changes device/geolocation/mock_location_provider to use fake_location_provider name * Moves blimp/client/feature/mock_location_provider into device/geolocation. BUG=614486 Committed: https://crrev.com/7a376a1a71743b1b50c6fb9c206ffbb0ca8c8ba2 Cr-Commit-Position: refs/heads/master@{#414797} ==========
Message was sent while issue was closed.
Patchset 31 (id:??) landed as https://crrev.com/7a376a1a71743b1b50c6fb9c206ffbb0ca8c8ba2 Cr-Commit-Position: refs/heads/master@{#414797} |
