Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2624843003:
Add support for cellular geolocation (Closed)
|
Created:
3 years, 11 months ago by can Skylar cook Modified:
3 years, 10 months ago Reviewers:
bartfab (slow), Alexander Alekseev, stevenjb, drustsmith, Steven Holte, joth, emaxx, Andrew T Wilson (Slow), stevenjb (google-dont-use), mcasas, Michael van Ouwerkerk, kumarniranjan, Ilya Sherman, Ben Chan CC:
stevenjb+watch_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for cellular geolocation
Shill cellular devices now collect and report cell tower information
(as of https://chromium-review.googlesource.com/#/c/412192/ ).
This makes geolocation_handler aware of the changes, and provides a mechanism
for consumers to query cell towers and wifi APNs separately. The
geolocation handler now plucks the keys of interest out of the
dictionary returned from shill's manager, rather than iterating and
assuming each entry corresponds to a wifi access point.
Use cell tower info in SimpleGeolocationProvider: it will now send
cell tower information if available and the setting enabled.
Cell towers and wifi APs are combined into a single management
setting (as cell tower information should be less sensitive than wifi
network information), but enables us to send them separately if so
desired.
BUG=b/30019266/
BUG=601174
TEST=Unittest verifying that requests are well-formed
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2624843003
Cr-Commit-Position: refs/heads/master@{#450183}
Committed: https://chromium.googlesource.com/chromium/src/+/eaf50cbe0129bf60335274cdea2b8b95354cd0d2
Patch Set 1 #Patch Set 2 : Add support for cellular geolocation #Patch Set 3 : Add support for cellular geolocation #Patch Set 4 : Add support for cellular geolocation #Patch Set 5 : Use cell tower info in SimpleGeolocationProvider. #
Total comments: 18
Patch Set 6 : Add support for cellular geolocation #
Total comments: 22
Patch Set 7 : address comments #
Total comments: 6
Patch Set 8 : Add SEND_ALL_NETWORK_INFO option for timezone detection #
Total comments: 5
Patch Set 9 : Remove GetCellTowers, fix indicator flag logic #
Total comments: 18
Patch Set 10 : Change verbiage, simplify network functions #
Total comments: 15
Patch Set 11 : Fix policy enum, cleanup browser policy checks #
Total comments: 5
Patch Set 12 : fix oopsies #Patch Set 13 : Update device tests to use correct dict key #Messages
Total messages: 61 (21 generated)
Description was changed from ========== Add support for cellular geolocation Shill cellular devices now collect and report cell tower information (as of https://chromium-review.googlesource.com/#/c/412192/ ). This makes geolocation_handler aware of the changes, and provides a mechanism for consumers to query cell towers and wifi APNs separately. The geolocation handler now plucks the keys of interest out of the dictionary returned from shill's manager, rather than iterating and testing each entry. BUG=b/30019266/ ========== to ========== Add support for cellular geolocation Shill cellular devices now collect and report cell tower information (as of https://chromium-review.googlesource.com/#/c/412192/ ). This makes geolocation_handler aware of the changes, and provides a mechanism for consumers to query cell towers and wifi APNs separately. The geolocation handler now plucks the keys of interest out of the dictionary returned from shill's manager, rather than iterating and testing each entry. Updating SimpleGeolocation in separate CL. BUG=b/30019266/ ==========
skylarc@google.com changed reviewers: + benchan@chromium.org, drustsmith@google.com, joth@chromium.org, kumarniranjan@google.com
Description was changed from ========== Add support for cellular geolocation Shill cellular devices now collect and report cell tower information (as of https://chromium-review.googlesource.com/#/c/412192/ ). This makes geolocation_handler aware of the changes, and provides a mechanism for consumers to query cell towers and wifi APNs separately. The geolocation handler now plucks the keys of interest out of the dictionary returned from shill's manager, rather than iterating and testing each entry. Updating SimpleGeolocation in separate CL. BUG=b/30019266/ ========== to ========== Add support for cellular geolocation Shill cellular devices now collect and report cell tower information (as of https://chromium-review.googlesource.com/#/c/412192/ ). This makes geolocation_handler aware of the changes, and provides a mechanism for consumers to query cell towers and wifi APNs separately. The geolocation handler now plucks the keys of interest out of the dictionary returned from shill's manager, rather than iterating and testing each entry. Updating SimpleGeolocation in separate CL. BUG=b/30019266/ BUG=601174 ==========
On 2017/02/02 20:22:27, can Skylar cook wrote: > Description was changed from > > ========== > Add support for cellular geolocation > > Shill cellular devices now collect and report cell tower information > (as of https://chromium-review.googlesource.com/#/c/412192/ ). This > makes geolocation_handler aware of the changes, and provides a mechanism > for consumers to query cell towers and wifi APNs separately. The > geolocation handler now plucks the keys of interest out of the > dictionary returned from shill's manager, rather than iterating and > testing each entry. Updating SimpleGeolocation in separate CL. > > BUG=b/30019266/ > ========== > > to > > ========== > Add support for cellular geolocation > > Shill cellular devices now collect and report cell tower information > (as of https://chromium-review.googlesource.com/#/c/412192/ ). This > makes geolocation_handler aware of the changes, and provides a mechanism > for consumers to query cell towers and wifi APNs separately. The > geolocation handler now plucks the keys of interest out of the > dictionary returned from shill's manager, rather than iterating and > testing each entry. Updating SimpleGeolocation in separate CL. > > BUG=b/30019266/ > BUG=601174 > ========== Well, I've failed at chromium. I had intended these to be separate CLs. Working on a fix...
https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... File chromeos/geolocation/simple_geolocation_provider.cc (right): https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... chromeos/geolocation/simple_geolocation_provider.cc:44: std::unique_ptr<CellTowerVector> result(new chromeos::CellTowerVector); auto result = base::MakeUnique<chromeos::CellTowerVector>(); https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... File chromeos/geolocation/simple_geolocation_request.cc (right): https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... chromeos/geolocation/simple_geolocation_request.cc:67: // const char kTimingAdvance[] = "timingAdvance"; either drop this line, or add a TODO for it commented-out code without comments would surely confuse someone someday. https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... chromeos/geolocation/simple_geolocation_request.cc:381: base::ListValue* cell_towers(new base::ListValue); keep it in unique_ptr auto cell_towers = base::MakeUnique<base::ListValue>(); https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... File chromeos/geolocation/simple_geolocation_unittest.cc (right): https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... chromeos/geolocation/simple_geolocation_unittest.cc:516: // Add an access point. access point -> cell tower? https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... chromeos/geolocation/simple_geolocation_unittest.cc:519: // Inititial call should return false and request cell towers. Initial? https://codereview.chromium.org/2624843003/diff/80001/chromeos/network/geoloc... File chromeos/network/geolocation_handler.cc (right): https://codereview.chromium.org/2624843003/diff/80001/chromeos/network/geoloc... chromeos/network/geolocation_handler.cc:67: return false; nit: use blank line to separate short if-statement without braces i.e. if (foo) return false; if (bar) return true; https://codereview.chromium.org/2624843003/diff/80001/chromeos/network/geoloc... chromeos/network/geolocation_handler.cc:175: if (device_type.compare(shill::kGeoWifiAccessPointsProperty) == 0) { why not use string::operator==? https://codereview.chromium.org/2624843003/diff/80001/chromeos/network/geoloc... File chromeos/network/geolocation_handler_unittest.cc (right): https://codereview.chromium.org/2624843003/diff/80001/chromeos/network/geoloc... chromeos/network/geolocation_handler_unittest.cc:52: return geolocation_handler_->GetCellTowers(&cell_towers_, NULL); nullptr https://codereview.chromium.org/2624843003/diff/80001/chromeos/network/geoloc... chromeos/network/geolocation_handler_unittest.cc:78: std::string lac = base::IntToString(idx * 10); these magic multiplications seem cryptic enough that need some more details in the comment.
https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... File chromeos/geolocation/simple_geolocation_provider.cc (right): https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... chromeos/geolocation/simple_geolocation_provider.cc:44: std::unique_ptr<CellTowerVector> result(new chromeos::CellTowerVector); On 2017/02/02 20:49:24, Ben Chan wrote: > auto result = base::MakeUnique<chromeos::CellTowerVector>(); Done. https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... File chromeos/geolocation/simple_geolocation_request.cc (right): https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... chromeos/geolocation/simple_geolocation_request.cc:67: // const char kTimingAdvance[] = "timingAdvance"; On 2017/02/02 20:49:24, Ben Chan wrote: > either drop this line, or add a TODO for it > > commented-out code without comments would surely confuse someone someday. Done. https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... chromeos/geolocation/simple_geolocation_request.cc:381: base::ListValue* cell_towers(new base::ListValue); On 2017/02/02 20:49:24, Ben Chan wrote: > keep it in unique_ptr > > auto cell_towers = base::MakeUnique<base::ListValue>(); Done. https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... File chromeos/geolocation/simple_geolocation_unittest.cc (right): https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... chromeos/geolocation/simple_geolocation_unittest.cc:516: // Add an access point. On 2017/02/02 20:49:25, Ben Chan wrote: > access point -> cell tower? Done. https://codereview.chromium.org/2624843003/diff/80001/chromeos/geolocation/si... chromeos/geolocation/simple_geolocation_unittest.cc:519: // Inititial call should return false and request cell towers. On 2017/02/02 20:49:25, Ben Chan wrote: > Initial? Done. https://codereview.chromium.org/2624843003/diff/80001/chromeos/network/geoloc... File chromeos/network/geolocation_handler.cc (right): https://codereview.chromium.org/2624843003/diff/80001/chromeos/network/geoloc... chromeos/network/geolocation_handler.cc:67: return false; On 2017/02/02 20:49:25, Ben Chan wrote: > nit: use blank line to separate short if-statement without braces > > i.e. > > if (foo) > return false; > > if (bar) > return true; Done. https://codereview.chromium.org/2624843003/diff/80001/chromeos/network/geoloc... chromeos/network/geolocation_handler.cc:175: if (device_type.compare(shill::kGeoWifiAccessPointsProperty) == 0) { On 2017/02/02 20:49:25, Ben Chan wrote: > why not use string::operator==? Because I'm still in (C++)-- mode haha. Done. https://codereview.chromium.org/2624843003/diff/80001/chromeos/network/geoloc... File chromeos/network/geolocation_handler_unittest.cc (right): https://codereview.chromium.org/2624843003/diff/80001/chromeos/network/geoloc... chromeos/network/geolocation_handler_unittest.cc:52: return geolocation_handler_->GetCellTowers(&cell_towers_, NULL); On 2017/02/02 20:49:25, Ben Chan wrote: > nullptr Done. https://codereview.chromium.org/2624843003/diff/80001/chromeos/network/geoloc... chromeos/network/geolocation_handler_unittest.cc:78: std::string lac = base::IntToString(idx * 10); On 2017/02/02 20:49:25, Ben Chan wrote: > these magic multiplications seem cryptic enough that need some more details in > the comment. Solely to differentiate the fields in a predictable way, open to better suggestions. Comment added.
Description was changed from ========== Add support for cellular geolocation Shill cellular devices now collect and report cell tower information (as of https://chromium-review.googlesource.com/#/c/412192/ ). This makes geolocation_handler aware of the changes, and provides a mechanism for consumers to query cell towers and wifi APNs separately. The geolocation handler now plucks the keys of interest out of the dictionary returned from shill's manager, rather than iterating and testing each entry. Updating SimpleGeolocation in separate CL. BUG=b/30019266/ BUG=601174 ========== to ========== Add support for cellular geolocation Shill cellular devices now collect and report cell tower information (as of https://chromium-review.googlesource.com/#/c/412192/ ). This makes geolocation_handler aware of the changes, and provides a mechanism for consumers to query cell towers and wifi APNs separately. The geolocation handler now plucks the keys of interest out of the dictionary returned from shill's manager, rather than iterating and assuming each entry corresponds to a wifi access point. Use cell tower info in SimpleGeolocationProvider: it will now send cell tower information if available and the setting enabled. Cell towers and wifi APs are combined into a single management setting (as cell tower information should be less sensitive than wifi network information), but enables us to send them separately if so desired. BUG=b/30019266/ BUG=601174 TEST=Unittest verifying that requests are well-formed ==========
skylarc@google.com changed reviewers: + alemate@chromium.org, stevenjb@chromium.org
Adding stevenjb@ for OWNER approval, as alemate@ is out until Monday (and I'm sure backlogged after vacation)
https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_request.cc (right): https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:48: // Top-level request data fields That would be nice :) In the meanwhile, we should convert all of these to use constexpr. https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:355: request->SetWithoutPathExpansion(kWifiAccessPoints, wifi_access_points); This is deprecated; use a unique_ptr to hold wifi_access_points and move it after appending all of the access points. https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:373: kSignalToNoiseRatio, access_point.signal_to_noise); nit: This would be more readable with the above put into a helper function, i.e. something like: wifi_access_points->Append(CreateAccessPointDictionary(access_point)); https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:380: auto cell_towers = base::MakeUnique<base::ListValue>(); Like this :) https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:398: } Ditto about moving the above to a helper function. https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_unittest.cc (right): https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_unittest.cc:73: const char kCellTower1MNC[] = "101"; constexpr https://codereview.chromium.org/2624843003/diff/100001/chromeos/network/geolo... File chromeos/network/geolocation_handler.cc (right): https://codereview.chromium.org/2624843003/diff/100001/chromeos/network/geolo... chromeos/network/geolocation_handler.cc:22: shill::kGeoCellTowersProperty}; constexpr https://codereview.chromium.org/2624843003/diff/100001/chromeos/network/geolo... chromeos/network/geolocation_handler.cc:164: const base::ListValue* entry_list = NULL; nullptr (feel free to fix everywhere in this file...) https://codereview.chromium.org/2624843003/diff/100001/chromeos/network/geolo... chromeos/network/geolocation_handler.cc:204: base::StringToInt(channel_str, &wap.channel); Looks like the above could be easily moved to a helper function? https://codereview.chromium.org/2624843003/diff/100001/chromeos/network/geolo... chromeos/network/geolocation_handler.cc:223: entry->GetString(shill::kGeoMobileNetworkCodeProperty, &ct.mnc); Helper function? https://codereview.chromium.org/2624843003/diff/100001/chromeos/network/geolo... File chromeos/network/geolocation_handler.h (right): https://codereview.chromium.org/2624843003/diff/100001/chromeos/network/geolo... chromeos/network/geolocation_handler.h:86: // Cellular enabled state nit: These comment's aren't helpful and this are not needed.
https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_request.cc (right): https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:48: // Top-level request data fields On 2017/02/03 02:12:02, stevenjb wrote: > That would be nice :) > In the meanwhile, we should convert all of these to use constexpr. Done. https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:355: request->SetWithoutPathExpansion(kWifiAccessPoints, wifi_access_points); On 2017/02/03 02:12:02, stevenjb wrote: > This is deprecated; use a unique_ptr to hold wifi_access_points and move it > after appending all of the access points. Whoops, meant to change that after Ben's comment on cellular below. Done. https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:373: kSignalToNoiseRatio, access_point.signal_to_noise); On 2017/02/03 02:12:02, stevenjb wrote: > nit: This would be more readable with the above put into a helper function, i.e. > something like: > wifi_access_points->Append(CreateAccessPointDictionary(access_point)); Done. https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:380: auto cell_towers = base::MakeUnique<base::ListValue>(); On 2017/02/03 02:12:02, stevenjb wrote: > Like this :) Acknowledged. https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:398: } On 2017/02/03 02:12:02, stevenjb wrote: > Ditto about moving the above to a helper function. Done. https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_unittest.cc (right): https://codereview.chromium.org/2624843003/diff/100001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_unittest.cc:73: const char kCellTower1MNC[] = "101"; On 2017/02/03 02:12:02, stevenjb wrote: > constexpr Done. https://codereview.chromium.org/2624843003/diff/100001/chromeos/network/geolo... File chromeos/network/geolocation_handler.cc (right): https://codereview.chromium.org/2624843003/diff/100001/chromeos/network/geolo... chromeos/network/geolocation_handler.cc:22: shill::kGeoCellTowersProperty}; On 2017/02/03 02:12:03, stevenjb wrote: > constexpr Done. https://codereview.chromium.org/2624843003/diff/100001/chromeos/network/geolo... chromeos/network/geolocation_handler.cc:164: const base::ListValue* entry_list = NULL; On 2017/02/03 02:12:02, stevenjb wrote: > nullptr (feel free to fix everywhere in this file...) Done. https://codereview.chromium.org/2624843003/diff/100001/chromeos/network/geolo... chromeos/network/geolocation_handler.cc:204: base::StringToInt(channel_str, &wap.channel); On 2017/02/03 02:12:02, stevenjb wrote: > Looks like the above could be easily moved to a helper function? Done. https://codereview.chromium.org/2624843003/diff/100001/chromeos/network/geolo... chromeos/network/geolocation_handler.cc:223: entry->GetString(shill::kGeoMobileNetworkCodeProperty, &ct.mnc); On 2017/02/03 02:12:02, stevenjb wrote: > Helper function? Done. https://codereview.chromium.org/2624843003/diff/100001/chromeos/network/geolo... File chromeos/network/geolocation_handler.h (right): https://codereview.chromium.org/2624843003/diff/100001/chromeos/network/geolo... chromeos/network/geolocation_handler.h:86: // Cellular enabled state On 2017/02/03 02:12:03, stevenjb wrote: > nit: These comment's aren't helpful and this are not needed. Done.
https://codereview.chromium.org/2624843003/diff/120001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_provider.cc (right): https://codereview.chromium.org/2624843003/diff/120001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_provider.cc:75: send_cell_towers ? GetCellTowerData() : nullptr)); This is not optimal, as both GetAccessPointData() / GetCellTowerData() initiate DBUS requests. Could you change API so that only one DBUS request is initiated here? May be we should initiate request by a separate call to geolocation handler, WDYT? https://codereview.chromium.org/2624843003/diff/120001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_request.cc (right): https://codereview.chromium.org/2624843003/diff/120001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:47: // TODO(skylarc): kill these and use dbus-constants instead? I think that dbus-constants could be used for DBUS requests only. But this are Google Maps API request/response keys. Should we move them to dbus-constants ?
Description was changed from ========== Add support for cellular geolocation Shill cellular devices now collect and report cell tower information (as of https://chromium-review.googlesource.com/#/c/412192/ ). This makes geolocation_handler aware of the changes, and provides a mechanism for consumers to query cell towers and wifi APNs separately. The geolocation handler now plucks the keys of interest out of the dictionary returned from shill's manager, rather than iterating and assuming each entry corresponds to a wifi access point. Use cell tower info in SimpleGeolocationProvider: it will now send cell tower information if available and the setting enabled. Cell towers and wifi APs are combined into a single management setting (as cell tower information should be less sensitive than wifi network information), but enables us to send them separately if so desired. BUG=b/30019266/ BUG=601174 TEST=Unittest verifying that requests are well-formed ========== to ========== Add support for cellular geolocation Shill cellular devices now collect and report cell tower information (as of https://chromium-review.googlesource.com/#/c/412192/ ). This makes geolocation_handler aware of the changes, and provides a mechanism for consumers to query cell towers and wifi APNs separately. The geolocation handler now plucks the keys of interest out of the dictionary returned from shill's manager, rather than iterating and assuming each entry corresponds to a wifi access point. Use cell tower info in SimpleGeolocationProvider: it will now send cell tower information if available and the setting enabled. Cell towers and wifi APs are combined into a single management setting (as cell tower information should be less sensitive than wifi network information), but enables us to send them separately if so desired. BUG=b/30019266/ BUG=601174 TEST=Unittest verifying that requests are well-formed CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Took a stab at adding the new policy setting. I believe the steps listed here are noops for us. https://sites.google.com/a/chromium.org/dev/developers/how-tos/enterprise/add... Asking alemate@ for a sanity check before adding a policy owner https://codereview.chromium.org/2624843003/diff/120001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_provider.cc (right): https://codereview.chromium.org/2624843003/diff/120001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_provider.cc:75: send_cell_towers ? GetCellTowerData() : nullptr)); On 2017/02/06 23:24:53, Alexander Alekseev (OOO) wrote: > This is not optimal, as both GetAccessPointData() / GetCellTowerData() initiate > DBUS requests. Could you change API so that only one DBUS request is initiated > here? May be we should initiate request by a separate call to geolocation > handler, WDYT? Good catch, agreed! I think it's useful to expose both methods in this class, but will streamline the usage here. https://codereview.chromium.org/2624843003/diff/120001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_request.cc (right): https://codereview.chromium.org/2624843003/diff/120001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:47: // TODO(skylarc): kill these and use dbus-constants instead? On 2017/02/06 23:24:53, Alexander Alekseev (OOO) wrote: > I think that dbus-constants could be used for DBUS requests only. > But this are Google Maps API request/response keys. > Should we move them to dbus-constants ? Good point -- they're technically *not* dbus constants, there's just currently overlap between the two sets of strings (dbus and geolocation keys). Since they're only used here, I think it's OK to have them defined in the file. If we begin sharing them, then we should definitely move to a constants file. Comment added to reflect this.
https://codereview.chromium.org/2624843003/diff/120001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_provider.cc (right): https://codereview.chromium.org/2624843003/diff/120001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_provider.cc:75: send_cell_towers ? GetCellTowerData() : nullptr)); On 2017/02/06 23:24:53, Alexander Alekseev wrote: > This is not optimal, as both GetAccessPointData() / GetCellTowerData() initiate > DBUS requests. Could you change API so that only one DBUS request is initiated > here? May be we should initiate request by a separate call to geolocation > handler, WDYT? That is a good point, each of these does send an identical request for geolocation data. It would be better to replace GeolocationHandler::GetWifiAccessPoints with a single method like GeolocationHandler::GetGeolocationData(wifi_access_points.get(), cell_tower_data.get(), &age_ms). The test for wifi/cellular_enabled can then be moved to GeolocationHander (and may be able to be removed from the public API).
https://codereview.chromium.org/2624843003/diff/120001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_request.cc (right): https://codereview.chromium.org/2624843003/diff/120001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:47: // TODO(skylarc): kill these and use dbus-constants instead? On 2017/02/07 21:32:39, can Skylar cook wrote: > On 2017/02/06 23:24:53, Alexander Alekseev (OOO) wrote: > > I think that dbus-constants could be used for DBUS requests only. > > But this are Google Maps API request/response keys. > > Should we move them to dbus-constants ? > > Good point -- they're technically *not* dbus constants, there's just currently > overlap between the two sets of strings (dbus and geolocation keys). Since > they're only used here, I think it's OK to have them defined in the file. If we > begin sharing them, then we should definitely move to a constants file. > > Comment added to reflect this. I agree that dbus-constants should be used for keys sent over dbus (i.e. keys provided by Shill) and other constants should be defined separately.
https://codereview.chromium.org/2624843003/diff/140001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_provider.cc (right): https://codereview.chromium.org/2624843003/diff/140001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_provider.cc:74: auto network_info = GetNetworkInfo(send_wifi_access_points, send_cell_towers); GetNetworkInfo() ignores individual values of send_wifi_access_points/send_cell_towers flags. So that if any of them are true, full network data will always be sent. Which is strange. https://codereview.chromium.org/2624843003/diff/140001/chromeos/network/geolo... File chromeos/network/geolocation_handler.h (right): https://codereview.chromium.org/2624843003/diff/140001/chromeos/network/geolo... chromeos/network/geolocation_handler.h:59: bool GetCellTowers(CellTowerVector* cell_towers, int64_t* age_ms); It looks like GetWifiAccessPoints()/GetCellTowers() are only used in tests. Could we update tests to use GetNetworkInformation() instead (for example, by allowing nullptr in place of unneeded data) ? Alternatively, we can rename these to GetWifiAccessPointsForTesting()/GetCellTowersForTesting(), but they seem to be a bit complicate for test methods.
https://codereview.chromium.org/2624843003/diff/140001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_provider.cc (right): https://codereview.chromium.org/2624843003/diff/140001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_provider.cc:74: auto network_info = GetNetworkInfo(send_wifi_access_points, send_cell_towers); On 2017/02/07 22:15:38, Alexander Alekseev wrote: > GetNetworkInfo() ignores individual values of > send_wifi_access_points/send_cell_towers flags. So that if any of them are true, > full network data will always be sent. Which is strange. Whoops, good catch. Fixed. https://codereview.chromium.org/2624843003/diff/140001/chromeos/network/geolo... File chromeos/network/geolocation_handler.h (right): https://codereview.chromium.org/2624843003/diff/140001/chromeos/network/geolo... chromeos/network/geolocation_handler.h:59: bool GetCellTowers(CellTowerVector* cell_towers, int64_t* age_ms); On 2017/02/07 22:15:38, Alexander Alekseev wrote: > It looks like GetWifiAccessPoints()/GetCellTowers() are only used in tests. > > Could we update tests to use GetNetworkInformation() instead (for example, by > allowing nullptr in place of unneeded data) ? > > Alternatively, we can rename these to > GetWifiAccessPointsForTesting()/GetCellTowersForTesting(), but they seem to be a > bit complicate for test methods. GetWifiAccessPoints() is used in a few locations, notably in WifiDataProviderChromeOs, where the age value is actually used. After talking this out with Steven, I'm going to drop GetCellTowers() and do as you suggest for tests (use GetNetworkInformation()).
https://codereview.chromium.org/2624843003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/system/timezone_resolver_manager.cc (right): https://codereview.chromium.org/2624843003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/system/timezone_resolver_manager.cc:134: enterprise_management::SystemTimezoneProto::SEND_WIFI_ACCESS_POINTS); Should this be | SEND_ALL_NETWORK_INFO ? https://codereview.chromium.org/2624843003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/system/timezone_resolver_manager.cc:142: bool TimeZoneResolverManager::CheckTimezoneManagementSetting(int expected_policy_value){ clang format https://codereview.chromium.org/2624843003/diff/160001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_provider.cc (right): https://codereview.chromium.org/2624843003/diff/160001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_provider.cc:29: WifiCellTowerPair GetNetworkInfo(bool send_wifi_access_points, use unique_ptr<WifiCellTowerPair> (also, see note below) https://codereview.chromium.org/2624843003/diff/160001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_provider.cc:74: auto network_info = GetNetworkInfo(send_wifi_access_points, send_cell_towers); We tend to avoid this use of auto since it isn't obvious what is returned. e.g. it's not obvious that this (currently) returns a struct and not an unowned pointer (which would be bad). Declaring this as a unique_ptr<WifiCellTowerPair> will make ownership crystal clear. https://codereview.chromium.org/2624843003/diff/160001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_provider.cc:79: std::move(network_info.cell_vector))); It would be more clear to make WifiCellTowerPair a member of SimpleGeolocationRequest and then we can pass |network_info| to SimpleGeolocationRequest(). Right now we end up with a struct containing empty vectors, which is a little bit awkward. Alternately, just inline GetNetworkInfo, then you don't need the awkward struct at all. https://codereview.chromium.org/2624843003/diff/160001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_request.cc (right): https://codereview.chromium.org/2624843003/diff/160001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:410: } This doesn't appear to need to be a member function? If it can be a local function in an anonymous namespace that would be preferred. https://codereview.chromium.org/2624843003/diff/160001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:430: } Ditto. https://codereview.chromium.org/2624843003/diff/160001/chromeos/network/geolo... File chromeos/network/geolocation_handler.cc (right): https://codereview.chromium.org/2624843003/diff/160001/chromeos/network/geolo... chromeos/network/geolocation_handler.cc:217: access_point_received_time_ = base::Time::Now(); This is redundant and a bit confusing. We should set this (and cell_towers_received_time_) once at the end of GeolocationCallback if the corresponding vector is not empty. Alternately, we can just use a single received_time here. This represents whether or not we have received any data from Shill (and when). GetNetworksForGeolocation() does not specify "just wifi", or "just cellular", so we can assume that we have all of the current data, there is no reason to track these separately. I think this would be more clear with a single received_time. https://codereview.chromium.org/2624843003/diff/160001/chromeos/network/geolo... File chromeos/network/geolocation_handler.h (right): https://codereview.chromium.org/2624843003/diff/160001/chromeos/network/geolo... chromeos/network/geolocation_handler.h:55: bool cellular_enabled() const { return cellular_enabled_; } Is this used now?
Comments addressed, updated verbiage as per launch tracker issue: https://bugs.chromium.org/p/chromium/issues/detail?id=685373#c8 https://codereview.chromium.org/2624843003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/system/timezone_resolver_manager.cc (right): https://codereview.chromium.org/2624843003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/system/timezone_resolver_manager.cc:134: enterprise_management::SystemTimezoneProto::SEND_WIFI_ACCESS_POINTS); On 2017/02/07 23:54:29, stevenjb wrote: > Should this be | SEND_ALL_NETWORK_INFO ? Done. https://codereview.chromium.org/2624843003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/system/timezone_resolver_manager.cc:142: bool TimeZoneResolverManager::CheckTimezoneManagementSetting(int expected_policy_value){ On 2017/02/07 23:54:29, stevenjb wrote: > clang format Done. https://codereview.chromium.org/2624843003/diff/160001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_provider.cc (right): https://codereview.chromium.org/2624843003/diff/160001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_provider.cc:29: WifiCellTowerPair GetNetworkInfo(bool send_wifi_access_points, On 2017/02/07 23:54:29, stevenjb wrote: > use unique_ptr<WifiCellTowerPair> (also, see note below) Done. https://codereview.chromium.org/2624843003/diff/160001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_provider.cc:74: auto network_info = GetNetworkInfo(send_wifi_access_points, send_cell_towers); On 2017/02/07 23:54:29, stevenjb wrote: > We tend to avoid this use of auto since it isn't obvious what is returned. e.g. > it's not obvious that this (currently) returns a struct and not an unowned > pointer (which would be bad). Declaring this as a unique_ptr<WifiCellTowerPair> > will make ownership crystal clear. Acknowledged. https://codereview.chromium.org/2624843003/diff/160001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_provider.cc:79: std::move(network_info.cell_vector))); On 2017/02/07 23:54:29, stevenjb wrote: > It would be more clear to make WifiCellTowerPair a member of > SimpleGeolocationRequest and then we can pass |network_info| to > SimpleGeolocationRequest(). Right now we end up with a struct containing empty > vectors, which is a little bit awkward. > > Alternately, just inline GetNetworkInfo, then you don't need the awkward struct > at all. Done. https://codereview.chromium.org/2624843003/diff/160001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_request.cc (right): https://codereview.chromium.org/2624843003/diff/160001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:410: } On 2017/02/07 23:54:29, stevenjb wrote: > This doesn't appear to need to be a member function? If it can be a local > function in an anonymous namespace that would be preferred. Done. https://codereview.chromium.org/2624843003/diff/160001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:430: } On 2017/02/07 23:54:29, stevenjb wrote: > Ditto. Done. https://codereview.chromium.org/2624843003/diff/160001/chromeos/network/geolo... File chromeos/network/geolocation_handler.cc (right): https://codereview.chromium.org/2624843003/diff/160001/chromeos/network/geolo... chromeos/network/geolocation_handler.cc:217: access_point_received_time_ = base::Time::Now(); On 2017/02/07 23:54:29, stevenjb wrote: > This is redundant and a bit confusing. We should set this (and > cell_towers_received_time_) once at the end of GeolocationCallback if the > corresponding vector is not empty. > > Alternately, we can just use a single received_time here. This represents > whether or not we have received any data from Shill (and when). > GetNetworksForGeolocation() does not specify "just wifi", or "just cellular", so > we can assume that we have all of the current data, there is no reason to track > these separately. I think this would be more clear with a single received_time. Done. https://codereview.chromium.org/2624843003/diff/160001/chromeos/network/geolo... File chromeos/network/geolocation_handler.h (right): https://codereview.chromium.org/2624843003/diff/160001/chromeos/network/geolo... chromeos/network/geolocation_handler.h:55: bool cellular_enabled() const { return cellular_enabled_; } On 2017/02/07 23:54:29, stevenjb wrote: > Is this used now? Done.
Description was changed from ========== Add support for cellular geolocation Shill cellular devices now collect and report cell tower information (as of https://chromium-review.googlesource.com/#/c/412192/ ). This makes geolocation_handler aware of the changes, and provides a mechanism for consumers to query cell towers and wifi APNs separately. The geolocation handler now plucks the keys of interest out of the dictionary returned from shill's manager, rather than iterating and assuming each entry corresponds to a wifi access point. Use cell tower info in SimpleGeolocationProvider: it will now send cell tower information if available and the setting enabled. Cell towers and wifi APs are combined into a single management setting (as cell tower information should be less sensitive than wifi network information), but enables us to send them separately if so desired. BUG=b/30019266/ BUG=601174 TEST=Unittest verifying that requests are well-formed CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add support for cellular geolocation Shill cellular devices now collect and report cell tower information (as of https://chromium-review.googlesource.com/#/c/412192/ ). This makes geolocation_handler aware of the changes, and provides a mechanism for consumers to query cell towers and wifi APNs separately. The geolocation handler now plucks the keys of interest out of the dictionary returned from shill's manager, rather than iterating and assuming each entry corresponds to a wifi access point. Use cell tower info in SimpleGeolocationProvider: it will now send cell tower information if available and the setting enabled. Cell towers and wifi APs are combined into a single management setting (as cell tower information should be less sensitive than wifi network information), but enables us to send them separately if so desired. BUG=b/30019266/ BUG=601174 TEST=Unittest verifying that requests are well-formed CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
skylarc@google.com changed reviewers: + atwilson@chromium.org, bartfab@chromium.org
skylarc@google.com changed reviewers: + emaxx@chromium.org
Adding a few policy owners: atwilson, bartfab, and emaxx. Can you PTAL at the policy change? Are additional steps necessary?
Adding a few policy owners: atwilson, bartfab, and emaxx. Can you PTAL at the policy change? Are additional steps necessary?
On 2017/02/08 21:59:03, can Skylar cook wrote: > Adding a few policy owners: atwilson, bartfab, and emaxx. > > Can you PTAL at the policy change? Are additional steps necessary? A note for policy reviewers: setting new value for chrome < 58 will turn the feature off completely and add a warning message about it in logs. Is there a way to limit new value to >= M58 ?
https://codereview.chromium.org/2624843003/diff/140001/chromeos/network/geolo... File chromeos/network/geolocation_handler.h (right): https://codereview.chromium.org/2624843003/diff/140001/chromeos/network/geolo... chromeos/network/geolocation_handler.h:59: bool GetCellTowers(CellTowerVector* cell_towers, int64_t* age_ms); On 2017/02/07 22:29:59, can Skylar cook wrote: > On 2017/02/07 22:15:38, Alexander Alekseev wrote: > > It looks like GetWifiAccessPoints()/GetCellTowers() are only used in tests. > > > > Could we update tests to use GetNetworkInformation() instead (for example, by > > allowing nullptr in place of unneeded data) ? > > > > Alternatively, we can rename these to > > GetWifiAccessPointsForTesting()/GetCellTowersForTesting(), but they seem to be > a > > bit complicate for test methods. > > GetWifiAccessPoints() is used in a few locations, notably in > WifiDataProviderChromeOs, where the age value is actually used. > > After talking this out with Steven, I'm going to drop GetCellTowers() and do as > you suggest for tests (use GetNetworkInformation()). WifiDataProviderChromeOs is a completely different code which is used for Web location only. ChromeOS geolocation code is separate and it does not seem to use this ;)
On 2017/02/08 22:10:32, Alexander Alekseev wrote: > https://codereview.chromium.org/2624843003/diff/140001/chromeos/network/geolo... > File chromeos/network/geolocation_handler.h (right): > > https://codereview.chromium.org/2624843003/diff/140001/chromeos/network/geolo... > chromeos/network/geolocation_handler.h:59: bool GetCellTowers(CellTowerVector* > cell_towers, int64_t* age_ms); > On 2017/02/07 22:29:59, can Skylar cook wrote: > > On 2017/02/07 22:15:38, Alexander Alekseev wrote: > > > It looks like GetWifiAccessPoints()/GetCellTowers() are only used in tests. > > > > > > Could we update tests to use GetNetworkInformation() instead (for example, > by > > > allowing nullptr in place of unneeded data) ? > > > > > > Alternatively, we can rename these to > > > GetWifiAccessPointsForTesting()/GetCellTowersForTesting(), but they seem to > be > > a > > > bit complicate for test methods. > > > > GetWifiAccessPoints() is used in a few locations, notably in > > WifiDataProviderChromeOs, where the age value is actually used. > > > > After talking this out with Steven, I'm going to drop GetCellTowers() and do > as > > you suggest for tests (use GetNetworkInformation()). > > WifiDataProviderChromeOs is a completely different code which is used for Web > location only. ChromeOS geolocation code is separate and it does not seem to use > this ;) AIUI this is the same method: https://cs.chromium.org/chromium/src/device/geolocation/wifi_data_provider_ch...
On 2017/02/08 22:12:28, can Skylar cook wrote: > On 2017/02/08 22:10:32, Alexander Alekseev wrote: > > > https://codereview.chromium.org/2624843003/diff/140001/chromeos/network/geolo... > > File chromeos/network/geolocation_handler.h (right): > > > > > https://codereview.chromium.org/2624843003/diff/140001/chromeos/network/geolo... > > chromeos/network/geolocation_handler.h:59: bool GetCellTowers(CellTowerVector* > > cell_towers, int64_t* age_ms); > > On 2017/02/07 22:29:59, can Skylar cook wrote: > > > On 2017/02/07 22:15:38, Alexander Alekseev wrote: > > > > It looks like GetWifiAccessPoints()/GetCellTowers() are only used in > tests. > > > > > > > > Could we update tests to use GetNetworkInformation() instead (for example, > > by > > > > allowing nullptr in place of unneeded data) ? > > > > > > > > Alternatively, we can rename these to > > > > GetWifiAccessPointsForTesting()/GetCellTowersForTesting(), but they seem > to > > be > > > a > > > > bit complicate for test methods. > > > > > > GetWifiAccessPoints() is used in a few locations, notably in > > > WifiDataProviderChromeOs, where the age value is actually used. > > > > > > After talking this out with Steven, I'm going to drop GetCellTowers() and do > > as > > > you suggest for tests (use GetNetworkInformation()). > > > > WifiDataProviderChromeOs is a completely different code which is used for Web > > location only. ChromeOS geolocation code is separate and it does not seem to > use > > this ;) > > AIUI this is the same method: > https://cs.chromium.org/chromium/src/device/geolocation/wifi_data_provider_ch... Yes, you are right. There is a common layer. Sorry for confusion.
lgtm with nits. https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_request.cc (right): https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:44: // The full request text. (no parameters are supported by now) nit: Change this comment to something like: "This request is used if sending location signals (like WiFi AP, Cell tower information, etc) is disabled." https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_unittest.cc (right): https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_unittest.cc:457: EXPECT_TRUE(GetWifiAccessPoints()); Could we also Add Cell tower here and check that it is not sent to the server since we supply false to "send cell towers" parameter of RequestGeolocation() ?
stevenjb@google.com changed reviewers: + stevenjb@google.com
looks good, just a couple more suggestions. https://codereview.chromium.org/2624843003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/system/timezone_resolver_manager.cc (right): https://codereview.chromium.org/2624843003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/system/timezone_resolver_manager.cc:158: AutomaticTimezoneDetectionType_MAX); Sorry, I should have suggested this the first time, but calling this twice does a fair bit of duplicate work. You could make this return |policy_value| instead and do the compare in Should...(). https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_provider.cc (right): https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_provider.cc:50: cell_vector = nullptr; No need to do this explicitly, just only make the call to eolocation_handler()->GetNetworkInformation() if (send_wifi_access_points || send_cell_towers), then the logic below will handle setting these to nullptr. https://codereview.chromium.org/2624843003/diff/180001/chromeos/network/geolo... File chromeos/network/geolocation_handler.cc (right): https://codereview.chromium.org/2624843003/diff/180001/chromeos/network/geolo... chromeos/network/geolocation_handler.cc:182: geolocation_received_time_ = base::Time::Now(); move this outside the outer for loop.
policy LGTM with one nit. You will need to also file a buganizer issue to track updating the CPanel UI to reflect this new setting - this would be a good place to also capture the requirement that the new setting not get sent down to devices < M58. https://codereview.chromium.org/2624843003/diff/180001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2624843003/diff/180001/components/policy/reso... components/policy/resources/policy_templates.json:8814: 'enum': [ 0, 1, 2, 3 ], Is this enum attribute correct?
https://codereview.chromium.org/2624843003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/system/timezone_resolver_manager.cc (right): https://codereview.chromium.org/2624843003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/system/timezone_resolver_manager.cc:158: AutomaticTimezoneDetectionType_MAX); On 2017/02/09 00:13:28, stevenjb (google-dont-use) wrote: > Sorry, I should have suggested this the first time, but calling this twice does > a fair bit of duplicate work. You could make this return |policy_value| instead > and do the compare in Should...(). Done. https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_provider.cc (right): https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_provider.cc:50: cell_vector = nullptr; On 2017/02/09 00:13:28, stevenjb (google-dont-use) wrote: > No need to do this explicitly, just only make the call to > eolocation_handler()->GetNetworkInformation() if (send_wifi_access_points || > send_cell_towers), then the logic below will handle setting these to nullptr. Ah, clever. Done. https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_request.cc (right): https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_request.cc:44: // The full request text. (no parameters are supported by now) On 2017/02/08 22:43:55, Alexander Alekseev wrote: > nit: Change this comment to something like: "This request is used if sending > location signals (like WiFi AP, Cell tower information, etc) is disabled." Done. https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_unittest.cc (right): https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_unittest.cc:457: EXPECT_TRUE(GetWifiAccessPoints()); On 2017/02/08 22:43:55, Alexander Alekseev wrote: > Could we also Add Cell tower here and check that it is not sent to the server > since we supply false to "send cell towers" parameter of RequestGeolocation() ? Done. https://codereview.chromium.org/2624843003/diff/180001/chromeos/network/geolo... File chromeos/network/geolocation_handler.cc (right): https://codereview.chromium.org/2624843003/diff/180001/chromeos/network/geolo... chromeos/network/geolocation_handler.cc:182: geolocation_received_time_ = base::Time::Now(); On 2017/02/09 00:13:29, stevenjb (google-dont-use) wrote: > move this outside the outer for loop. Done. https://codereview.chromium.org/2624843003/diff/180001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2624843003/diff/180001/components/policy/reso... components/policy/resources/policy_templates.json:8814: 'enum': [ 0, 1, 2, 3 ], On 2017/02/09 09:32:47, Andrew T Wilson (Slow) wrote: > Is this enum attribute correct? Nope! Missed this line, thanks.
https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_unittest.cc (right): https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_unittest.cc:457: EXPECT_TRUE(GetWifiAccessPoints()); On 2017/02/09 19:44:30, can Skylar cook wrote: > On 2017/02/08 22:43:55, Alexander Alekseev wrote: > > Could we also Add Cell tower here and check that it is not sent to the server > > since we supply false to "send cell towers" parameter of RequestGeolocation() > ? > > Done. I cannot find a check itself here, like: EXPECT_FALSE(GetCellTowers());
https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_unittest.cc (right): https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_unittest.cc:457: EXPECT_TRUE(GetWifiAccessPoints()); On 2017/02/09 19:54:56, Alexander Alekseev wrote: > On 2017/02/09 19:44:30, can Skylar cook wrote: > > On 2017/02/08 22:43:55, Alexander Alekseev wrote: > > > Could we also Add Cell tower here and check that it is not sent to the > server > > > since we supply false to "send cell towers" parameter of > RequestGeolocation() > > ? > > > > Done. > > I cannot find a check itself here, like: > EXPECT_FALSE(GetCellTowers()); The `EXPECT_EQ(kOneWiFiAPRequestBody, requests_monitor.last_request_body())` below is the definitive test for what we care about: does the sent request only contain the WiFi AP?
https://codereview.chromium.org/2624843003/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/system/timezone_resolver_manager.cc (right): https://codereview.chromium.org/2624843003/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/system/timezone_resolver_manager.cc:137: enterprise_management::SystemTimezoneProto::SEND_ALL_LOCATION_INFO)); This still calls GetTimezoneManagementSetting() twice. You need a local here. https://codereview.chromium.org/2624843003/diff/200001/chromeos/network/geolo... File chromeos/network/geolocation_handler.cc (right): https://codereview.chromium.org/2624843003/diff/200001/chromeos/network/geolo... chromeos/network/geolocation_handler.cc:183: geolocation_received_time_ = base::Time::Now(); You're still doing this twice, it only needs to be done once at the end of the function. https://codereview.chromium.org/2624843003/diff/200001/chromeos/network/geolo... File chromeos/network/geolocation_handler.h (right): https://codereview.chromium.org/2624843003/diff/200001/chromeos/network/geolo... chromeos/network/geolocation_handler.h:91: // Cached netork information and update time network
https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... File chromeos/geolocation/simple_geolocation_unittest.cc (right): https://codereview.chromium.org/2624843003/diff/180001/chromeos/geolocation/s... chromeos/geolocation/simple_geolocation_unittest.cc:457: EXPECT_TRUE(GetWifiAccessPoints()); On 2017/02/09 19:59:35, can Skylar cook wrote: > On 2017/02/09 19:54:56, Alexander Alekseev wrote: > > On 2017/02/09 19:44:30, can Skylar cook wrote: > > > On 2017/02/08 22:43:55, Alexander Alekseev wrote: > > > > Could we also Add Cell tower here and check that it is not sent to the > > server > > > > since we supply false to "send cell towers" parameter of > > RequestGeolocation() > > > ? > > > > > > Done. > > > > I cannot find a check itself here, like: > > EXPECT_FALSE(GetCellTowers()); > > The `EXPECT_EQ(kOneWiFiAPRequestBody, requests_monitor.last_request_body())` > below is the definitive test for what we care about: does the sent request only > contain the WiFi AP? Yes, thank you, I missed this.
https://codereview.chromium.org/2624843003/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/system/timezone_resolver_manager.cc (right): https://codereview.chromium.org/2624843003/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/system/timezone_resolver_manager.cc:137: enterprise_management::SystemTimezoneProto::SEND_ALL_LOCATION_INFO)); On 2017/02/09 20:12:52, stevenjb wrote: > This still calls GetTimezoneManagementSetting() twice. You need a local here. D'oh https://codereview.chromium.org/2624843003/diff/200001/chromeos/network/geolo... File chromeos/network/geolocation_handler.cc (right): https://codereview.chromium.org/2624843003/diff/200001/chromeos/network/geolo... chromeos/network/geolocation_handler.cc:183: geolocation_received_time_ = base::Time::Now(); On 2017/02/09 20:12:52, stevenjb wrote: > You're still doing this twice, it only needs to be done once at the end of the > function. Done. I assumed we only wanted to update if we found one of the expected properties, I should have read the existing code more thoroughly.
lgtm
The CQ bit was checked by skylarc@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from alemate@chromium.org, atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/2624843003/#ps220001 (title: "fix oopsies")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
skylarc@google.com changed reviewers: + holte@chromium.org, isherman@chromium.org
Adding OWNERS for histograms, Steven and Ilya can you PTAL?
histograms lgtm
The CQ bit was checked by skylarc@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
skylarc@google.com changed reviewers: + mcasas@chromium.org, mvanouwerkerk@chromium.org
mcasas or mvanouwerkerk : PTAL at the device tests -- we're now fetching locations by key from a dictionary and this updates the test to use the key *actually* sent by shill / chromeos.
On 2017/02/13 22:38:44, can Skylar cook wrote: > mcasas or mvanouwerkerk : > > PTAL at the device tests -- we're now fetching locations by key from a > dictionary and this updates the test to use the key *actually* sent by shill / > chromeos. lgtm since this is OS- and test- specific and tiny.
The CQ bit was checked by skylarc@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from alemate@chromium.org, stevenjb@chromium.org, atwilson@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2624843003/#ps240001 (title: "Update device tests to use correct dict key")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1487026681372630,
"parent_rev": "c767e3a0419746e45d73fa891ee9291a3a216a8e", "commit_rev":
"eaf50cbe0129bf60335274cdea2b8b95354cd0d2"}
Message was sent while issue was closed.
Description was changed from ========== Add support for cellular geolocation Shill cellular devices now collect and report cell tower information (as of https://chromium-review.googlesource.com/#/c/412192/ ). This makes geolocation_handler aware of the changes, and provides a mechanism for consumers to query cell towers and wifi APNs separately. The geolocation handler now plucks the keys of interest out of the dictionary returned from shill's manager, rather than iterating and assuming each entry corresponds to a wifi access point. Use cell tower info in SimpleGeolocationProvider: it will now send cell tower information if available and the setting enabled. Cell towers and wifi APs are combined into a single management setting (as cell tower information should be less sensitive than wifi network information), but enables us to send them separately if so desired. BUG=b/30019266/ BUG=601174 TEST=Unittest verifying that requests are well-formed CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add support for cellular geolocation Shill cellular devices now collect and report cell tower information (as of https://chromium-review.googlesource.com/#/c/412192/ ). This makes geolocation_handler aware of the changes, and provides a mechanism for consumers to query cell towers and wifi APNs separately. The geolocation handler now plucks the keys of interest out of the dictionary returned from shill's manager, rather than iterating and assuming each entry corresponds to a wifi access point. Use cell tower info in SimpleGeolocationProvider: it will now send cell tower information if available and the setting enabled. Cell towers and wifi APs are combined into a single management setting (as cell tower information should be less sensitive than wifi network information), but enables us to send them separately if so desired. BUG=b/30019266/ BUG=601174 TEST=Unittest verifying that requests are well-formed CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2624843003 Cr-Commit-Position: refs/heads/master@{#450183} Committed: https://chromium.googlesource.com/chromium/src/+/eaf50cbe0129bf60335274cdea2b... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/eaf50cbe0129bf60335274cdea2b... |
