New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 718694 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature

Blocking:
issue 769158



Sign in to add a comment

Coarse IP-based location inference

Project Member Reported by amoylan@chromium.org, May 5 2017

Issue description

We need a library for coarse (i.e. country/regional) location guessing based on IP address. This will help with e.g. future efforts to better guess which languages a user may care about for translation.

IP address is already a factor in the overall geolocation library. We could refactor that slightly to make it available separately, but the final approach is TBD.
 
Cc: yyushkina@chromium.org
Issue 726949 has been merged into this issue.
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/17329fccf83134a098084e70a1213564d71cfa6f

commit 17329fccf83134a098084e70a1213564d71cfa6f
Author: Andrew Moylan <amoylan@chromium.org>
Date: Mon Sep 25 07:43:30 2017

Use fixed endpoint URL for network geolocation

This change makes NetworkLocationRequest no longer accept a
user-provided endpoint URL for geolocation. Instead, it always uses the
Google Maps API geolocate endpoint URL (which is the existing default).
In practice, this is the only URL which can be used anyway, as
NetworkLocationRequest's implementation is tied closely to the details
of that API.

With this change, LocationArbitrator always creates exactly one
NetworkLocationProvider, instead of creating one NetworkLocationProvider
per entry in the access token store. The theoretical possibility of
using multiple NetworkLocationProviders with different URLs was not
useful in practice because NetworkLocationProvider/NetworkLocationRequest
are closely tied specifically to the Google Maps API interface.

For more details / motivation / results of looking at internal/external
embedders, please see the following design document:
https://docs.google.com/document/d/1Bkd06tmAFb9OY0qkq_IeWc4kr3ZDxzhrYkg3mGUMGRE

The only cases where a non-default URL appear to be in use (internal or
external to Chromium repo) are for embedders manually appending their
Googgle API key to the URL. Therefore, this change adds support for
setting the geolocation API key if desired. Embedders do this by
overriding ContentBrowserClient::GetGeolocationApiKey().

Embedders who have been using the default endpoint URL were
automatically using the key taken from google_apis::GetAPIKey(). These
embedders will now need to override GetGeolocationApiKey() to return
google_apis::GetAPIKey(). This CL does this for Chrome & Chromecast,
which are the only two embedders in Chromium that are using
NetworkLocationProviders.

[Another option would be to make google_apis::GetAPIKey() the default
implementation (Chrome and Chromecast then wouldn't need to provide an
override). I didn't go for this as content/ has a -"google_apis" DEPS
entry and expresses a preference for embedders using google_apis
directly.]

This change is part of the overall removal of AccessTokenStore, as
detailed in the above doc.

Misc:
- Added an opportunistic integration test checking that, for Chrome, the
  expected URL and API key does in fact get used.
- Removed an unused layer of abstraction in the form of the
  |NewNetworkLocationProvider| factory function.

Bug:  748921 , 718694 
Change-Id: I12bae25908c48cfbe7fb93cc2d25f1a91ce03bd8
Reviewed-on: https://chromium-review.googlesource.com/675023
Commit-Queue: Andrew Moylan <amoylan@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504002}
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/chrome/browser/geolocation/geolocation_browsertest.cc
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/content/browser/service_manager/service_manager_context.cc
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/content/public/browser/content_browser_client.h
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/device/geolocation/BUILD.gn
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/device/geolocation/DEPS
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/device/geolocation/geolocation_provider.h
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/device/geolocation/geolocation_provider_impl.cc
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/device/geolocation/location_arbitrator.cc
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/device/geolocation/location_arbitrator.h
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/device/geolocation/location_arbitrator_unittest.cc
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/device/geolocation/network_location_provider.cc
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/device/geolocation/network_location_provider.h
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/device/geolocation/network_location_provider_unittest.cc
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/device/geolocation/network_location_request.cc
[modify] https://crrev.com/17329fccf83134a098084e70a1213564d71cfa6f/device/geolocation/network_location_request.h

Status: Started (was: Assigned)
Blocking: 769158
Cc: renjieliu@chromium.org napper@chromium.org
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7

commit 8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7
Author: Andrew Moylan <amoylan@chromium.org>
Date: Tue Oct 10 04:29:04 2017

Get Geolocation URL context via ContentBrowserClient

