Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2091023006:
Adds EngineGeolocationFeature for Blimp Geolocation project. (Closed)
|
Created:
4 years, 6 months ago by CJ Modified:
4 years, 4 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+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, Garrett Casto Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds EngineGeolocationFeature for Blimp Geolocation project.
This change provides the engine-side classes needed to receive and send
geolocation messages from the client. EngineGeolocationFeature sends
messages from the engine that can either indicate the client-side to start
sending location messages of the specified accuracy, request a refresh of
the location, or indicate that the engine side no longer is listening for
updates. The EngineGeolocationFeature also receives messages from the
client, which can either provide a location or indicate an error.
Other changes:
*Moves EmptyMessage out of tab_control.proto to common.proto
BUG=614486
Committed: https://crrev.com/695c49437b5eb038e09e08c805f26ce7f7da253a
Cr-Commit-Position: refs/heads/master@{#407934}
Patch Set 1 #
Total comments: 52
Patch Set 2 : Addresses kmarshall's #3 comments #
Total comments: 10
Patch Set 3 : Addresses kmarshall's #10 #
Total comments: 26
Patch Set 4 : Addresses kmarshall's #12 comments and unittests #Patch Set 5 : Updating repo to see if try is fixed #
Total comments: 87
Patch Set 6 : Wasn't running all tests. Should all be fixed now. #Patch Set 7 : In response to Wez's #23 comment #
Total comments: 18
Patch Set 8 : Addresses Wez's #29 comments #
Total comments: 33
Patch Set 9 : Addresses Wez's #31 comments and adds BlimpLocationProvider unittests #Patch Set 10 : Try and Lint errors fix #
Total comments: 16
Patch Set 11 : Merge with head #
Total comments: 26
Patch Set 12 : Addresses Wez's #44 comments #Patch Set 13 : Addresses kmarshall's #47 comments #Patch Set 14 : Remove DLOG(FATAL)-related tests #
Total comments: 2
Patch Set 15 : Comment update #
Total comments: 16
Patch Set 16 : Addresses kmarshalls' #59 comments #
Total comments: 2
Patch Set 17 : Addresses Wez's #61 comments #
Total comments: 5
Patch Set 18 : Addresses Wez's #63 comments #Patch Set 19 : Merge from Master #
Total comments: 4
Patch Set 20 : Merge #
Total comments: 4
Patch Set 21 : Addresses Wez's #77 comments #Patch Set 22 : Merge #Dependent Patchsets: Messages
Total messages: 99 (58 generated)
lethalantidote@chromium.org changed reviewers: + kmarshall@chromium.org, wez@chromium.org
Hello, Here is the preliminary CL for EngineGeolocationFeature. To be noted, unit tests are on their way, but I thought I'd start the conversation over this now, just to make sure the approach seems sound. Thanks, CJ
https://codereview.chromium.org/2091023006/diff/1/blimp/common/logging.cc File blimp/common/logging.cc (right): https://codereview.chromium.org/2091023006/diff/1/blimp/common/logging.cc#new... blimp/common/logging.cc:308: AddField("subtype", "UPDATE_LISTEN_STATE_MESSAGE", output); Remove _MESSAGE suffix https://codereview.chromium.org/2091023006/diff/1/blimp/common/proto/geolocat... File blimp/common/proto/geolocation.proto (right): https://codereview.chromium.org/2091023006/diff/1/blimp/common/proto/geolocat... blimp/common/proto/geolocation.proto:11: import "tab_control.proto"; Promote EmptyMessage to a "common.proto" file - this dependency is confusing otherwise. https://codereview.chromium.org/2091023006/diff/1/blimp/common/proto/geolocat... blimp/common/proto/geolocation.proto:16: // These values follow the W3C geolocation specification and can be returned Add a link to the specification? https://codereview.chromium.org/2091023006/diff/1/blimp/common/proto/geolocat... blimp/common/proto/geolocation.proto:32: // Please refer to content/public/common/geoposition.h for explanation of nit: "please" is optional - one-liner comments are generally more declarative than polite https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:28: (enable_high_accuracy) Use conventional if/else syntax. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:62: DCHECK(delegate); Also check !delegate_ ? https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:71: DCHECK(message->feature_case() == BlimpMessage::kGeolocation); DCHECK_EQ(expected, actual) https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:74: GeolocationMessage geolocation_message = message->geolocation(); Use "const GeolocationMessage&" instead, so that you don't make a copy here. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:75: content::Geoposition geoposition; "output" ? https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:81: geoposition.latitude = location.latitude(); Put this in an anonymous namespace method. Too much disjoint LoC in a switch statement makes code hard to decipher. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:89: base::Time::FromJsTime(location.timestamp_millis()); DCHECK if geoposition is valid (using Geoposition::Validate()) https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:95: ErrorMessage error_message = geolocation_message.error(); Store this as a const reference or just refer to the error() field inline https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:96: switch (error_message.error_code()) { Put this in an anonymous namespace method for mapping error codes https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:118: default: Handle TYPE_NOT_SET instead of using "default" so that the enum coverage checker will work https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:123: void EngineGeolocationFeature::UpdateGeolocation( This is just a one-liner method whose implementation body is pretty trivial; recommend just calling the delegate directly instead of going through a helper function. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:124: content::Geoposition* geoposition) { Take a const reference instead of a pointer here. Pointers are only used for read/write objects or in cases where the pointer is meant to be held beyond the lifetime of the function call. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... File blimp/engine/feature/geolocation/engine_geolocation_feature.h (right): https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:15: // geolocation. Delegates can be added to be notified of incoming Can you rephrase the last sentence to use the active voice instead of the passive voice? https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:20: class GeolocationMessageDelegate { "Delegate" is fine here, since all references to this class are unambiguous anyways ("EngineGeolocationFeature::Delegate") https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:25: virtual void OnLocationResponse(const content::Geoposition& position); Make this pure virtual? https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:30: void set_geolocation_message_sender( nit: I think we've been calling it "outgoing_message_processor"? Not a big fan of that though, since the "sender" terminology has had more traction... (seems like a job for a fixit CL) Regardless, let's use feature-agnostic terminology for these types of common operations. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:35: void SendUpdateListenStateMessage(bool enable_high_accuracy); Who's supposed to call these methods? Does it make sense to have them be public? https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:37: // Notifies the client that the engine is no long listening for updates. typo: longer https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:40: // Notifies the client that an update in geoposition is needed as soon suggestion: Requests an updated geoposition from the client. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:51: GeolocationMessageDelegate* delegate_; = nullptr;
Description was changed from ========== Adds EngineGeolocationFeature for Blimp Geolocation project. This change provides the engine-side classes needed to receive and send geolocation messages from the client. EngineGeolocationFeature sends messages from the engine that can either indicate the client-side to start sending location messages of the specified accuracy, request a refresh of the location, or indicate that the engine side no longer is listening for updates. The EngineGeolocationFeature also receives messages from the client, which can either provide a location or indicate an error. BUG=614486 ========== to ========== Adds EngineGeolocationFeature for Blimp Geolocation project. This change provides the engine-side classes needed to receive and send geolocation messages from the client. EngineGeolocationFeature sends messages from the engine that can either indicate the client-side to start sending location messages of the specified accuracy, request a refresh of the location, or indicate that the engine side no longer is listening for updates. The EngineGeolocationFeature also receives messages from the client, which can either provide a location or indicate an error. Other changes: *Moves EmptyMessage out of tab_control.proto to common.proto BUG=614486 ==========
https://codereview.chromium.org/2091023006/diff/1/blimp/common/logging.cc File blimp/common/logging.cc (right): https://codereview.chromium.org/2091023006/diff/1/blimp/common/logging.cc#new... blimp/common/logging.cc:308: AddField("subtype", "UPDATE_LISTEN_STATE_MESSAGE", output); On 2016/06/27 16:57:40, Kevin M wrote: > Remove _MESSAGE suffix Done. https://codereview.chromium.org/2091023006/diff/1/blimp/common/proto/geolocat... File blimp/common/proto/geolocation.proto (right): https://codereview.chromium.org/2091023006/diff/1/blimp/common/proto/geolocat... blimp/common/proto/geolocation.proto:11: import "tab_control.proto"; On 2016/06/27 16:57:41, Kevin M wrote: > Promote EmptyMessage to a "common.proto" file - this dependency is confusing > otherwise. Good idea. Done. https://codereview.chromium.org/2091023006/diff/1/blimp/common/proto/geolocat... blimp/common/proto/geolocation.proto:16: // These values follow the W3C geolocation specification and can be returned On 2016/06/27 16:57:41, Kevin M wrote: > Add a link to the specification? Done. https://codereview.chromium.org/2091023006/diff/1/blimp/common/proto/geolocat... blimp/common/proto/geolocation.proto:32: // Please refer to content/public/common/geoposition.h for explanation of On 2016/06/27 16:57:41, Kevin M wrote: > nit: "please" is optional - one-liner comments are generally more declarative > than polite Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:28: (enable_high_accuracy) On 2016/06/27 16:57:41, Kevin M wrote: > Use conventional if/else syntax. Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:62: DCHECK(delegate); On 2016/06/27 16:57:41, Kevin M wrote: > Also check !delegate_ ? Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:71: DCHECK(message->feature_case() == BlimpMessage::kGeolocation); On 2016/06/27 16:57:41, Kevin M wrote: > DCHECK_EQ(expected, actual) Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:74: GeolocationMessage geolocation_message = message->geolocation(); On 2016/06/27 16:57:41, Kevin M wrote: > Use "const GeolocationMessage&" instead, so that you don't make a copy here. Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:75: content::Geoposition geoposition; On 2016/06/27 16:57:41, Kevin M wrote: > "output" ? Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:81: geoposition.latitude = location.latitude(); On 2016/06/27 16:57:41, Kevin M wrote: > Put this in an anonymous namespace method. Too much disjoint LoC in a switch > statement makes code hard to decipher. Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:89: base::Time::FromJsTime(location.timestamp_millis()); On 2016/06/27 16:57:41, Kevin M wrote: > DCHECK if geoposition is valid (using Geoposition::Validate()) This already gets handled upstream by the LocationArbitratorImpl https://cs.chromium.org/chromium/src/content/browser/geolocation/location_arb... . Perhaps it should still be handled there? https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:95: ErrorMessage error_message = geolocation_message.error(); On 2016/06/27 16:57:41, Kevin M wrote: > Store this as a const reference or just refer to the error() field inline Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:96: switch (error_message.error_code()) { On 2016/06/27 16:57:41, Kevin M wrote: > Put this in an anonymous namespace method for mapping error codes Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:118: default: On 2016/06/27 16:57:41, Kevin M wrote: > Handle TYPE_NOT_SET instead of using "default" so that the enum coverage checker > will work Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:123: void EngineGeolocationFeature::UpdateGeolocation( On 2016/06/27 16:57:41, Kevin M wrote: > This is just a one-liner method whose implementation body is pretty trivial; > recommend just calling the delegate directly instead of going through a helper > function. Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:124: content::Geoposition* geoposition) { On 2016/06/27 16:57:41, Kevin M wrote: > Take a const reference instead of a pointer here. Pointers are only used for > read/write objects or in cases where the pointer is meant to be held beyond the > lifetime of the function call. Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... File blimp/engine/feature/geolocation/engine_geolocation_feature.h (right): https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:15: // geolocation. Delegates can be added to be notified of incoming On 2016/06/27 16:57:41, Kevin M wrote: > Can you rephrase the last sentence to use the active voice instead of the > passive voice? Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:20: class GeolocationMessageDelegate { On 2016/06/27 16:57:42, Kevin M wrote: > "Delegate" is fine here, since all references to this class are unambiguous > anyways ("EngineGeolocationFeature::Delegate") Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:25: virtual void OnLocationResponse(const content::Geoposition& position); On 2016/06/27 16:57:41, Kevin M wrote: > Make this pure virtual? Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:30: void set_geolocation_message_sender( On 2016/06/27 16:57:41, Kevin M wrote: > nit: I think we've been calling it "outgoing_message_processor"? Not a big fan > of that though, since the "sender" terminology has had more traction... (seems > like a job for a fixit CL) > > Regardless, let's use feature-agnostic terminology for these types of common > operations. I was following the pattern found here: https://cs.chromium.org/chromium/src/blimp/engine/feature/engine_render_widge... I havent really seen the outgoing_message_processor variable. I'm happy either way. Let me know what you think. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:35: void SendUpdateListenStateMessage(bool enable_high_accuracy); On 2016/06/27 16:57:42, Kevin M wrote: > Who's supposed to call these methods? Does it make sense to have them be public? BlimpLocationProvider would call this through it's RequestRefresh() function. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:37: // Notifies the client that the engine is no long listening for updates. On 2016/06/27 16:57:41, Kevin M wrote: > typo: longer Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:40: // Notifies the client that an update in geoposition is needed as soon On 2016/06/27 16:57:41, Kevin M wrote: > suggestion: Requests an updated geoposition from the client. Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:51: GeolocationMessageDelegate* delegate_; On 2016/06/27 16:57:41, Kevin M wrote: > = nullptr; 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: This issue passed the CQ dry run.
Getting close. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... File blimp/engine/feature/geolocation/engine_geolocation_feature.h (right): https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:30: void set_geolocation_message_sender( On 2016/06/27 22:04:27, CJ wrote: > On 2016/06/27 16:57:41, Kevin M wrote: > > nit: I think we've been calling it "outgoing_message_processor"? Not a big fan > > of that though, since the "sender" terminology has had more traction... (seems > > like a job for a fixit CL) > > > > Regardless, let's use feature-agnostic terminology for these types of common > > operations. > > I was following the pattern found here: > https://cs.chromium.org/chromium/src/blimp/engine/feature/engine_render_widge... > > I havent really seen the outgoing_message_processor variable. > > I'm happy either way. Let me know what you think. Yeah, it needs fixin'. Use outgoing_message_processor for now and we'll address it later. Maybe file a bug to standardize the variable & getter names? https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:35: void SendUpdateListenStateMessage(bool enable_high_accuracy); On 2016/06/27 22:04:27, CJ wrote: > On 2016/06/27 16:57:42, Kevin M wrote: > > Who's supposed to call these methods? Does it make sense to have them be > public? > > BlimpLocationProvider would call this through it's RequestRefresh() function. I think that we should probably invert that relationship. Features are clients of platform code, not the other way around. So, BlimpLocationProvider calls BlimpLocationProvider::Delegate methods, of which EngineGeolocationFeature is an impl class. https://codereview.chromium.org/2091023006/diff/20001/blimp/common/proto/geol... File blimp/common/proto/geolocation.proto (right): https://codereview.chromium.org/2091023006/diff/20001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:46: enum ListenState { Comment on this enum? https://codereview.chromium.org/2091023006/diff/20001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:15: void GetErrorMessageDetail(content::Geoposition* output, What about just returning the error code int instead of writing to an output parameter? https://codereview.chromium.org/2091023006/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:34: void GetLocationMessageDetail(content::Geoposition* output, ConvertLocationMessage? https://codereview.chromium.org/2091023006/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:131: case GeolocationMessage::kUpdateListenState: These are going to be implemented at some point, right? If so, write TODOs for them and put in a NOTIMPLEMENTED(). https://codereview.chromium.org/2091023006/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:134: NOTREACHED(); break;
https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... File blimp/engine/feature/geolocation/engine_geolocation_feature.h (right): https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:30: void set_geolocation_message_sender( On 2016/06/28 01:14:38, Kevin M wrote: > On 2016/06/27 22:04:27, CJ wrote: > > On 2016/06/27 16:57:41, Kevin M wrote: > > > nit: I think we've been calling it "outgoing_message_processor"? Not a big > fan > > > of that though, since the "sender" terminology has had more traction... > (seems > > > like a job for a fixit CL) > > > > > > Regardless, let's use feature-agnostic terminology for these types of common > > > operations. > > > > I was following the pattern found here: > > > https://cs.chromium.org/chromium/src/blimp/engine/feature/engine_render_widge... > > > > I havent really seen the outgoing_message_processor variable. > > > > I'm happy either way. Let me know what you think. > > Yeah, it needs fixin'. Use outgoing_message_processor for now and we'll address > it later. Maybe file a bug to standardize the variable & getter names? Done. https://codereview.chromium.org/2091023006/diff/1/blimp/engine/feature/geoloc... blimp/engine/feature/geolocation/engine_geolocation_feature.h:35: void SendUpdateListenStateMessage(bool enable_high_accuracy); On 2016/06/28 01:14:38, Kevin M wrote: > On 2016/06/27 22:04:27, CJ wrote: > > On 2016/06/27 16:57:42, Kevin M wrote: > > > Who's supposed to call these methods? Does it make sense to have them be > > public? > > > > BlimpLocationProvider would call this through it's RequestRefresh() function. > > I think that we should probably invert that relationship. Features are clients > of platform code, not the other way around. > > So, BlimpLocationProvider calls BlimpLocationProvider::Delegate methods, of > which EngineGeolocationFeature is an impl class. Done. https://codereview.chromium.org/2091023006/diff/20001/blimp/common/proto/geol... File blimp/common/proto/geolocation.proto (right): https://codereview.chromium.org/2091023006/diff/20001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:46: enum ListenState { On 2016/06/28 01:14:38, Kevin M wrote: > Comment on this enum? Done. https://codereview.chromium.org/2091023006/diff/20001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:15: void GetErrorMessageDetail(content::Geoposition* output, On 2016/06/28 01:14:38, Kevin M wrote: > What about just returning the error code int instead of writing to an output > parameter? Done. https://codereview.chromium.org/2091023006/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:34: void GetLocationMessageDetail(content::Geoposition* output, On 2016/06/28 01:14:38, Kevin M wrote: > ConvertLocationMessage? Done. https://codereview.chromium.org/2091023006/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:131: case GeolocationMessage::kUpdateListenState: On 2016/06/28 01:14:38, Kevin M wrote: > These are going to be implemented at some point, right? If so, write TODOs for > them and put in a NOTIMPLEMENTED(). No? These are error cases. I've added error handling similar to that of navigation_feature. https://codereview.chromium.org/2091023006/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:134: NOTREACHED(); On 2016/06/28 01:14:38, Kevin M wrote: > break; Done.
I recommend adding unit tests to the next patch, I'm seeing functional errors here that would be caught by some unit testing. https://codereview.chromium.org/2091023006/diff/40001/blimp/common/create_bli... File blimp/common/create_blimp_message.cc (right): https://codereview.chromium.org/2091023006/diff/40001/blimp/common/create_bli... blimp/common/create_blimp_message.cc:93: std::unique_ptr<BlimpMessage> output(new BlimpMessage); You can use the new function in ptr_util.h: std::make_unique<BlimpMessage>() https://codereview.chromium.org/2091023006/diff/40001/blimp/common/proto/geol... File blimp/common/proto/geolocation.proto (right): https://codereview.chromium.org/2091023006/diff/40001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:25: ERROR_CODE_LAST = 3; No need to define this - the pb compiler defines a validity check function that we can use. allow_alias can be removed too. https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:46: NotifyCallback(position); Pick one: NOTIMPLEMENTED() with nothing else, or add an implementation. ;) https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:53: DCHECK(!delegate); Not sure if you intended to do this. ;) https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:18: class BlimpLocationProviderDelegate { nit: Just "Delegate" should suffice https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:24: virtual void SendUpdateListenStateMessage(bool enable_high_accuracy) = 0; Remove "Send" and "Message" from these method names. The message sending aspect shouldn't be exposed to this layer. https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:28: virtual void SendStopListenStateMessage() = 0; I thought we were just going to have Update, not Start/Stop (or Update/Stop in this case)? https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:44: void SetDelegate(BlimpLocationProviderDelegate* delegate); These methods should be placed above the "implementation" block https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:107: remove these blank lines - they're part of the same semantic block https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:116: ditto https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:124: NOTREACHED() << "Engine received unexpected geolocation type."; break; https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/engine_geolocation_feature.h (right): https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.h:16: // geolocation. Delegates are notified of incoming messages by * There are no "delegates" here (implies calls are *sent* via a delegate) - the class is a delegate (*receives* calls). * The delegate methods do not receive messages. https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.h:38: BlimpLocationProvider* location_provider_; This is never set?
Hey CJ, I'll take over the review from Kevin. As he suggests, seems a good idea to get the unit-tests in place so you can fix up the functional errors for a final review.
Hi all, There was some major restructuring needed. EngineGeolocationFeature now receives a callback, which should call straight back to the LocationArbitratorImpl. Let me know what you think. There are also unittests now. Please let me know if you see anything I missed. Thanks, CJ https://codereview.chromium.org/2091023006/diff/40001/blimp/common/create_bli... File blimp/common/create_blimp_message.cc (right): https://codereview.chromium.org/2091023006/diff/40001/blimp/common/create_bli... blimp/common/create_blimp_message.cc:93: std::unique_ptr<BlimpMessage> output(new BlimpMessage); On 2016/06/29 17:52:55, Kevin M wrote: > You can use the new function in ptr_util.h: std::make_unique<BlimpMessage>() trouble finding std::make_unique. I know about base::MakeUnique. Are these different? https://codereview.chromium.org/2091023006/diff/40001/blimp/common/proto/geol... File blimp/common/proto/geolocation.proto (right): https://codereview.chromium.org/2091023006/diff/40001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:25: ERROR_CODE_LAST = 3; On 2016/06/29 17:52:55, Kevin M wrote: > No need to define this - the pb compiler defines a validity check function that > we can use. > > allow_alias can be removed too. Removed them. Can you explain more about the validity check function? https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:46: NotifyCallback(position); On 2016/06/29 17:52:55, Kevin M wrote: > Pick one: NOTIMPLEMENTED() with nothing else, or add an implementation. ;) Done. https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:53: DCHECK(!delegate); On 2016/06/29 17:52:55, Kevin M wrote: > Not sure if you intended to do this. ;) oops. https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:18: class BlimpLocationProviderDelegate { On 2016/06/29 17:52:55, Kevin M wrote: > nit: Just "Delegate" should suffice Done. https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:24: virtual void SendUpdateListenStateMessage(bool enable_high_accuracy) = 0; On 2016/06/29 17:52:55, Kevin M wrote: > Remove "Send" and "Message" from these method names. The message sending aspect > shouldn't be exposed to this layer. Done. https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:28: virtual void SendStopListenStateMessage() = 0; On 2016/06/29 17:52:55, Kevin M wrote: > I thought we were just going to have Update, not Start/Stop (or Update/Stop in > this case)? It is expressed that way in the proto, but I thought it would make sense just to have separate functions. https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:44: void SetDelegate(BlimpLocationProviderDelegate* delegate); On 2016/06/29 17:52:55, Kevin M wrote: > These methods should be placed above the "implementation" block Done. https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:107: On 2016/06/29 17:52:56, Kevin M wrote: > remove these blank lines - they're part of the same semantic block Done. https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:116: On 2016/06/29 17:52:56, Kevin M wrote: > ditto Done. https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:124: NOTREACHED() << "Engine received unexpected geolocation type."; On 2016/06/29 17:52:56, Kevin M wrote: > break; Done. https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/engine_geolocation_feature.h (right): https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.h:16: // geolocation. Delegates are notified of incoming messages by On 2016/06/29 17:52:56, Kevin M wrote: > * There are no "delegates" here (implies calls are *sent* via a delegate) - the > class is a delegate (*receives* calls). > * The delegate methods do not receive messages. Just removed stuff of delegates, and will update it on BlimpLocationProvider. https://codereview.chromium.org/2091023006/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.h:38: BlimpLocationProvider* location_provider_; On 2016/06/29 17:52:56, Kevin M wrote: > This is never set? 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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lethalantidote@chromium.org 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 to run a CQ dry run
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...)
Code comments; will follow up w/ code-review of the tests. https://codereview.chromium.org/2091023006/diff/80001/blimp/64 File blimp/64 (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/64#newcode1 blimp/64:1: // Copyright 2016 The Chromium Authors. All rights reserved. Looks like you copied a file into your working tree and checked it in by mistake? https://codereview.chromium.org/2091023006/diff/80001/blimp/common/logging.cc File blimp/common/logging.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/common/logging.cc... blimp/common/logging.cc:318: AddField("latitude", message.geolocation().location().latitude(), output); nit: Take a local const& to message.geolocation.location() to make this code more readable. https://codereview.chromium.org/2091023006/diff/80001/blimp/common/proto/geol... File blimp/common/proto/geolocation.proto (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:15: message ErrorMessage { Remember that message names are scoped to the package, so you're creating a blimp::ErrorMessage - there's nothing in that name to tie this to Geolocation. https://codereview.chromium.org/2091023006/diff/80001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:17: // to JavaScript without the need for a conversion. The Blink interface for PositionError (see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/geoloc...) has an enum that includes only the three valid error codes, though, and instances can only be created with the enum, so we definitely need a conversion. If you're arguing that we can static_cast<> then that's technically true, but we still shouldn't - we're receiving this data across the network, so we should treat it as "untrusted" and sanity-check all the values that we receive. In the case of enums simply casting to a C++ enum from an integer has undefined behaviour if the integer doesn't have one of the enums valid integer values. e.g. if you have: enum FooBar { FOO = 1, BAR = 2 } bool foo(int wibble) { switch (static_cast<FooBar>(wibble)) { case FOO: return true; case BAR: return false; } } and then call foo(5) then the compiler is at liberty to create a program that returns a random bool value, or even crashes. https://codereview.chromium.org/2091023006/diff/80001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:20: ERROR_CODE_NONE = 0; Do we need NONE as an error enum value? Surely by definition if you're receiving an ErrorMessage then there was an error? https://codereview.chromium.org/2091023006/diff/80001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:30: message LocationMessage { nit: We should use either Location or Geolocation consistently; the latter is more precise so I'd suggest using that for the proto messages (where we're overriding interfaces with LocationFoo naming, we should be consistent with the interface though :-/ ) https://codereview.chromium.org/2091023006/diff/80001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:31: // Refer to content/public/common/geoposition.h for explanation of geoposition.h is part of the Chromium-specific Content-layer implementation of this stuff - it's implementation-specific. I'd recommend referring instead to the Geolocation API specification, e.g. to the Position and Coordinates interfaces. I'd also suggest actually mirroring those interfaces in the protos, so that this would become: message GeolocationPositionMessage { GeolocationCoordinates coords; // Need a comment to explain what this is relative to, since Client & Engine have different clocks - do we need to do some kind of adaptation at the Engine to convert to DOMTimestamp..? int64 timestamp_millis; } https://codereview.chromium.org/2091023006/diff/80001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:43: message UpdateListenStateMessage { nit: Suggest GeolocationRequestMessage, GeolocationInterestMessage or GeolocationListenerMessage and tweaking the enum to have values like REQUEST_NONE/HIGH/LOW. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_content_browser_client.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.cc:24: base::WeakPtr<EngineGeolocationFeature> feature) { nit: Looks like you mean to just pass WeakPtr<BlimpLocationProvider::Delegate> here (in which case rename parameter to feature_delegate or something). https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.cc:38: base::WeakPtr<BlimpLocationProvider::Delegate> feature_; You're assigning this from an EngineGeolocationFeature, above, though - are you sure this compiles? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.cc:77: ->GetGeolocationFeature(); nit: Is this the git cl format preferred line-wrap? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.cc:79: new BlimpGeolocationDelegate(geolocation_feature->GetWeakPtr()); Rather than expose WeakPtr<EngineGelocationFeature> outside the feature, why not move BlimpGeolocationDelegate to be an internal implementation detail of EngineGeolocationFeature, since that's effectively what it is? So then EGFeature just has a CreateGeolocationDelegate() method. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:30: DCHECK(delegate_); |delegate_| is a WeakPtr, so it's perfectly valid for it to be null, surely? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:56: NOTIMPLEMENTED(); Add a comment to explain why we're both requesting a refresh AND saying NOTIMPLEMENTED here? What is this API actually supposed to do? Are we not supposed to start generating output until we get this call, for example..? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:61: DCHECK(!callback.is_null()); LocationProvider does not specify that it's invalid to provide a null callback, and if you look at the base implementation, it actually explicitly allows for it being null - so your DCHECK is enforcing a requirement that callers don't expect to have to meet. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:63: delegate_->SetUpdateCallback(base::Bind(callback, base::Unretained(this))); You're passing Bind()ing to Unretained(this) - what happens if you are deleted before the Delegate? Might it still send you another event, and thereby cause you to crash? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:19: class Delegate { This interface looks exactly like the LocationProvider interface, except that you've renamed some things, and we don't have GetPosition() nor OnPermissionGranted() - can we follow the naming scheme of LocationProvider instead, and just have a comment explaining that the Delegate implements a subset of the LocationProvider's function? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:29: virtual void StopListenState() = 0; This name is rather confusing; what does it mean to "stop listen state"? You might reasonably replace the start/stop calls with a single RequestAccuracy(NONE/HIGH/LOW), if that makes the code simpler. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:37: virtual void NotifyCallback(const content::Geoposition& position) = 0; Why does the Delegate need a NotifyCallback() API? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:43: void SetDelegate(base::WeakPtr<Delegate> delegate); This needs a comment to explain what the |delegate| is for, and why we pass in a WeakPtr to it. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:55: base::WeakPtr<Delegate> delegate_ = nullptr; I'm surprised that this even compiles, since you're trying to initialize your WeakPtr from a raw pointer. WeakPtrs (like unique_ptr<>, scoped_refptr<> etc) are initialized to null by default. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:57: content::Geoposition position_; nit: Add a comment to explain why this is required - presumably because GetPosition() expects us to return a cached position immediately? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:19: content::Geoposition::ErrorCode GetErrorCode( nit: why is this GetErrorCode but the next method is ConvertLocationMessage? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:34: const LocationMessage& location) { nit: Any reason this can't be: Geoposition ConvertLocationMessage(LocationMessage) ? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:66: UpdateListenStateMessage* details = nit: Name the variable after the field in the message, for clarity. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:79: CHECK(false); This should break your tests, right? But the try-bots are green, so are your tests not running? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:80: GeolocationMessage* geolocation_message; nit: Here and elsewhere, initialize this to nullptr, in case CreateBlimpMessage is buggy and doesn't. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:105: DCHECK(!callback.is_null()); See BLP comment - I don't think this check (here and elsewhere) is valid? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:111: const content::Geoposition& position) { Why do you need this method to be part of the Delegate interface? Isn't it just an implementation detail of this class? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:122: content::Geoposition output; You only need output in case of kLocation and kError, so define it there, not here. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:140: NOTREACHED() << "Engine received unexpected geolocation type."; NOTREACHED() means "this line of code will never ever be executed", which is stronger than you intend, I think; you probably want DLOG(FATAL) << "Client sent unexpected message type:" << type (DLOG(FATAL) will compile to the same as NOTREACHED, I think, but IMO it more clearly expresses the intent here - this is a protocol error, not a coding error, basically) https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:142: case GeolocationMessage::TYPE_NOT_SET: nit: You can combine this with the two other unexpected cases, above. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/engine_geolocation_feature.h (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.h:30: // BlimpLocationProviderDelegate implementation. nit: BlimpLocationProvider::Delegate https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.h:46: base::Callback<void(const content::Geoposition&)> callback_; nit: Give this callback a more descriptive name? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:224: geolocation_feature_(), nit: Why include this in the initializer list if it doesn't need any parameters? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:100: EngineGeolocationFeature* GetGeolocationFeature(); nit: "gets" is ambiguous; be explicit re ownership expectations, e.g. "Returns a pointer to the Geolocation feature." makes it clearer that it does not return _ownership_ of the feature.
https://codereview.chromium.org/2091023006/diff/80001/blimp/64 File blimp/64 (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/64#newcode1 blimp/64:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/07/12 21:35:12, Wez wrote: > Looks like you copied a file into your working tree and checked it in by > mistake? Yeah not even sure where this came from. Sorry about that. There seems to be some weird things floating about. Not sure how I did that, but I'll get on it. https://codereview.chromium.org/2091023006/diff/80001/blimp/common/logging.cc File blimp/common/logging.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/common/logging.cc... blimp/common/logging.cc:318: AddField("latitude", message.geolocation().location().latitude(), output); On 2016/07/12 21:35:12, Wez wrote: > nit: Take a local const& to message.geolocation.location() to make this code > more readable. Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/common/proto/geol... File blimp/common/proto/geolocation.proto (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:15: message ErrorMessage { On 2016/07/12 21:35:12, Wez wrote: > Remember that message names are scoped to the package, so you're creating a > blimp::ErrorMessage - there's nothing in that name to tie this to Geolocation. Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:17: // to JavaScript without the need for a conversion. On 2016/07/12 21:35:12, Wez wrote: > The Blink interface for PositionError (see > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/geoloc...) > has an enum that includes only the three valid error codes, though, and > instances can only be created with the enum, so we definitely need a conversion. > > If you're arguing that we can static_cast<> then that's technically true, but we > still shouldn't - we're receiving this data across the network, so we should > treat it as "untrusted" and sanity-check all the values that we receive. > > In the case of enums simply casting to a C++ enum from an integer has undefined > behaviour if the integer doesn't have one of the enums valid integer values. > e.g. if you have: > > enum FooBar { FOO = 1, BAR = 2 } > > bool foo(int wibble) { > switch (static_cast<FooBar>(wibble)) { > case FOO: return true; > case BAR: return false; > } > } > > and then call foo(5) then the compiler is at liberty to create a program that > returns a random bool value, or even crashes. Removed comment. https://codereview.chromium.org/2091023006/diff/80001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:20: ERROR_CODE_NONE = 0; On 2016/07/12 21:35:12, Wez wrote: > Do we need NONE as an error enum value? Surely by definition if you're receiving > an ErrorMessage then there was an error? Sure. Good point. Since I'm making a different message for Location anyway, Error_CODE_NONE would never show up. https://codereview.chromium.org/2091023006/diff/80001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:30: message LocationMessage { On 2016/07/12 21:35:12, Wez wrote: > nit: We should use either Location or Geolocation consistently; the latter is > more precise so I'd suggest using that for the proto messages (where we're > overriding interfaces with LocationFoo naming, we should be consistent with the > interface though :-/ ) Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:31: // Refer to content/public/common/geoposition.h for explanation of On 2016/07/12 21:35:12, Wez wrote: > geoposition.h is part of the Chromium-specific Content-layer implementation of > this stuff - it's implementation-specific. > > I'd recommend referring instead to the Geolocation API specification, e.g. to > the Position and Coordinates interfaces. I'd also suggest actually mirroring > those interfaces in the protos, so that this would become: > > message GeolocationPositionMessage { > GeolocationCoordinates coords; > > // Need a comment to explain what this is relative to, since Client & Engine > have different clocks - do we need to do some kind of adaptation at the Engine > to convert to DOMTimestamp..? > int64 timestamp_millis; > } > > Added GeolocationCoordinates. As for the comment, done. There probably should be something. As for this field, would it better serve to have it as double seconds? https://codereview.chromium.org/2091023006/diff/80001/blimp/common/proto/geol... blimp/common/proto/geolocation.proto:43: message UpdateListenStateMessage { On 2016/07/12 21:35:12, Wez wrote: > nit: Suggest GeolocationRequestMessage, GeolocationInterestMessage or > GeolocationListenerMessage and tweaking the enum to have values like > REQUEST_NONE/HIGH/LOW. Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_content_browser_client.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.cc:24: base::WeakPtr<EngineGeolocationFeature> feature) { On 2016/07/12 21:35:12, Wez wrote: > nit: Looks like you mean to just pass WeakPtr<BlimpLocationProvider::Delegate> > here (in which case rename parameter to feature_delegate or something). Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.cc:38: base::WeakPtr<BlimpLocationProvider::Delegate> feature_; On 2016/07/12 21:35:12, Wez wrote: > You're assigning this from an EngineGeolocationFeature, above, though - are you > sure this compiles? It does. EngineGeolocationFeature is a BlimpLocationProvider::Delegate. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.cc:77: ->GetGeolocationFeature(); On 2016/07/12 21:35:12, Wez wrote: > nit: Is this the git cl format preferred line-wrap? Yes. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.cc:79: new BlimpGeolocationDelegate(geolocation_feature->GetWeakPtr()); On 2016/07/12 21:35:13, Wez wrote: > Rather than expose WeakPtr<EngineGelocationFeature> outside the feature, why not > move BlimpGeolocationDelegate to be an internal implementation detail of > EngineGeolocationFeature, since that's effectively what it is? > > So then EGFeature just has a CreateGeolocationDelegate() method. BlimpGeolocationDelegate has nothing to do with EngineGeolocationFeature except that's where I happen to be hooking BlimpLocationProvider and EngineGeolocationFeature together. BlimpGeolocationDelegate has all the previously content related functions in it. I am a little confused. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:30: DCHECK(delegate_); On 2016/07/12 21:35:13, Wez wrote: > |delegate_| is a WeakPtr, so it's perfectly valid for it to be null, surely? Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:56: NOTIMPLEMENTED(); On 2016/07/12 21:35:13, Wez wrote: > Add a comment to explain why we're both requesting a refresh AND saying > NOTIMPLEMENTED here? > > What is this API actually supposed to do? Are we not supposed to start > generating output until we get this call, for example..? I think I can remove the NOTIMPLEMENTED here. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:61: DCHECK(!callback.is_null()); On 2016/07/12 21:35:13, Wez wrote: > LocationProvider does not specify that it's invalid to provide a null callback, > and if you look at the base implementation, it actually explicitly allows for it > being null - so your DCHECK is enforcing a requirement that callers don't expect > to have to meet. Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:63: delegate_->SetUpdateCallback(base::Bind(callback, base::Unretained(this))); On 2016/07/12 21:35:13, Wez wrote: > You're passing Bind()ing to Unretained(this) - what happens if you are deleted > before the Delegate? Might it still send you another event, and thereby cause > you to crash? I don't think that could happen? BlimpLocationProvider is owned by LocationArbitratorImpl, where this callback eventually ends up. The BlimpLocationProvider is given to be the first argument to OnLocationResponse. If there is a concern, wouldn't we just want to add a check in OnLocationResponse? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:19: class Delegate { On 2016/07/12 21:35:13, Wez wrote: > This interface looks exactly like the LocationProvider interface, except that > you've renamed some things, and we don't have GetPosition() nor > OnPermissionGranted() - can we follow the naming scheme of LocationProvider > instead, and just have a comment explaining that the Delegate implements a > subset of the LocationProvider's function? Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:29: virtual void StopListenState() = 0; On 2016/07/12 21:35:13, Wez wrote: > This name is rather confusing; what does it mean to "stop listen state"? > > You might reasonably replace the start/stop calls with a single > RequestAccuracy(NONE/HIGH/LOW), if that makes the code simpler. It's now changed to StopListener, as per the previous comment. Rather keep the Start/Stop functions separated, that way the BlimpLocaitonProvider doesn't need to know about None/High/Low, if that makes sense. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:37: virtual void NotifyCallback(const content::Geoposition& position) = 0; On 2016/07/12 21:35:13, Wez wrote: > Why does the Delegate need a NotifyCallback() API? Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:43: void SetDelegate(base::WeakPtr<Delegate> delegate); On 2016/07/12 21:35:13, Wez wrote: > This needs a comment to explain what the |delegate| is for, and why we pass in a > WeakPtr to it. Tried to explain. Please tell me if I'm off. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:55: base::WeakPtr<Delegate> delegate_ = nullptr; On 2016/07/12 21:35:13, Wez wrote: > I'm surprised that this even compiles, since you're trying to initialize your > WeakPtr from a raw pointer. > > WeakPtrs (like unique_ptr<>, scoped_refptr<> etc) are initialized to null by > default. Didn't know that. Removing nullptr. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:57: content::Geoposition position_; On 2016/07/12 21:35:13, Wez wrote: > nit: Add a comment to explain why this is required - presumably because > GetPosition() expects us to return a cached position immediately? Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:19: content::Geoposition::ErrorCode GetErrorCode( On 2016/07/12 21:35:14, Wez wrote: > nit: why is this GetErrorCode but the next method is ConvertLocationMessage? Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:34: const LocationMessage& location) { On 2016/07/12 21:35:13, Wez wrote: > nit: Any reason this can't be: > > Geoposition ConvertLocationMessage(LocationMessage) > > ? Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:66: UpdateListenStateMessage* details = On 2016/07/12 21:35:13, Wez wrote: > nit: Name the variable after the field in the message, for clarity. Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:79: CHECK(false); On 2016/07/12 21:35:14, Wez wrote: > This should break your tests, right? But the try-bots are green, so are your > tests not running? Fixed. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:80: GeolocationMessage* geolocation_message; On 2016/07/12 21:35:14, Wez wrote: > nit: Here and elsewhere, initialize this to nullptr, in case CreateBlimpMessage > is buggy and doesn't. Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:105: DCHECK(!callback.is_null()); On 2016/07/12 21:35:13, Wez wrote: > See BLP comment - I don't think this check (here and elsewhere) is valid? Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:111: const content::Geoposition& position) { On 2016/07/12 21:35:13, Wez wrote: > Why do you need this method to be part of the Delegate interface? Isn't it just > an implementation detail of this class? Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:122: content::Geoposition output; On 2016/07/12 21:35:14, Wez wrote: > You only need output in case of kLocation and kError, so define it there, not > here. Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:140: NOTREACHED() << "Engine received unexpected geolocation type."; On 2016/07/12 21:35:14, Wez wrote: > NOTREACHED() means "this line of code will never ever be executed", which is > stronger than you intend, I think; you probably want DLOG(FATAL) << "Client sent > unexpected message type:" << type > > (DLOG(FATAL) will compile to the same as NOTREACHED, I think, but IMO it more > clearly expresses the intent here - this is a protocol error, not a coding > error, basically) Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:142: case GeolocationMessage::TYPE_NOT_SET: On 2016/07/12 21:35:13, Wez wrote: > nit: You can combine this with the two other unexpected cases, above. Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/engine_geolocation_feature.h (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.h:30: // BlimpLocationProviderDelegate implementation. On 2016/07/12 21:35:14, Wez wrote: > nit: BlimpLocationProvider::Delegate Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.h:46: base::Callback<void(const content::Geoposition&)> callback_; On 2016/07/12 21:35:14, Wez wrote: > nit: Give this callback a more descriptive name? Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:224: geolocation_feature_(), On 2016/07/12 21:35:14, Wez wrote: > nit: Why include this in the initializer list if it doesn't need any parameters? Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:100: EngineGeolocationFeature* GetGeolocationFeature(); On 2016/07/12 21:35:14, Wez wrote: > nit: "gets" is ambiguous; be explicit re ownership expectations, e.g. "Returns a > pointer to the Geolocation feature." makes it clearer that it does not return > _ownership_ of the feature. 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2091023006/diff/80001/blimp/common/logging.cc File blimp/common/logging.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/common/logging.cc... blimp/common/logging.cc:318: AddField("latitude", message.geolocation().location().latitude(), output); On 2016/07/13 21:49:19, CJ wrote: > On 2016/07/12 21:35:12, Wez wrote: > > nit: Take a local const& to message.geolocation.location() to make this code > > more readable. > > Done. nit: I'd suggest taking the reference to location.coordinates() - then you'll need a "long line" for |timestamp_millis| but the rest will be shorter and easier to read. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_content_browser_client.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.cc:38: base::WeakPtr<BlimpLocationProvider::Delegate> feature_; On 2016/07/13 21:49:19, CJ wrote: > On 2016/07/12 21:35:12, Wez wrote: > > You're assigning this from an EngineGeolocationFeature, above, though - are > you > > sure this compiles? > > It does. EngineGeolocationFeature is a BlimpLocationProvider::Delegate. Right, but it's strange to require that the caller pass an EngineGeolocationFeature pointer, but then to store it as a less-specific thing - either you need to have an EngineGeolocationFeature, or you can cope with any BlimpLocationProvider::Delegate (this would be more relevant if this class were to get its own set of unit-tests, since being able to pass a BlimpLocationProvider::Delegate would enable you to pass it a mock) https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.cc:79: new BlimpGeolocationDelegate(geolocation_feature->GetWeakPtr()); On 2016/07/13 21:49:19, CJ wrote: > On 2016/07/12 21:35:13, Wez wrote: > > Rather than expose WeakPtr<EngineGelocationFeature> outside the feature, why > not > > move BlimpGeolocationDelegate to be an internal implementation detail of > > EngineGeolocationFeature, since that's effectively what it is? > > > > So then EGFeature just has a CreateGeolocationDelegate() method. > > BlimpGeolocationDelegate has nothing to do with EngineGeolocationFeature except > that's where I happen to be hooking BlimpLocationProvider and > EngineGeolocationFeature together. BlimpGeolocationDelegate has all the > previously content related functions in it. I am a little confused. I'm not sure what you mean by "has nothing to do with" - EngineGeolocationFeature encapsulates the Engine-side geolocation implementation, of which our content::GeolocationDelegate and content::LocationProvider implementations are a part. They seem pretty closely related. :P https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:30: DCHECK(delegate_); On 2016/07/13 21:49:19, CJ wrote: > On 2016/07/12 21:35:13, Wez wrote: > > |delegate_| is a WeakPtr, so it's perfectly valid for it to be null, surely? > > Done. I meant that, if it is possible for the WeakPtr to have become null by this point then you must check it before you try to use it. All WeakPtr does is allow the owner to "invalidate" it, after which it behaves like a null pointer - callers can check it each time they're about to use it and skip doing work that would use the pointer if it has become null. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:56: NOTIMPLEMENTED(); On 2016/07/13 21:49:20, CJ wrote: > On 2016/07/12 21:35:13, Wez wrote: > > Add a comment to explain why we're both requesting a refresh AND saying > > NOTIMPLEMENTED here? > > > > What is this API actually supposed to do? Are we not supposed to start > > generating output until we get this call, for example..? > > I think I can remove the NOTIMPLEMENTED here. Acknowledged. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:63: delegate_->SetUpdateCallback(base::Bind(callback, base::Unretained(this))); On 2016/07/13 21:49:20, CJ wrote: > On 2016/07/12 21:35:13, Wez wrote: > > You're passing Bind()ing to Unretained(this) - what happens if you are deleted > > before the Delegate? Might it still send you another event, and thereby cause > > you to crash? > > I don't think that could happen? BlimpLocationProvider is owned by > LocationArbitratorImpl, where this callback eventually ends up. Not sure what you mean by "ends up". You're passing the callback to the Delegate, i.e. to the Feature - what stops the Feature calling it after the LocationArbitratorImpl has torn-down the BlimpLocationProvider? > The > BlimpLocationProvider is given to be the first argument to OnLocationResponse. > If there is a concern, wouldn't we just want to add a check in > OnLocationResponse? Ah, so you are arguing that OnLocationResponse can cope with being called with a pointer to a dead LocationProvider - which is the case if it really only uses it to compare with its internal list of providers? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:29: virtual void StopListenState() = 0; On 2016/07/13 21:49:20, CJ wrote: > On 2016/07/12 21:35:13, Wez wrote: > > This name is rather confusing; what does it mean to "stop listen state"? > > > > You might reasonably replace the start/stop calls with a single > > RequestAccuracy(NONE/HIGH/LOW), if that makes the code simpler. > > It's now changed to StopListener, as per the previous comment. > > Rather keep the Start/Stop functions separated, that way the > BlimpLocaitonProvider doesn't need to know about None/High/Low, if that makes > sense. IIUC you're arguing that this allows you to isolate the BlimpLocationProvider from the NONE/HIGH/LOW enums in your proto? The BlimpLocationProvider is part of the EngineGeolocationFeature implementation, though, so it's not unreasonable for it to know about that enum. It's also the BlimpLocationProvider's sole function to adapt between the content::LocationProvider API, and something more useful to the EngineGeolocationFeature - otherwise we could just have EngineGeolocationFeature implement the LocationProvider API itself, and have this class be a dumb LocationProvider proxy. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:43: void SetDelegate(base::WeakPtr<Delegate> delegate); On 2016/07/13 21:49:20, CJ wrote: > On 2016/07/12 21:35:13, Wez wrote: > > This needs a comment to explain what the |delegate| is for, and why we pass in > a > > WeakPtr to it. > > Tried to explain. Please tell me if I'm off. nit: Looks like you can move the |delegate| parameter to be a parameter of the BlimpLocationProvider constructor, since you never need to construct one _without_ a delegate, right? https://codereview.chromium.org/2091023006/diff/120001/blimp/common/proto/geo... File blimp/common/proto/geolocation.proto (right): https://codereview.chromium.org/2091023006/diff/120001/blimp/common/proto/geo... blimp/common/proto/geolocation.proto:39: // Client-side time If you check the LocationProvider interface you'll see that there's an expectation that the timestamp is the browser's notion of time, not the source's (even in Chrome, the location info might come from a GPS unit with its own clock :) I would suggest that we omit this field, for now, and synthesize it when we receive the location messages at the Engine - it's intended for use by content in determining the freshness of location information, and so is mainly useful in getCurrentPosition() results. When JS receives position change events, by definition those are as fresh as we can make them, so the timestamp may as well be "now". https://codereview.chromium.org/2091023006/diff/120001/blimp/common/proto/geo... blimp/common/proto/geolocation.proto:48: // for updates. My suggestion was to rename ListenState and the enum values to be consistent with the message terminology. I'd recommend calling this something like GeolocationSetLevelMessage with enum called Level and values NO_LOCATION, HIGH_ACCURACY, LOW_ACCURACY. That way there is a single notion of what "level" of location information the Engine wants, ranging from none at all to high-accuracy. https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:23: // Implments subset of LocationProvider's functions. typo: Implements. https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:121: const GeolocationPositionMessage location = nit: const& here https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:128: content::Geoposition output; nit: As per style-guide, only declare |output| immediately before you first need to use it. https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:129: const GeolocationErrorMessage error_message = geolocation_message.error(); nit: const & https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.h (right): https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.h:45: geolocation_received_callback_; nit: You mean geoposition_received_callback_? https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.h:48: void NotifyCallback(const content::Geoposition& position); nit: Order methods vs member variables as per the style-guide, plz. :)
https://codereview.chromium.org/2091023006/diff/80001/blimp/common/logging.cc File blimp/common/logging.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/common/logging.cc... blimp/common/logging.cc:318: AddField("latitude", message.geolocation().location().latitude(), output); On 2016/07/14 01:19:21, Wez wrote: > On 2016/07/13 21:49:19, CJ wrote: > > On 2016/07/12 21:35:12, Wez wrote: > > > nit: Take a local const& to message.geolocation.location() to make this code > > > more readable. > > > > Done. > > nit: I'd suggest taking the reference to location.coordinates() - then you'll > need a "long line" for |timestamp_millis| but the rest will be shorter and > easier to read. Any draw back to leaving the location reference in as well? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_content_browser_client.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.cc:38: base::WeakPtr<BlimpLocationProvider::Delegate> feature_; On 2016/07/14 01:19:21, Wez wrote: > On 2016/07/13 21:49:19, CJ wrote: > > On 2016/07/12 21:35:12, Wez wrote: > > > You're assigning this from an EngineGeolocationFeature, above, though - are > > you > > > sure this compiles? > > > > It does. EngineGeolocationFeature is a BlimpLocationProvider::Delegate. > > Right, but it's strange to require that the caller pass an > EngineGeolocationFeature pointer, but then to store it as a less-specific thing > - either you need to have an EngineGeolocationFeature, or you can cope with any > BlimpLocationProvider::Delegate > > (this would be more relevant if this class were to get its own set of > unit-tests, since being able to pass a BlimpLocationProvider::Delegate would > enable you to pass it a mock) Acknowledged. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/app/blimp_... blimp/engine/app/blimp_content_browser_client.cc:79: new BlimpGeolocationDelegate(geolocation_feature->GetWeakPtr()); On 2016/07/14 01:19:21, Wez wrote: > On 2016/07/13 21:49:19, CJ wrote: > > On 2016/07/12 21:35:13, Wez wrote: > > > Rather than expose WeakPtr<EngineGelocationFeature> outside the feature, why > > not > > > move BlimpGeolocationDelegate to be an internal implementation detail of > > > EngineGeolocationFeature, since that's effectively what it is? > > > > > > So then EGFeature just has a CreateGeolocationDelegate() method. > > > > BlimpGeolocationDelegate has nothing to do with EngineGeolocationFeature > except > > that's where I happen to be hooking BlimpLocationProvider and > > EngineGeolocationFeature together. BlimpGeolocationDelegate has all the > > previously content related functions in it. I am a little confused. > > I'm not sure what you mean by "has nothing to do with" - > EngineGeolocationFeature encapsulates the Engine-side geolocation > implementation, of which our content::GeolocationDelegate and > content::LocationProvider implementations are a part. They seem pretty closely > related. :P Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:30: DCHECK(delegate_); On 2016/07/14 01:19:21, Wez wrote: > On 2016/07/13 21:49:19, CJ wrote: > > On 2016/07/12 21:35:13, Wez wrote: > > > |delegate_| is a WeakPtr, so it's perfectly valid for it to be null, surely? > > > > Done. > > I meant that, if it is possible for the WeakPtr to have become null by this > point then you must check it before you try to use it. > > All WeakPtr does is allow the owner to "invalidate" it, after which it behaves > like a null pointer - callers can check it each time they're about to use it and > skip doing work that would use the pointer if it has become null. Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:63: delegate_->SetUpdateCallback(base::Bind(callback, base::Unretained(this))); On 2016/07/14 01:19:21, Wez wrote: > On 2016/07/13 21:49:20, CJ wrote: > > On 2016/07/12 21:35:13, Wez wrote: > > > You're passing Bind()ing to Unretained(this) - what happens if you are > deleted > > > before the Delegate? Might it still send you another event, and thereby > cause > > > you to crash? > > > > I don't think that could happen? BlimpLocationProvider is owned by > > LocationArbitratorImpl, where this callback eventually ends up. > > Not sure what you mean by "ends up". You're passing the callback to the > Delegate, i.e. to the Feature - what stops the Feature calling it after the > LocationArbitratorImpl has torn-down the BlimpLocationProvider? > > > The > > BlimpLocationProvider is given to be the first argument to OnLocationResponse. > > If there is a concern, wouldn't we just want to add a check in > > OnLocationResponse? > > Ah, so you are arguing that OnLocationResponse can cope with being called with a > pointer to a dead LocationProvider - which is the case if it really only uses it > to compare with its internal list of providers? I believe so yes. It just uses that pointer to update it's position_provider field. https://cs.chromium.org/chromium/src/content/browser/geolocation/location_arb... So should I put a check in OnLocationResponse? https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:29: virtual void StopListenState() = 0; On 2016/07/14 01:19:21, Wez wrote: > On 2016/07/13 21:49:20, CJ wrote: > > On 2016/07/12 21:35:13, Wez wrote: > > > This name is rather confusing; what does it mean to "stop listen state"? > > > > > > You might reasonably replace the start/stop calls with a single > > > RequestAccuracy(NONE/HIGH/LOW), if that makes the code simpler. > > > > It's now changed to StopListener, as per the previous comment. > > > > Rather keep the Start/Stop functions separated, that way the > > BlimpLocaitonProvider doesn't need to know about None/High/Low, if that makes > > sense. > > IIUC you're arguing that this allows you to isolate the BlimpLocationProvider > from the NONE/HIGH/LOW enums in your proto? > > The BlimpLocationProvider is part of the EngineGeolocationFeature > implementation, though, so it's not unreasonable for it to know about that enum. > > It's also the BlimpLocationProvider's sole function to adapt between the > content::LocationProvider API, and something more useful to the > EngineGeolocationFeature - otherwise we could just have EngineGeolocationFeature > implement the LocationProvider API itself, and have this class be a dumb > LocationProvider proxy. Done. https://codereview.chromium.org/2091023006/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:43: void SetDelegate(base::WeakPtr<Delegate> delegate); On 2016/07/14 01:19:21, Wez wrote: > On 2016/07/13 21:49:20, CJ wrote: > > On 2016/07/12 21:35:13, Wez wrote: > > > This needs a comment to explain what the |delegate| is for, and why we pass > in > > a > > > WeakPtr to it. > > > > Tried to explain. Please tell me if I'm off. > > nit: Looks like you can move the |delegate| parameter to be a parameter of the > BlimpLocationProvider constructor, since you never need to construct one > _without_ a delegate, right? Done. https://codereview.chromium.org/2091023006/diff/120001/blimp/common/proto/geo... File blimp/common/proto/geolocation.proto (right): https://codereview.chromium.org/2091023006/diff/120001/blimp/common/proto/geo... blimp/common/proto/geolocation.proto:39: // Client-side time On 2016/07/14 01:19:21, Wez wrote: > If you check the LocationProvider interface you'll see that there's an > expectation that the timestamp is the browser's notion of time, not the source's > (even in Chrome, the location info might come from a GPS unit with its own clock > :) > > I would suggest that we omit this field, for now, and synthesize it when we > receive the location messages at the Engine - it's intended for use by content > in determining the freshness of location information, and so is mainly useful in > getCurrentPosition() results. > > When JS receives position change events, by definition those are as fresh as we > can make them, so the timestamp may as well be "now". Ah okay! Did not know that. Updating. https://codereview.chromium.org/2091023006/diff/120001/blimp/common/proto/geo... blimp/common/proto/geolocation.proto:48: // for updates. On 2016/07/14 01:19:21, Wez wrote: > My suggestion was to rename ListenState and the enum values to be consistent > with the message terminology. > > I'd recommend calling this something like GeolocationSetLevelMessage with enum > called Level and values NO_LOCATION, HIGH_ACCURACY, LOW_ACCURACY. That way there > is a single notion of what "level" of location information the Engine wants, > ranging from none at all to high-accuracy. Done, but slightly different. Thought NO_INTEREST would be better than NO_LOCATION and keeping Interest in the name to be explicit. https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:23: // Implments subset of LocationProvider's functions. On 2016/07/14 01:19:22, Wez wrote: > typo: Implements. Done. https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:121: const GeolocationPositionMessage location = On 2016/07/14 01:19:22, Wez wrote: > nit: const& here Done. https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:128: content::Geoposition output; On 2016/07/14 01:19:22, Wez wrote: > nit: As per style-guide, only declare |output| immediately before you first need > to use it. Done. https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:129: const GeolocationErrorMessage error_message = geolocation_message.error(); On 2016/07/14 01:19:22, Wez wrote: > nit: const & Done. https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.h (right): https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.h:45: geolocation_received_callback_; On 2016/07/14 01:19:22, Wez wrote: > nit: You mean geoposition_received_callback_? Done. https://codereview.chromium.org/2091023006/diff/120001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.h:48: void NotifyCallback(const content::Geoposition& position); On 2016/07/14 01:19:22, Wez wrote: > nit: Order methods vs member variables as per the style-guide, plz. :) Done.
Some remaining nits and test cleanup, but otherwise this is looking great. https://codereview.chromium.org/2091023006/diff/120001/blimp/common/proto/geo... File blimp/common/proto/geolocation.proto (right): https://codereview.chromium.org/2091023006/diff/120001/blimp/common/proto/geo... blimp/common/proto/geolocation.proto:39: // Client-side time On 2016/07/14 23:55:09, CJ wrote: > On 2016/07/14 01:19:21, Wez wrote: > > If you check the LocationProvider interface you'll see that there's an > > expectation that the timestamp is the browser's notion of time, not the > source's > > (even in Chrome, the location info might come from a GPS unit with its own > clock > > :) > > > > I would suggest that we omit this field, for now, and synthesize it when we > > receive the location messages at the Engine - it's intended for use by content > > in determining the freshness of location information, and so is mainly useful > in > > getCurrentPosition() results. > > > > When JS receives position change events, by definition those are as fresh as > we > > can make them, so the timestamp may as well be "now". > > Ah okay! Did not know that. Updating. Acknowledged. https://codereview.chromium.org/2091023006/diff/120001/blimp/common/proto/geo... blimp/common/proto/geolocation.proto:48: // for updates. On 2016/07/14 23:55:09, CJ wrote: > On 2016/07/14 01:19:21, Wez wrote: > > My suggestion was to rename ListenState and the enum values to be consistent > > with the message terminology. > > > > I'd recommend calling this something like GeolocationSetLevelMessage with enum > > called Level and values NO_LOCATION, HIGH_ACCURACY, LOW_ACCURACY. That way > there > > is a single notion of what "level" of location information the Engine wants, > > ranging from none at all to high-accuracy. > > Done, but slightly different. Thought NO_INTEREST would be better than > NO_LOCATION and keeping Interest in the name to be explicit. Acknowledged. https://codereview.chromium.org/2091023006/diff/140001/blimp/common/logging.cc File blimp/common/logging.cc (right): https://codereview.chromium.org/2091023006/diff/140001/blimp/common/logging.c... blimp/common/logging.cc:318: AddField("subtype", "COORDINDATES", output); typo: COORDINATES https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:47: DCHECK(position); nit: No need for this, since *position = foo will give a nice clean crash if position is null. :D https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:64: delegate_->SetUpdateCallback(base::Bind(callback, base::Unretained(this))); So we are able to assume that the caller will call SetUpdateCallback immediately after we return this, before delegate_ can become null? (Not disagreeing, just confirming ;) https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:21: // A provider of services needed by Geolocation. nit: Suggest something more like "content::GeolocationDelegate implementation integrated with the EngineGeolocationFeature." or similar. :) https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:29: bool UseNetworkLocationProviders() final { return false; } BTW I had no idea we were using final in C++ now. Neat! https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:35: return base::WrapUnique(location_provider); nit: Just put the new inside WrapUnique() https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:85: return new BlimpGeolocationDelegate(GetWeakPtr()); Just call weak_factory_.GetweakPtr() directly here - no need for this publicly-visible GetWeakPtr() method. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.h (right): https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.h:29: content::GeolocationDelegate* CreateGeolocationDelegate(); unique_ptr<..> here, please, so it's clear that the caller ends up owning it :) https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.h:31: base::WeakPtr<EngineGeolocationFeature> GetWeakPtr(); You don't need this method any more, which is nice because you no longer risk some later CL (ab)using your WeakPtrs. Hurrah! https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.h:33: // BlimpLocationProvider::Delegate implementation. nit: Since this is an implementation detail you can move these methods into the private section (the inheritance of the Delegate interface has to stay public, though). ProcessMessage is part of the actual public interface of the class, so would need to stay in the public section. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc:58: MATCHER_P(EqualsUpdatedListenState, accuracy, "") { Looks like some of the naming in the tests need updating? https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc:105: TEST_F(EngineGeolocationFeatureTest, TestLocationReceived) { nit: Don't prefix your test names with Test - the fixture name already ends in Test, and this is clearly test code :) More generally, tests should focus on particular behaviours, not particular APIs - it's a little trickier with this code since it's message-processing, so tests for each message type does make more sense here. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc:150: feature_.RequestAccuracy(GeolocationSetInterestLevelMessage::NO_INTEREST); You could fold this into the preceding test. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc:153: TEST_F(EngineGeolocationFeatureTest, TestSendRequestRefreshMessage) { How about a test for EngineGeolocationFeature seeing some message that it isn't expecting? Or a message that it is expecting, but that is malformed? :)
https://codereview.chromium.org/2091023006/diff/140001/blimp/common/logging.cc File blimp/common/logging.cc (right): https://codereview.chromium.org/2091023006/diff/140001/blimp/common/logging.c... blimp/common/logging.cc:318: AddField("subtype", "COORDINDATES", output); On 2016/07/15 01:46:17, Wez wrote: > typo: COORDINATES Done. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:47: DCHECK(position); On 2016/07/15 01:46:17, Wez wrote: > nit: No need for this, since *position = foo will give a nice clean crash if > position is null. :D Done. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:64: delegate_->SetUpdateCallback(base::Bind(callback, base::Unretained(this))); On 2016/07/15 01:46:17, Wez wrote: > So we are able to assume that the caller will call SetUpdateCallback immediately > after we return this, before delegate_ can become null? > > (Not disagreeing, just confirming ;) There shouldn't be any lag between two SetUpdateCallback calls, but perhaps it would be prudent to add a check to see if the delegate is null in the first place? https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:21: // A provider of services needed by Geolocation. On 2016/07/15 01:46:17, Wez wrote: > nit: Suggest something more like "content::GeolocationDelegate implementation > integrated with the EngineGeolocationFeature." or similar. :) Done. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:29: bool UseNetworkLocationProviders() final { return false; } On 2016/07/15 01:46:17, Wez wrote: > BTW I had no idea we were using final in C++ now. Neat! Acknowledged. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:35: return base::WrapUnique(location_provider); On 2016/07/15 01:46:17, Wez wrote: > nit: Just put the new inside WrapUnique() Done. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:85: return new BlimpGeolocationDelegate(GetWeakPtr()); On 2016/07/15 01:46:17, Wez wrote: > Just call weak_factory_.GetweakPtr() directly here - no need for this > publicly-visible GetWeakPtr() method. Done. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.h (right): https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.h:29: content::GeolocationDelegate* CreateGeolocationDelegate(); On 2016/07/15 01:46:17, Wez wrote: > unique_ptr<..> here, please, so it's clear that the caller ends up owning it :) Done. Will follow up with CL that propagates that change through to the content layer. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.h:31: base::WeakPtr<EngineGeolocationFeature> GetWeakPtr(); On 2016/07/15 01:46:17, Wez wrote: > You don't need this method any more, which is nice because you no longer risk > some later CL (ab)using your WeakPtrs. Hurrah! Done. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.h:33: // BlimpLocationProvider::Delegate implementation. On 2016/07/15 01:46:17, Wez wrote: > nit: Since this is an implementation detail you can move these methods into the > private section (the inheritance of the Delegate interface has to stay public, > though). > > ProcessMessage is part of the actual public interface of the class, so would > need to stay in the public section. Done. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc:58: MATCHER_P(EqualsUpdatedListenState, accuracy, "") { On 2016/07/15 01:46:17, Wez wrote: > Looks like some of the naming in the tests need updating? Done. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc:105: TEST_F(EngineGeolocationFeatureTest, TestLocationReceived) { On 2016/07/15 01:46:18, Wez wrote: > nit: Don't prefix your test names with Test - the fixture name already ends in > Test, and this is clearly test code :) > > More generally, tests should focus on particular behaviours, not particular APIs > - it's a little trickier with this code since it's message-processing, so tests > for each message type does make more sense here. Done. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc:150: feature_.RequestAccuracy(GeolocationSetInterestLevelMessage::NO_INTEREST); On 2016/07/15 01:46:18, Wez wrote: > You could fold this into the preceding test. Done. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc:153: TEST_F(EngineGeolocationFeatureTest, TestSendRequestRefreshMessage) { On 2016/07/15 01:46:18, Wez wrote: > How about a test for EngineGeolocationFeature seeing some message that it isn't > expecting? Or a message that it is expecting, but that is malformed? :) I ended up just trying one of the unexpected Geolocation message types.
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) 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_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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lethalantidote@chromium.org 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...
Just a few remaining nits! :D https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:23: StopProvider(); What's the rationale for removing this call? Don't we want to stop getting location info if there is no LocationProvider? nit: This also reminds me that we are assuming that there is at-most-one BlimpLocationProvider active at any time (i.e. that the content layer will not create several running in parallel for some reason). We should think about whether to DCHECK for that somehow. https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:64: delegate_->SetUpdateCallback(base::Bind(callback, base::Unretained(this))); On 2016/07/18 21:11:56, CJ wrote: > On 2016/07/15 01:46:17, Wez wrote: > > So we are able to assume that the caller will call SetUpdateCallback > immediately > > after we return this, before delegate_ can become null? > > > > (Not disagreeing, just confirming ;) > > There shouldn't be any lag between two SetUpdateCallback calls, but perhaps it > would be prudent to add a check to see if the delegate is null in the first > place? That's what I'm getting at - are we able to assume that calling code looks like: provider = OverrideSystemLocationProvider(); provider->SetUpdateCallback(...); in which case this is correct, or do we need to support the caller creating the provider and handing posting a task or something, after which we might find |delegate_| as gone, before it calls SetUpdateCallback? https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc:153: TEST_F(EngineGeolocationFeatureTest, TestSendRequestRefreshMessage) { On 2016/07/18 21:11:56, CJ wrote: > On 2016/07/15 01:46:18, Wez wrote: > > How about a test for EngineGeolocationFeature seeing some message that it > isn't > > expecting? Or a message that it is expecting, but that is malformed? :) > > I ended up just trying one of the unexpected Geolocation message types. Acknowledged. https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:66: location_provider_->StopProvider(); nit: Add a test case for if the caller deletes the location provider without stopping it first? https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:100: EXPECT_CALL(*delegate_, SetUpdateCallback(_)).Times(1); I'd suggest having this test actually store the callback (you can find examples of gmock SaveArg something-or-other) so that you can verify that it gets correctly passed-through to the delegate. Testing that it's correctly-passed is a little tricky since it'll be a different callback, since the interfaces differ, but you can just call the callback that the delegate receives, and verify that the mock gets called with the right values, say. https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:104: TEST_F(BlimpLocationProviderTest, SetUpdateCallbackHandlesNullDelegate) { These test-cases look great. :) You could reasonably have had a single NullDelegate test and made all the calls in that one place, I think, but this is also fine. https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:91: DCHECK(!callback.is_null()); I think this DCHECK is actually incorrect - we allow null completion callbacks (though the header doesn't make that clear - we should fix that (in a separate CL!)). https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:119: callback.Run(net::OK); nit: You can put: base::ScopedClosureRunner run_callback(base::Bind(callback, net::OK)); at the top of the function to have callback.Run(OK) called at the end of the function, iff callback is non-null. :) https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/mock_blimp_location_provider_delegate.h (right): https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/mock_blimp_location_provider_delegate.h:27: base::WeakPtrFactory<MockBlimpLocationProviderDelegate> weak_factory_; nit: Make this private and provide a GetWeakPtr() method on the mock class. Also you need DISALLOW_COPY_AND_ASSIGN here. :)
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/2091023006/diff/200001/blimp/common/proto/com... File blimp/common/proto/common.proto (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/common/proto/com... blimp/common/proto/common.proto:5: // Message definitions shared between proto files. nit: between => across https://codereview.chromium.org/2091023006/diff/200001/blimp/common/proto/geo... File blimp/common/proto/geolocation.proto (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/common/proto/geo... blimp/common/proto/geolocation.proto:5: // Message definitions for geolocation messages. nit: This is already implied by the filename and file type. I suggest either removing the comment or adding detail about how these messages will be used, etc. https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/app/blimp... File blimp/engine/app/blimp_content_browser_client.cc (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/app/blimp... blimp/engine/app/blimp_content_browser_client.cc:46: content::GeolocationDelegate* no action required for this CL: I so wish this was unique_ptr :F https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:19: // A delegate that handles outgoing geolocation messages. nit: messages are N/A at this layer https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:30: const base::Callback<void(const content::Geoposition&)>& callback) = 0; suggestion: create an alias for the callback type with "using" https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:47: // The lifetime of the delegate relative to the BlimpLocationProvider nit: I think the lifetime comment is already implied by the use of a WeakPtr, as weak pointers are a defensive measure against this kind of uncertainty. Consider removing the comment https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:52: content::Geoposition position_; I think that we might be able to remove the comment if we called this cached_position_. https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:25: new BlimpLocationProvider(delegate_->weak_factory_.GetWeakPtr())) {} Oh - I see what's going on now. Move the weak_factory_ out of the mock object and into BlimpLocationProviderTest. Then delete the weak_factory_ object in your test. https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:29: void OnLocationUpdate(const content::LocationProvider* provider, I recommend making this a MOCK_METHOD2 and setting expectations for it. https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:116: DLOG(FATAL) << "Client sent unexpected message type."; Fail the connection by calling callback.Run(net::ERR_NOT_IMPLEMENTED); return; https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/mock_blimp_location_provider_delegate.h (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/mock_blimp_location_provider_delegate.h:27: base::WeakPtrFactory<MockBlimpLocationProviderDelegate> weak_factory_; Move this into the test https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/session/b... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.h:99: // Returns a pointer to the Geolocation feature. nit: comment is superfluous
https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:23: StopProvider(); On 2016/07/18 23:55:21, Wez wrote: > What's the rationale for removing this call? Don't we want to stop getting > location info if there is no LocationProvider? > > nit: This also reminds me that we are assuming that there is at-most-one > BlimpLocationProvider active at any time (i.e. that the content layer will not > create several running in parallel for some reason). We should think about > whether to DCHECK for that somehow. Due to my misunderstanding on how EXPECT_CALL worked, I removed it. It's back now, along with proper EXPECT_CALL's. Would that be handled here or rather in the LocationArbitrator? https://codereview.chromium.org/2091023006/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:64: delegate_->SetUpdateCallback(base::Bind(callback, base::Unretained(this))); On 2016/07/18 23:55:21, Wez wrote: > On 2016/07/18 21:11:56, CJ wrote: > > On 2016/07/15 01:46:17, Wez wrote: > > > So we are able to assume that the caller will call SetUpdateCallback > > immediately > > > after we return this, before delegate_ can become null? > > > > > > (Not disagreeing, just confirming ;) > > > > There shouldn't be any lag between two SetUpdateCallback calls, but perhaps it > > would be prudent to add a check to see if the delegate is null in the first > > place? > > That's what I'm getting at - are we able to assume that calling code looks like: > > provider = OverrideSystemLocationProvider(); > provider->SetUpdateCallback(...); > > in which case this is correct, or do we need to support the caller creating the > provider and handing posting a task or something, after which we might find > |delegate_| as gone, before it calls SetUpdateCallback? It seems pretty sequential, but a check was added anyway. https://cs.chromium.org/chromium/src/content/browser/geolocation/location_arb... https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:66: location_provider_->StopProvider(); On 2016/07/18 23:55:21, Wez wrote: > nit: Add a test case for if the caller deletes the location provider without > stopping it first? Done. https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:100: EXPECT_CALL(*delegate_, SetUpdateCallback(_)).Times(1); On 2016/07/18 23:55:21, Wez wrote: > I'd suggest having this test actually store the callback (you can find examples > of gmock SaveArg something-or-other) so that you can verify that it gets > correctly passed-through to the delegate. > > Testing that it's correctly-passed is a little tricky since it'll be a different > callback, since the interfaces differ, but you can just call the callback that > the delegate receives, and verify that the mock gets called with the right > values, say. Done. https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:104: TEST_F(BlimpLocationProviderTest, SetUpdateCallbackHandlesNullDelegate) { On 2016/07/18 23:55:21, Wez wrote: > These test-cases look great. :) You could reasonably have had a single > NullDelegate test and made all the calls in that one place, I think, but this is > also fine. Acknowledged. https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:91: DCHECK(!callback.is_null()); On 2016/07/18 23:55:21, Wez wrote: > I think this DCHECK is actually incorrect - we allow null completion callbacks > (though the header doesn't make that clear - we should fix that (in a separate > CL!)). Remove later then? Was removed until I read the next comment. Should this just be replaced with the function you suggested? https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:119: callback.Run(net::OK); On 2016/07/18 23:55:21, Wez wrote: > nit: You can put: > > base::ScopedClosureRunner run_callback(base::Bind(callback, net::OK)); > > at the top of the function to have callback.Run(OK) called at the end of the > function, iff callback is non-null. :) Little confused now. I thought we allow null callbacks, re your last comment. Is this an addition for the intermediate, before the cl to fix up the header goes through, or? https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/mock_blimp_location_provider_delegate.h (right): https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/mock_blimp_location_provider_delegate.h:27: base::WeakPtrFactory<MockBlimpLocationProviderDelegate> weak_factory_; On 2016/07/18 23:55:21, Wez wrote: > nit: Make this private and provide a GetWeakPtr() method on the mock class. > > Also you need DISALLOW_COPY_AND_ASSIGN here. :) Done.
https://codereview.chromium.org/2091023006/diff/200001/blimp/common/proto/com... File blimp/common/proto/common.proto (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/common/proto/com... blimp/common/proto/common.proto:5: // Message definitions shared between proto files. On 2016/07/19 16:01:41, Kevin M wrote: > nit: between => across Done. https://codereview.chromium.org/2091023006/diff/200001/blimp/common/proto/geo... File blimp/common/proto/geolocation.proto (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/common/proto/geo... blimp/common/proto/geolocation.proto:5: // Message definitions for geolocation messages. On 2016/07/19 16:01:41, Kevin M wrote: > nit: This is already implied by the filename and file type. I suggest either > removing the comment or adding detail about how these messages will be used, > etc. Removed. https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/app/blimp... File blimp/engine/app/blimp_content_browser_client.cc (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/app/blimp... blimp/engine/app/blimp_content_browser_client.cc:46: content::GeolocationDelegate* On 2016/07/19 16:01:41, Kevin M wrote: > no action required for this CL: I so wish this was unique_ptr :F Agreed. https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:19: // A delegate that handles outgoing geolocation messages. On 2016/07/19 16:01:41, Kevin M wrote: > nit: messages are N/A at this layer Done. https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:30: const base::Callback<void(const content::Geoposition&)>& callback) = 0; On 2016/07/19 16:01:41, Kevin M wrote: > suggestion: create an alias for the callback type with "using" Done. https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:47: // The lifetime of the delegate relative to the BlimpLocationProvider On 2016/07/19 16:01:41, Kevin M wrote: > nit: I think the lifetime comment is already implied by the use of a WeakPtr, as > weak pointers are a defensive measure against this kind of uncertainty. Consider > removing the comment Done. https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:52: content::Geoposition position_; On 2016/07/19 16:01:41, Kevin M wrote: > I think that we might be able to remove the comment if we called this > cached_position_. Done. https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:25: new BlimpLocationProvider(delegate_->weak_factory_.GetWeakPtr())) {} On 2016/07/19 16:01:42, Kevin M wrote: > Oh - I see what's going on now. > > Move the weak_factory_ out of the mock object and into > BlimpLocationProviderTest. Then delete the weak_factory_ object in your test. Is this referencing testing to see if BlimpLocationProvider can handle a null delegate? I thought that weak_factory_ objects reside in the class they are making weak pointers to? https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:29: void OnLocationUpdate(const content::LocationProvider* provider, On 2016/07/19 16:01:41, Kevin M wrote: > I recommend making this a MOCK_METHOD2 and setting expectations for it. Done. https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:116: DLOG(FATAL) << "Client sent unexpected message type."; On 2016/07/19 16:01:42, Kevin M wrote: > Fail the connection by calling callback.Run(net::ERR_NOT_IMPLEMENTED); return; Why ERR_NOT_IMPLEMENTED? https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/mock_blimp_location_provider_delegate.h (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/mock_blimp_location_provider_delegate.h:27: base::WeakPtrFactory<MockBlimpLocationProviderDelegate> weak_factory_; On 2016/07/19 16:01:42, Kevin M wrote: > Move this into the test Little confused about this suggestion. Why would one do that? https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/session/b... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.h:99: // Returns a pointer to the Geolocation feature. On 2016/07/19 16:01:42, Kevin M wrote: > nit: comment is superfluous 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: 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.
lgtm This CL is really nice now. https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:25: new BlimpLocationProvider(delegate_->weak_factory_.GetWeakPtr())) {} On 2016/07/19 20:35:58, CJ wrote: > On 2016/07/19 16:01:42, Kevin M wrote: > > Oh - I see what's going on now. > > > > Move the weak_factory_ out of the mock object and into > > BlimpLocationProviderTest. Then delete the weak_factory_ object in your test. > > Is this referencing testing to see if BlimpLocationProvider can handle a null > delegate? > I thought that weak_factory_ objects reside in the class they are making weak > pointers to? That is generally the case for production objects, but in test code, all that really matters is that the factory is deleted, not the object associated with the factory. So, we can pull out the factory into the test code, and simply delete it as necessary, so there's no need to add WeakPtr logic to the mock class. What's there is fine, though. https://codereview.chromium.org/2091023006/diff/260001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/260001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:65: EXPECT_CALL(*delegate_, RequestAccuracy(_)).Times(0); Consider using StrictMock so you don't have to check for these times=0 cases. https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:30: remove newline https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:66: delegate_.reset(); add newline to visually separate execution setup and code execution blocks, here and elsewhere in this test file https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:80: remove newline
https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/200001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:25: new BlimpLocationProvider(delegate_->weak_factory_.GetWeakPtr())) {} On 2016/07/21 17:37:05, Kevin M wrote: > On 2016/07/19 20:35:58, CJ wrote: > > On 2016/07/19 16:01:42, Kevin M wrote: > > > Oh - I see what's going on now. > > > > > > Move the weak_factory_ out of the mock object and into > > > BlimpLocationProviderTest. Then delete the weak_factory_ object in your > test. > > > > Is this referencing testing to see if BlimpLocationProvider can handle a null > > delegate? > > I thought that weak_factory_ objects reside in the class they are making weak > > pointers to? > > That is generally the case for production objects, but in test code, all that > really matters is that the factory is deleted, not the object associated with > the factory. So, we can pull out the factory into the test code, and simply > delete it as necessary, so there's no need to add WeakPtr logic to the mock > class. > > What's there is fine, though. Is it more acceptable or beneficial to do it the other way though? https://codereview.chromium.org/2091023006/diff/260001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/260001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:65: EXPECT_CALL(*delegate_, RequestAccuracy(_)).Times(0); On 2016/07/21 17:37:05, Kevin M wrote: > Consider using StrictMock so you don't have to check for these times=0 cases. StrictMock seems to not like when I reset the pointer. Any tips on that? Would your previous suggestion about the WeakPtrFactory help with this? https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:30: On 2016/07/21 17:37:05, Kevin M wrote: > remove newline Done. https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:66: delegate_.reset(); On 2016/07/21 17:37:06, Kevin M wrote: > add newline to visually separate execution setup and code execution blocks, here > and elsewhere in this test file Done. https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:80: On 2016/07/21 17:37:06, Kevin M wrote: > remove newline Done.
LGTM once the ProcessMessage null-callback handling is fixed and the start/stopprovider question resolved. https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:91: DCHECK(!callback.is_null()); On 2016/07/19 20:04:17, CJ wrote: > On 2016/07/18 23:55:21, Wez wrote: > > I think this DCHECK is actually incorrect - we allow null completion callbacks > > (though the header doesn't make that clear - we should fix that (in a separate > > CL!)). > > Remove later then? Was removed until I read the next comment. Should this just > be replaced with the function you suggested? We should not DCHECK here, in this CL. We can fix the header comments on MessageProcessor later. https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:119: callback.Run(net::OK); On 2016/07/19 20:04:17, CJ wrote: > On 2016/07/18 23:55:21, Wez wrote: > > nit: You can put: > > > > base::ScopedClosureRunner run_callback(base::Bind(callback, net::OK)); > > > > at the top of the function to have callback.Run(OK) called at the end of the > > function, iff callback is non-null. :) > > Little confused now. I thought we allow null callbacks, re your last comment. Is > this an addition for the intermediate, before the cl to fix up the header goes > through, or? ScopedClosureRunner only runs the callback if it is non-null - so if someone passes you a base::Callback and you just assign it to a ScopedClosureRunner then you should get the baheviour we want. if you prefer to call Run explicitly then just add an !is_null check here. https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:20: is_started_ = false; nit: Can we move these to be member initializers, plz? https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:43: if (delegate_ && is_started_) { Did we establish whether it's valid to call StopProvider() without having first called StartProvider()? If it's not valid, then I'd prefer DCHECK(is_started_) here, and an if (is_started_) check in the destructor before calling StopProvider(). :) https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:54: if (delegate_ && is_started_) { Again, is it actually valid for the caller to request a refresh for a not-started provider? That seems like a caller bug! https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:53: // Indicates whether a successful StartProvider call has occured. nit: Suggest "True if a ..." https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:55: // Occurs during tear down. nit: Suggest a little more clarifying wording, e.g: // BlimpLocationProvider implicitly stops on teardown, if it was started. https://codereview.chromium.org/2091023006/diff/300001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/300001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:146: callback.Run(position); nit: Can this be callback.Run(content::Geoposition()); ?
https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:91: DCHECK(!callback.is_null()); On 2016/07/22 01:18:58, Wez wrote: > On 2016/07/19 20:04:17, CJ wrote: > > On 2016/07/18 23:55:21, Wez wrote: > > > I think this DCHECK is actually incorrect - we allow null completion > callbacks > > > (though the header doesn't make that clear - we should fix that (in a > separate > > > CL!)). > > > > Remove later then? Was removed until I read the next comment. Should this just > > be replaced with the function you suggested? > > We should not DCHECK here, in this CL. > > We can fix the header comments on MessageProcessor later. Done. https://codereview.chromium.org/2091023006/diff/180001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:119: callback.Run(net::OK); On 2016/07/22 01:18:58, Wez wrote: > On 2016/07/19 20:04:17, CJ wrote: > > On 2016/07/18 23:55:21, Wez wrote: > > > nit: You can put: > > > > > > base::ScopedClosureRunner run_callback(base::Bind(callback, net::OK)); > > > > > > at the top of the function to have callback.Run(OK) called at the end of the > > > function, iff callback is non-null. :) > > > > Little confused now. I thought we allow null callbacks, re your last comment. > Is > > this an addition for the intermediate, before the cl to fix up the header goes > > through, or? > > ScopedClosureRunner only runs the callback if it is non-null - so if someone > passes you a base::Callback and you just assign it to a ScopedClosureRunner then > you should get the baheviour we want. > > if you prefer to call Run explicitly then just add an !is_null check here. Added the !is_null check https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:20: is_started_ = false; On 2016/07/22 01:18:58, Wez wrote: > nit: Can we move these to be member initializers, plz? Done. https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:43: if (delegate_ && is_started_) { On 2016/07/22 01:18:58, Wez wrote: > Did we establish whether it's valid to call StopProvider() without having first > called StartProvider()? > > If it's not valid, then I'd prefer DCHECK(is_started_) here, and an if > (is_started_) check in the destructor before calling StopProvider(). :) Done. https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:54: if (delegate_ && is_started_) { On 2016/07/22 01:18:58, Wez wrote: > Again, is it actually valid for the caller to request a refresh for a > not-started provider? That seems like a caller bug! Done. https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:53: // Indicates whether a successful StartProvider call has occured. On 2016/07/22 01:18:58, Wez wrote: > nit: Suggest "True if a ..." Done. https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/280001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:55: // Occurs during tear down. On 2016/07/22 01:18:58, Wez wrote: > nit: Suggest a little more clarifying wording, e.g: > > // BlimpLocationProvider implicitly stops on teardown, if it was started. Done. https://codereview.chromium.org/2091023006/diff/300001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/300001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:146: callback.Run(position); On 2016/07/22 01:18:58, Wez wrote: > nit: Can this be callback.Run(content::Geoposition()); ? Done.
LGTM w/ the test-case and null callback check addressed. I'd suggest also adding a test that your implementation copes with null callbacks for both expected and unexpected messages! https://codereview.chromium.org/2091023006/diff/320001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2091023006/diff/320001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:37: return false; nit: You could get rid of the return true above and simply return is_started_ here :) https://codereview.chromium.org/2091023006/diff/320001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/320001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:118: callback.Run(net::ERR_UNEXPECTED); You need the if (!callback_is_null()) here as well, I think? You could instead have e.g: int result = net::OK; switch (...) { ... case ...: result = net::ERR_UNEXPECTED; } if (...) callback.Run(result); https://codereview.chromium.org/2091023006/diff/320001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc (right): https://codereview.chromium.org/2091023006/diff/320001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc:82: EXPECT_EQ(net::ERR_UNEXPECTED, cb.WaitForResult()); nit: I would prefer that the callback and EXPECT are in the test itself, so it's clear to the reader how we're verifying the expected behaviour. Personally I'd have this function be CreateUnexpectedMessage() and then put these three lines in the test itself. :)
https://codereview.chromium.org/2091023006/diff/320001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2091023006/diff/320001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:37: return false; On 2016/07/22 22:09:44, Wez wrote: > nit: You could get rid of the return true above and simply return is_started_ > here :) Done. https://codereview.chromium.org/2091023006/diff/320001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2091023006/diff/320001/blimp/engine/feature/g... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:118: callback.Run(net::ERR_UNEXPECTED); On 2016/07/22 22:09:44, Wez wrote: > You need the if (!callback_is_null()) here as well, I think? > > You could instead have e.g: > > int result = net::OK; > switch (...) { > ... > case ...: > result = net::ERR_UNEXPECTED; > } > > if (...) callback.Run(result); Done.
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2091023006/#ps360001 (title: "Merge from Master")
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
Failed to apply patch for blimp/engine/app/blimp_content_browser_client.cc:
While running git apply --index -3 -p1;
error: patch failed: blimp/engine/app/blimp_content_browser_client.cc:6
Falling back to three-way merge...
Applied patch to 'blimp/engine/app/blimp_content_browser_client.cc' with
conflicts.
U blimp/engine/app/blimp_content_browser_client.cc
Patch: blimp/engine/app/blimp_content_browser_client.cc
Index: blimp/engine/app/blimp_content_browser_client.cc
diff --git a/blimp/engine/app/blimp_content_browser_client.cc
b/blimp/engine/app/blimp_content_browser_client.cc
index
90ea2f52e4c78637305c63ea9346ec75d5085f3b..6597e8feace127de1f004b84cca780ab78f5663f
100644
--- a/blimp/engine/app/blimp_content_browser_client.cc
+++ b/blimp/engine/app/blimp_content_browser_client.cc
@@ -6,32 +6,15 @@
#include "blimp/engine/app/blimp_browser_main_parts.h"
#include "blimp/engine/app/settings_manager.h"
#include "blimp/engine/feature/geolocation/blimp_location_provider.h"
+#include "blimp/engine/feature/geolocation/engine_geolocation_feature.h"
#include "blimp/engine/mojo/blob_channel_service.h"
+#include "blimp/engine/session/blimp_engine_session.h"
#include "content/public/browser/geolocation_delegate.h"
#include "services/shell/public/cpp/interface_registry.h"
namespace blimp {
namespace engine {
-namespace {
-// A provider of services needed by Geolocation.
-class BlimpGeolocationDelegate : public content::GeolocationDelegate {
- public:
- BlimpGeolocationDelegate() = default;
-
- bool UseNetworkLocationProviders() final { return false; }
-
- std::unique_ptr<content::LocationProvider> OverrideSystemLocationProvider()
- final {
- return base::WrapUnique(new BlimpLocationProvider());
- }
-
- private:
- DISALLOW_COPY_AND_ASSIGN(BlimpGeolocationDelegate);
-};
-
-} // anonymous namespace
-
BlimpContentBrowserClient::BlimpContentBrowserClient() {}
BlimpContentBrowserClient::~BlimpContentBrowserClient() {}
@@ -62,7 +45,10 @@ BlimpBrowserContext*
BlimpContentBrowserClient::GetBrowserContext() {
content::GeolocationDelegate*
BlimpContentBrowserClient::CreateGeolocationDelegate() {
- return new BlimpGeolocationDelegate();
+ EngineGeolocationFeature* geolocation_feature = blimp_browser_main_parts()
+ ->GetBlimpEngineSession()
+
->GetGeolocationFeature();
+ return geolocation_feature->CreateGeolocationDelegate().release();
}
void BlimpContentBrowserClient::ExposeInterfacesToRenderer(
https://codereview.chromium.org/2091023006/diff/360001/blimp/engine/session/b... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2091023006/diff/360001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.cc:8: #include <utility> nit: Do we need this extra include?
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2091023006/#ps380001 (title: "Merge")
The CQ bit was unchecked by lethalantidote@chromium.org
https://codereview.chromium.org/2091023006/diff/360001/blimp/engine/session/b... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2091023006/diff/360001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.cc:8: #include <utility> On 2016/07/25 22:24:57, Wez wrote: > nit: Do we need this extra include? Lint complains otherwise.
The CQ bit was checked by lethalantidote@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2091023006/diff/360001/blimp/engine/session/b... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2091023006/diff/360001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.cc:8: #include <utility> On 2016/07/25 22:52:46, CJ wrote: > On 2016/07/25 22:24:57, Wez wrote: > > nit: Do we need this extra include? > > Lint complains otherwise. OK, FYI I think Lint complains because e.g. std::move() is defined in <utility>, and we use it here. https://codereview.chromium.org/2091023006/diff/380001/blimp/engine/session/b... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2091023006/diff/380001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.cc:245: GetGeolocationFeature()->CreateGeolocationDelegate()); nit: Please put a blank line before this, to make it obvious, visually, that this is not related to the blob stuff immediately above. :) https://codereview.chromium.org/2091023006/diff/380001/blimp/engine/session/b... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2091023006/diff/380001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.h:99: EngineGeolocationFeature* GetGeolocationFeature(); I don't think you need this API any more, since you don't need to call to the Feature from the ContentBrowserClient impl?
https://codereview.chromium.org/2091023006/diff/360001/blimp/engine/session/b... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2091023006/diff/360001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.cc:8: #include <utility> On 2016/07/25 23:06:19, Wez wrote: > On 2016/07/25 22:52:46, CJ wrote: > > On 2016/07/25 22:24:57, Wez wrote: > > > nit: Do we need this extra include? > > > > Lint complains otherwise. > > OK, FYI I think Lint complains because e.g. std::move() is defined in <utility>, > and we use it here. Right. https://codereview.chromium.org/2091023006/diff/380001/blimp/engine/session/b... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2091023006/diff/380001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.cc:245: GetGeolocationFeature()->CreateGeolocationDelegate()); On 2016/07/25 23:06:19, Wez wrote: > nit: Please put a blank line before this, to make it obvious, visually, that > this is not related to the blob stuff immediately above. :) Done. https://codereview.chromium.org/2091023006/diff/380001/blimp/engine/session/b... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2091023006/diff/380001/blimp/engine/session/b... blimp/engine/session/blimp_engine_session.h:99: EngineGeolocationFeature* GetGeolocationFeature(); On 2016/07/25 23:06:19, Wez wrote: > I don't think you need this API any more, since you don't need to call to the > Feature from the ContentBrowserClient impl? Done.
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2091023006/#ps400001 (title: "Addresses Wez's #77 comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Great - thanks! On 25 July 2016 at 16:21, commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > CQ is trying da patch. Follow status at > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > https://codereview.chromium.org/2091023006/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lethalantidote@chromium.org 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.
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.
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2091023006/#ps420001 (title: "Merge")
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 EngineGeolocationFeature for Blimp Geolocation project. This change provides the engine-side classes needed to receive and send geolocation messages from the client. EngineGeolocationFeature sends messages from the engine that can either indicate the client-side to start sending location messages of the specified accuracy, request a refresh of the location, or indicate that the engine side no longer is listening for updates. The EngineGeolocationFeature also receives messages from the client, which can either provide a location or indicate an error. Other changes: *Moves EmptyMessage out of tab_control.proto to common.proto BUG=614486 ========== to ========== Adds EngineGeolocationFeature for Blimp Geolocation project. This change provides the engine-side classes needed to receive and send geolocation messages from the client. EngineGeolocationFeature sends messages from the engine that can either indicate the client-side to start sending location messages of the specified accuracy, request a refresh of the location, or indicate that the engine side no longer is listening for updates. The EngineGeolocationFeature also receives messages from the client, which can either provide a location or indicate an error. Other changes: *Moves EmptyMessage out of tab_control.proto to common.proto BUG=614486 ==========
Message was sent while issue was closed.
Committed patchset #22 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== Adds EngineGeolocationFeature for Blimp Geolocation project. This change provides the engine-side classes needed to receive and send geolocation messages from the client. EngineGeolocationFeature sends messages from the engine that can either indicate the client-side to start sending location messages of the specified accuracy, request a refresh of the location, or indicate that the engine side no longer is listening for updates. The EngineGeolocationFeature also receives messages from the client, which can either provide a location or indicate an error. Other changes: *Moves EmptyMessage out of tab_control.proto to common.proto BUG=614486 ========== to ========== Adds EngineGeolocationFeature for Blimp Geolocation project. This change provides the engine-side classes needed to receive and send geolocation messages from the client. EngineGeolocationFeature sends messages from the engine that can either indicate the client-side to start sending location messages of the specified accuracy, request a refresh of the location, or indicate that the engine side no longer is listening for updates. The EngineGeolocationFeature also receives messages from the client, which can either provide a location or indicate an error. Other changes: *Moves EmptyMessage out of tab_control.proto to common.proto BUG=614486 Committed: https://crrev.com/695c49437b5eb038e09e08c805f26ce7f7da253a Cr-Commit-Position: refs/heads/master@{#407934} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/695c49437b5eb038e09e08c805f26ce7f7da253a Cr-Commit-Position: refs/heads/master@{#407934} |