This CL changes the Geolocation system's LocationArbitrator to obtain a
URLRequestContextGetter (used for network geolocation requests) from an
embedder-provided method in ContentBrowserClient. Previously, the
URLRequestContextGetter was provided as a side effect of
the embedder-provided AccessTokenStore::LoadAccessTokens.

The motivations for this change are the following:
1. AccessTokenStore is going away as access tokens are no longer needed:
   https://docs.google.com/document/d/1Bkd06tmAFb9OY0qkq_IeWc4kr3ZDxzhrYkg3mGUMGRE
2. Geolocation is moving to Device service and ContentBrowserClient is
   an appropriate home for embedder supplied parameters in that world.

With this change, AccessTokenStore::LoadAccessTokens is no longer called
by LocationArbitrator (or by any non-test code).

The only remaining use for AccessTokenStore is its role in determining
whether network geolocation should be performed. (Namely: If
CreateAccessTokenStore() returns non-nullptr, then do perform network
geolocation.) This final use of AccessTokenStore will be removed in a
followup CL (a nullptr URLRequestContextGetter will become the flag
instead). Following that, AccessTokenStore will be removed wholly.

The embedders which currently use network geolocation are Chrome and
Chromecast (reasoning: these are the only implementers of
CreateAccessTokenStore()). Therefore these two embedders have been
provided with implementations of
ContentBrowserClient::GetGeolocationRequestContextGetter. The
implementations are identical to the existing implementations in
ChromeAccessTokenStore and CastAccessTokenStore respectively.

I explored the possibility of removing the existing (now unused)
URLRequestContextGetter functionality from
(Chrome|Cast)AccessTokenStore. I opted not to do so because the
associated modifications of existing access token store unit tests end
up requiring either (a) quite a bit of refactoring to make them
rational, or (b) minimal surgery leaving them in a weird state. Neither
option seems as comprehensible as leaving the functionality intact but
unused.

For reference, the existing Chrome request-context-fetching code can be
found at: chrome/browser/geolocation/chrome_access_token_store.cc:86
And the existing cast code can be found at:
chromecast/browser/geolocation/cast_access_token_store.cc:25

Bug:  748921 , 718694 
Change-Id: I84301bbb3152639ac329ca36ed5284a28aed4747
Reviewed-on: https://chromium-review.googlesource.com/674425
Commit-Queue: Andrew Moylan <amoylan@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Stephen Lanham <slan@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Colin Blundell (at a convergence, slow until Oct 16) <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507585}
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/chromecast/browser/cast_content_browser_client.cc
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/chromecast/browser/cast_content_browser_client.h
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/content/browser/service_manager/service_manager_context.cc
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/content/public/browser/content_browser_client.h
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/device/geolocation/geolocation_provider.h
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/device/geolocation/geolocation_provider_impl.cc
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/device/geolocation/location_arbitrator.cc
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/device/geolocation/location_arbitrator.h
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/device/geolocation/location_arbitrator_unittest.cc
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/device/geolocation/network_location_provider.cc
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/device/geolocation/network_location_provider.h
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/device/geolocation/network_location_request.cc
[modify] https://crrev.com/8673dbae36f4ba1a4d7fc8319af8c7d2882fddd7/device/geolocation/network_location_request.h

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b0663bc78138a9525211a0d0d2bc509502a2607f

commit b0663bc78138a9525211a0d0d2bc509502a2607f
Author: Renjie Liu <renjieliu@chromium.org>
Date: Wed Oct 18 00:51:34 2017

Add partial traffic annotation to network_location_request

This is for
https://docs.google.com/document/d/1oSQM3GEMTL1raWLYv8S6_Yyt93FF2zQbANI4bKJ6LDs/edit?pli=1#heading=h.2st2zzoz32x8
and that we plan to reuse NetworkLocationRequest from there,
and a different "setting" and/or "chrome_policy" field will be
appropriate depending on the caller.

Bug:  718694 , 769158 
Change-Id: Ie966a1ec6139f1d613ba2e0c5f83d02b9dc42ace
Reviewed-on: https://chromium-review.googlesource.com/717596
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Renjie Liu <renjieliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509632}
[modify] https://crrev.com/b0663bc78138a9525211a0d0d2bc509502a2607f/device/geolocation/network_location_provider.cc
[modify] https://crrev.com/b0663bc78138a9525211a0d0d2bc509502a2607f/device/geolocation/network_location_request.cc
[modify] https://crrev.com/b0663bc78138a9525211a0d0d2bc509502a2607f/device/geolocation/network_location_request.h

Project Member

Comment 10 by bugdroid1@chromium.org, Nov 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d37d5173129c7d561cc2cb3a54bdddc63a6d50f4

commit d37d5173129c7d561cc2cb3a54bdddc63a6d50f4
Author: Andrew Moylan <amoylan@chromium.org>
Date: Mon Nov 06 00:20:29 2017

Expand NetworkLocationRequest::MakeRequest comment

May as well document this behaviour of MakeRequest. It's used by the
caching mechanism of existing NetworkLocationProvider, and will be
useful to reuse this class for IP geo.

Bug:  718694 
Change-Id: I64cde3dfd5afd02d6cb53a1cfeaa12cc0a3d66c3
Reviewed-on: https://chromium-review.googlesource.com/729603
Commit-Queue: Andrew Moylan <amoylan@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514074}
[modify] https://crrev.com/d37d5173129c7d561cc2cb3a54bdddc63a6d50f4/device/geolocation/network_location_request.h

Project Member

Comment 11 by bugdroid1@chromium.org, Nov 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b81abebc56a42328483292d9b8342253f244f2ee

commit b81abebc56a42328483292d9b8342253f244f2ee
Author: Renjie Liu <renjieliu@chromium.org>
Date: Sat Nov 18 01:19:28 2017

PublicIpAddressLocationNotifier implementation

Original owner: amoylan@chromium.org
Original cl: https://chromium-review.googlesource.com/c/chromium/src/+/754254

This CL is part of the implementation of public-IP-only geolocation
as described in the design doc:
https://docs.google.com/document/d/1oSQM3GEMTL1raWLYv8S6_Yyt93FF2zQbANI4bKJ6LDs

This part is the implementation of the centralized
PublicIpAddressLocationNotifier that issues IP-geolocation requests
to handle any/all clients.

This reuses the NetworkLocationRequest class from the existing
Geolocation service (which provides web-platform geolocation)
in //device/geolocation.

This CL creates //services/device/geolocation/ and includes a "+net"
DEPS entry. (This "+net" dependency, among others, will eventually be
needed when all of //device/geolocation eventually moves into
//services/device/geolocation as part of Geolocation Servicification.)

Tests: Unit tests for PublicIpAddressGeolocationNotifier covering the
logic to do with responding to different sequences of "network change
detected" and "client wants a new/next position".

Add PartialNetworkTrafficAnnotation as well.

Bug:  718694 ,  769158 

Bug: 
Change-Id: I792a1793a54b283949b97f411b70a71ea27ec6c8
Reviewed-on: https://chromium-review.googlesource.com/757821
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
Reviewed-by: Renjie Liu <renjieliu@chromium.org>
Commit-Queue: Renjie Liu <renjieliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517661}
[modify] https://crrev.com/b81abebc56a42328483292d9b8342253f244f2ee/device/geolocation/BUILD.gn
[modify] https://crrev.com/b81abebc56a42328483292d9b8342253f244f2ee/device/geolocation/network_location_request.h
[modify] https://crrev.com/b81abebc56a42328483292d9b8342253f244f2ee/services/device/BUILD.gn
[add] https://crrev.com/b81abebc56a42328483292d9b8342253f244f2ee/services/device/geolocation/BUILD.gn
[add] https://crrev.com/b81abebc56a42328483292d9b8342253f244f2ee/services/device/geolocation/DEPS
[add] https://crrev.com/b81abebc56a42328483292d9b8342253f244f2ee/services/device/geolocation/public_ip_address_location_notifier.cc
[add] https://crrev.com/b81abebc56a42328483292d9b8342253f244f2ee/services/device/geolocation/public_ip_address_location_notifier.h
[add] https://crrev.com/b81abebc56a42328483292d9b8342253f244f2ee/services/device/geolocation/public_ip_address_location_notifier_unittest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Nov 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b08f01ba16baffa0783efd5696a2c652facde0b9

commit b08f01ba16baffa0783efd5696a2c652facde0b9
Author: Renjie Liu <renjieliu@chromium.org>
Date: Wed Nov 22 22:57:02 2017

PublicIpAddressGeolocator implementation

original owner: amoylan@chromium.org
original cl: https://chromium-review.googlesource.com/c/chromium/src/+/754153

This CL is part of the implementation of public-IP-only geolocation
as described in the design doc:
https://docs.google.com/document/d/1oSQM3GEMTL1raWLYv8S6_Yyt93FF2zQbANI4bKJ6LDs

This part is an implementation of the mojom::Geolocation interface
that is separate from the existing web-platform implementation of
that interface (which lives in //device/geolocation currently).

This implementation uses the PublicIpAddressLocationNotifier
(upstream dependent CL) to provide IP-only geolocation.

Tested: Basic unit tests added.

Bug:  718694 ,  769158 
Change-Id: I716b52d8ea21cb8ad4f94b128fddc0c0cef967a3
Reviewed-on: https://chromium-review.googlesource.com/770306
Commit-Queue: Renjie Liu <renjieliu@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518784}
[modify] https://crrev.com/b08f01ba16baffa0783efd5696a2c652facde0b9/services/device/BUILD.gn
[modify] https://crrev.com/b08f01ba16baffa0783efd5696a2c652facde0b9/services/device/geolocation/BUILD.gn
[add] https://crrev.com/b08f01ba16baffa0783efd5696a2c652facde0b9/services/device/geolocation/public_ip_address_geolocator.cc
[add] https://crrev.com/b08f01ba16baffa0783efd5696a2c652facde0b9/services/device/geolocation/public_ip_address_geolocator.h
[add] https://crrev.com/b08f01ba16baffa0783efd5696a2c652facde0b9/services/device/geolocation/public_ip_address_geolocator_unittest.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Dec 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9b34d5365079be9b65905b0ba9204305e527d776

commit 9b34d5365079be9b65905b0ba9204305e527d776
Author: Renjie Liu <renjieliu@chromium.org>
Date: Mon Dec 18 23:48:38 2017

Provide PublicIpAddressGeolocationProvider in DeviceService

original owner: amoylan@chromium.org
orginal cl: https://chromium-review.googlesource.com/c/chromium/src/+/754256

This CL is part of the implementation of public-IP-only geolocation
as described in the design doc:
https://docs.google.com/document/d/1oSQM3GEMTL1raWLYv8S6_Yyt93FF2zQbANI4bKJ6LDs

This final part changes DeviceService to serve requests for
PublicIpAddressGeolocationProvider. The necessary dependencies (context
for URL requests, and Google API Key for geolocation queries) are injected
during the construction of DeviceService (currently hosted by browser
process).

In a subsequent CL, this new capability will be used to implement
GeoLanguageProvider, as described in this design doc:
https://docs.google.com/document/d/18WqVHz5F9vaUiE32E8Ge6QHmku2QSJKvlqB9JjnIM-g

Bug:  718694 ,  769158 
Change-Id: I79e96d482955260184bf5b4653b79d02b1ee75c7
Reviewed-on: https://chromium-review.googlesource.com/798993
Commit-Queue: Renjie Liu <renjieliu@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Reviewed-by: Ke He <ke.he@intel.com>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524863}
[modify] https://crrev.com/9b34d5365079be9b65905b0ba9204305e527d776/content/browser/service_manager/service_manager_context.cc
[modify] https://crrev.com/9b34d5365079be9b65905b0ba9204305e527d776/services/device/DEPS
[modify] https://crrev.com/9b34d5365079be9b65905b0ba9204305e527d776/services/device/device_service.cc
[modify] https://crrev.com/9b34d5365079be9b65905b0ba9204305e527d776/services/device/device_service.h
[modify] https://crrev.com/9b34d5365079be9b65905b0ba9204305e527d776/services/device/device_service_test_base.cc
[modify] https://crrev.com/9b34d5365079be9b65905b0ba9204305e527d776/services/device/geolocation/public_ip_address_geolocation_provider.cc
[modify] https://crrev.com/9b34d5365079be9b65905b0ba9204305e527d776/services/device/geolocation/public_ip_address_geolocation_provider.h
[modify] https://crrev.com/9b34d5365079be9b65905b0ba9204305e527d776/services/device/manifest.json

Status: Fixed (was: Started)
Cc: ma...@chromium.org

Sign in to add a comment