New issue
Advanced search Search tips

Issue 748921 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 709301



Sign in to add a comment

Remove Geolocation AccessTokenStore

Project Member Reported by amoylan@chromium.org, Jul 26 2017

Issue description

It turns out that access tokens are not required (and are ignored) by the geolocate[1] Maps API called by NetworkLocationProvider. (Our support for access tokens was migrated from an earlier Gears geolocation API which is long since deprecated.)

Therefore we can remove the logic for persisting access tokens between calls to the network geolocation API, i.e. remove AccessTokenStore class.

Detailed plan to follow.

[1]: https://developers.google.com/maps/documentation/geolocation/intro
 
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 7 2017

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

commit 7697a35a5c86bf1a26d22c62d638ad7fa387eb2a
Author: Andrew Moylan <amoylan@chromium.org>
Date: Thu Sep 07 01:44:42 2017

Don't send or expect access tokens with network geolocation requests

This CL stops NetworkLocationRequest from sending or expecting to receive
access tokens from the Maps Geolocation API.

After this change, NetworkLocationProvider and NetworkLocationRequest are
unaware of access tokens.

This is part of the overall removal of access tokens, which are not part of the
Geolocation API any more. See design:
https://docs.google.com/document/d/1A-ZYMadJkwdQad_pC3MbpuCVNOOnRP_UZjGnXl-FOqE/edit?pli=1#

This CL is step 1 of the implementation plan outlined there.

TEST=Ran these:
device_unittests
browser_tests --gtest_filter="Geolocation*"
contentunit_tests --gtest_filter="Geolocation*"

Bug:  748921 
Change-Id: I009dd9ff0816a5932dbd19cdb2aabf1d13ed18f2
Reviewed-on: https://chromium-review.googlesource.com/633259
Commit-Queue: Andrew Moylan <amoylan@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500172}
[modify] https://crrev.com/7697a35a5c86bf1a26d22c62d638ad7fa387eb2a/device/geolocation/location_arbitrator.cc
[modify] https://crrev.com/7697a35a5c86bf1a26d22c62d638ad7fa387eb2a/device/geolocation/network_location_provider.cc
[modify] https://crrev.com/7697a35a5c86bf1a26d22c62d638ad7fa387eb2a/device/geolocation/network_location_provider.h
[modify] https://crrev.com/7697a35a5c86bf1a26d22c62d638ad7fa387eb2a/device/geolocation/network_location_provider_unittest.cc
[modify] https://crrev.com/7697a35a5c86bf1a26d22c62d638ad7fa387eb2a/device/geolocation/network_location_request.cc
[modify] https://crrev.com/7697a35a5c86bf1a26d22c62d638ad7fa387eb2a/device/geolocation/network_location_request.h

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 7 2017

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

commit cca5b6582227c7d9d643f87bb5b51533306a336d
Author: Andrew Moylan <amoylan@chromium.org>
Date: Thu Sep 07 03:22:59 2017

Explicitly disable network location for shell

The content shell's implementation of AccessTokenStore returns a dummy
(empty-string) endpoint URL for network geolocation. The effect of this is to
cause NetworkLocationProvider::StartProvider() to return (false) early and not
start fetching location updates. This also triggers a LOG(WARNING) of
!url.is_valid().

This CL instead explicitly disables network geolocation (via the existing
UseNetworkLocationProviders() in GeolocationDelegate) for the content shell.
The LOG(WARNING) is elevated to a DCHECK in NetworkLocationRequest.

This removes an edge case in the kinds of URLs passed to
NetworkLocationProvider and is a step towards making NetworkLocationProvider
always use the Google Maps Geolocation API endpoint URL (realistically the only
URL with which it is compatible). This is in turn part of the removal of
AccessTokenStore and is described in more detail in the design:
https://docs.google.com/document/d/1A-ZYMadJkwdQad_pC3MbpuCVNOOnRP_UZjGnXl-FOqE/edit?pli=1#

Bug:  748921 
Change-Id: Iaab7072c86fcd989155e5b9521ca4651ef59a3f7
Reviewed-on: https://chromium-review.googlesource.com/641175
Commit-Queue: Andrew Moylan <amoylan@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500205}
[modify] https://crrev.com/cca5b6582227c7d9d643f87bb5b51533306a336d/content/shell/browser/shell_access_token_store.cc
[modify] https://crrev.com/cca5b6582227c7d9d643f87bb5b51533306a336d/content/shell/browser/shell_browser_main_parts.cc
[modify] https://crrev.com/cca5b6582227c7d9d643f87bb5b51533306a336d/device/geolocation/network_location_provider.cc
[modify] https://crrev.com/cca5b6582227c7d9d643f87bb5b51533306a336d/device/geolocation/network_location_request.cc

Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 10 2017

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

commit ca1a7c380122e8dcfa270d1aaf71462a4372203a
Author: Andrew Moylan <amoylan@chromium.org>
Date: Sun Sep 10 23:27:28 2017

Expand comments in NetworkLocationProvider tests

Here I am just sprucing up the unittests for NetworkLocationProvider
with some comments that I would have found handy when studying these, as
part of updating them for some upcoming changes (separate future CL).

Also removed a relic unused argument of one helper function.

Bug:  748921 
Change-Id: Ifdb47837a6be66a1d1a728b88f219f4291ca7606
Reviewed-on: https://chromium-review.googlesource.com/656879
Commit-Queue: Andrew Moylan <amoylan@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500817}
[modify] https://crrev.com/ca1a7c380122e8dcfa270d1aaf71462a4372203a/device/geolocation/network_location_provider_unittest.cc

Cc: yyushkina@chromium.org
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 13 2017

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

commit ce4c522e456697bc8a31b4bdac30ed3bbc4be386
Author: Andrew Moylan <amoylan@chromium.org>
Date: Wed Sep 13 01:10:55 2017

Remove shell & webview AccessTokenStores

Content shell and Android webview don't make network geolocation
requests. Previously these embedders had dummy (no-op) implementations
of AccessTokenStore. This CL removes the dummy implementations and
instead uses the existing UseNetworkLocationProviders flag in
GeolocationDelegate as a gate.

This is part of the removal of all AccessTokenStores. Overall design:
https://docs.google.com/document/d/1A-ZYMadJkwdQad_pC3MbpuCVNOOnRP_UZjGnXl-FOqE

Details of reasoning why this is safe:

Content shell:
AccessTokenStore was being created but LoadAccessTokens was not being
called on it (after crrev.com/c/641175).

Android webview:
AccessTokenStore was being created and LoadAccessTokens was being called
on it, but to no consequence due to an "#if defined(OS_ANDROID)" block at
location_arbitrator.cc:174.

Bug:  748921 
Change-Id: I10ed3668ee015f855acf604e445643617bbe7713
Reviewed-on: https://chromium-review.googlesource.com/658010
Commit-Queue: Andrew Moylan <amoylan@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Selim Gurun <sgurun@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501494}
[modify] https://crrev.com/ce4c522e456697bc8a31b4bdac30ed3bbc4be386/android_webview/browser/aw_browser_main_parts.cc
[modify] https://crrev.com/ce4c522e456697bc8a31b4bdac30ed3bbc4be386/content/shell/BUILD.gn
[delete] https://crrev.com/fa312b778f668d1c9576da18969efcb3576d6f77/content/shell/browser/shell_access_token_store.cc
[delete] https://crrev.com/fa312b778f668d1c9576da18969efcb3576d6f77/content/shell/browser/shell_access_token_store.h
[modify] https://crrev.com/ce4c522e456697bc8a31b4bdac30ed3bbc4be386/content/shell/browser/shell_browser_main_parts.cc
[modify] https://crrev.com/ce4c522e456697bc8a31b4bdac30ed3bbc4be386/device/geolocation/geolocation_delegate.h
[modify] https://crrev.com/ce4c522e456697bc8a31b4bdac30ed3bbc4be386/device/geolocation/location_arbitrator.cc

Cc: language-bugs@chromium.org
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 22 2017

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

commit babe76124f3e49dfe5c895ded11060b38ec30c35
Author: Andrew Moylan <amoylan@chromium.org>
Date: Fri Sep 22 00:10:31 2017

Remove UseNetworkLocationProviders

This CL removes GeolocationDelegate::UseNetworkLocationProviders. Its
responsibility (of optionally disabling network geolocation) is
explicitly moved to GeolocationDelegate::CreateAccessTokenStore.

Today GeolocationDelegate has UseNetworkLocationProviders (default true)
and CreateAccessTokenStore (default nullptr), and these are effectively
&&'d together by LocationArbitrator to decide whether to try to use
network geolocation. Since AccessTokenStore has other uses,
UseNetworkLocationProviders is essentially redundant.

I surveyed embedders internal and external to Chromium repo and found no
cases of contradictory use of these two signals (i.e. no cases of
setting UseNetworkLocationProviders to false but still overriding
CreateAccessTokenStore to return a non-nullptr).

I also spruced up the LocationArbitrator unit tests with some comments
and made them a bit more "injecty" to simplify understanding them.

Bug:  748921 
Change-Id: I528f6f0a81263df868795be81201141c260aa836
Reviewed-on: https://chromium-review.googlesource.com/670964
Reviewed-by: Selim Gurun <sgurun@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Andrew Moylan <amoylan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503619}
[modify] https://crrev.com/babe76124f3e49dfe5c895ded11060b38ec30c35/android_webview/browser/aw_browser_main_parts.cc
[modify] https://crrev.com/babe76124f3e49dfe5c895ded11060b38ec30c35/content/shell/browser/shell_browser_main_parts.cc
[modify] https://crrev.com/babe76124f3e49dfe5c895ded11060b38ec30c35/device/geolocation/geolocation_delegate.cc
[modify] https://crrev.com/babe76124f3e49dfe5c895ded11060b38ec30c35/device/geolocation/geolocation_delegate.h
[modify] https://crrev.com/babe76124f3e49dfe5c895ded11060b38ec30c35/device/geolocation/location_arbitrator.cc
[modify] https://crrev.com/babe76124f3e49dfe5c895ded11060b38ec30c35/device/geolocation/location_arbitrator_unittest.cc

Components: Blink>Geolocation
Components: -Blink>Location
Project Member

Comment 12 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

Cc: amoylan@chromium.org
 Issue 725048  has been merged into this issue.
Blocking: 709301
Project Member

Comment 15 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 16 by bugdroid1@chromium.org, Oct 17 2017

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

commit 83b35b2be33e82a7bd7163b5d7628eb45141c379
Author: Andrew Moylan <amoylan@chromium.org>
Date: Tue Oct 17 22:03:29 2017

Return void from LocationProvider::StartProvider

This CL changes the return type of LocationProvider::StartProvider (and
related functions) from bool to void.

Currently, no-one inspects the value returned from this function, except
to pass it through to other places that don't inspect it.

Most impls always return true. The situation where an impl doesn't
always return true (LocationArbitrator) are responded-to by other means.
Specifically, LocationArbitrator provides an Error geoposition in the
case that it couldn't create any location providers.

I also opportunistically removed the unused MockLocationProvider in this
CL.

Bug:  748921 
Change-Id: Iaa7903b3eecaacc0076b3c1f1cce3be86187285d
Reviewed-on: https://chromium-review.googlesource.com/722139
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Andrew Moylan <amoylan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509561}
[modify] https://crrev.com/83b35b2be33e82a7bd7163b5d7628eb45141c379/device/geolocation/BUILD.gn
[modify] https://crrev.com/83b35b2be33e82a7bd7163b5d7628eb45141c379/device/geolocation/android/java/src/org/chromium/device/geolocation/LocationProviderAdapter.java
[modify] https://crrev.com/83b35b2be33e82a7bd7163b5d7628eb45141c379/device/geolocation/fake_location_provider.cc
[modify] https://crrev.com/83b35b2be33e82a7bd7163b5d7628eb45141c379/device/geolocation/fake_location_provider.h
[modify] https://crrev.com/83b35b2be33e82a7bd7163b5d7628eb45141c379/device/geolocation/location_api_adapter_android.cc
[modify] https://crrev.com/83b35b2be33e82a7bd7163b5d7628eb45141c379/device/geolocation/location_api_adapter_android.h
[modify] https://crrev.com/83b35b2be33e82a7bd7163b5d7628eb45141c379/device/geolocation/location_arbitrator.cc
[modify] https://crrev.com/83b35b2be33e82a7bd7163b5d7628eb45141c379/device/geolocation/location_arbitrator.h
[modify] https://crrev.com/83b35b2be33e82a7bd7163b5d7628eb45141c379/device/geolocation/location_provider.h
[modify] https://crrev.com/83b35b2be33e82a7bd7163b5d7628eb45141c379/device/geolocation/location_provider_android.cc
[modify] https://crrev.com/83b35b2be33e82a7bd7163b5d7628eb45141c379/device/geolocation/location_provider_android.h
[delete] https://crrev.com/eda21d3879cf6451ab838530a2ca25167fa3b7c5/device/geolocation/mock_location_provider.cc
[delete] https://crrev.com/eda21d3879cf6451ab838530a2ca25167fa3b7c5/device/geolocation/mock_location_provider.h
[modify] https://crrev.com/83b35b2be33e82a7bd7163b5d7628eb45141c379/device/geolocation/network_location_provider.cc
[modify] https://crrev.com/83b35b2be33e82a7bd7163b5d7628eb45141c379/device/geolocation/network_location_provider.h
[modify] https://crrev.com/83b35b2be33e82a7bd7163b5d7628eb45141c379/device/geolocation/network_location_provider_unittest.cc

Project Member

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

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

commit b5081e3d2cc6d46805a6ce6362eae1754d5fa1db
Author: Andrew Moylan <amoylan@chromium.org>
Date: Wed Oct 18 00:11:28 2017

Skip network geolocation if request context = null

This CL changes the way embedders enable/disable network geolocation:

Before: Disabled if GeolocationDelegate::CreateAccessTokenStore()
        returns null (the default).
Now:    Disabled if
        ContentBrowserClient::GetGeolocationRequestContext
	produces a nullptr URLRequestContextGetter (also the default).

This change doesn't alter which embedders enable/disable network
geolocation, because the only two embedders that currently return a
non-null result from CreateAccessTokenStore() are also the only two
embedders that produce a non-null result from
GetGeolocationRequestContextGetter: Chrome and Chromecast.

Embedders external to the Chromium repo will need to implement
GetGeolocationRequestContext:
https://groups.google.com/a/chromium.org/forum/#!topic/embedder-dev/0ln9lxrsBw8

With this change, LocationArbitrator no longer knows about
AccessTokenStore at all, and AccessTokenStore becomes fully unused. It
can be removed in a subsequent CL.

This is a step in the removal of AccessTokenStore as detailed here:
https://docs.google.com/document/d/1A-ZYMadJkwdQad_pC3MbpuCVNOOnRP_UZjGnXl-FOqE

Bug:  748921 
Change-Id: Id413a0cdb207fcec8fd25f381068c6c13ad4068c
Reviewed-on: https://chromium-review.googlesource.com/676826
Commit-Queue: Andrew Moylan <amoylan@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509616}
[modify] https://crrev.com/b5081e3d2cc6d46805a6ce6362eae1754d5fa1db/device/geolocation/location_arbitrator.cc
[modify] https://crrev.com/b5081e3d2cc6d46805a6ce6362eae1754d5fa1db/device/geolocation/location_arbitrator.h
[modify] https://crrev.com/b5081e3d2cc6d46805a6ce6362eae1754d5fa1db/device/geolocation/location_arbitrator_unittest.cc

Project Member

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

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

commit 2dbc85c3b174787fecc1aaa96d0eda2ee759ced2
Author: Andrew Moylan <amoylan@chromium.org>
Date: Fri Oct 20 06:08:33 2017

Remove AccessTokenStore

This CL removes AccessTokenStore and derived classes. After previous
CLs as part of  crbug.com/748921 , this class is now unused.

This CL also removes the registration of the access_token_store pref
field which won't be used any more.

Embedders which were providing non-trivial overrides of AccessTokenStore
should read:
https://groups.google.com/a/chromium.org/forum/#!topic/embedder-dev/0ln9lxrsBw8

This is the last step in the removal of AccessTokenStore as detailed in
the design:
https://docs.google.com/document/d/1A-ZYMadJkwdQad_pC3MbpuCVNOOnRP_UZjGnXl-FOqE

Bug:  748921 
Change-Id: Iad74af6fca5e261970ada5a5ad9e58e98fe83293
Reviewed-on: https://chromium-review.googlesource.com/708494
Reviewed-by: Stephen Lanham <slan@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Andrew Moylan <amoylan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510354}
[modify] https://crrev.com/2dbc85c3b174787fecc1aaa96d0eda2ee759ced2/android_webview/browser/aw_browser_main_parts.cc
[modify] https://crrev.com/2dbc85c3b174787fecc1aaa96d0eda2ee759ced2/android_webview/browser/aw_content_browser_client.cc
[modify] https://crrev.com/2dbc85c3b174787fecc1aaa96d0eda2ee759ced2/chrome/browser/BUILD.gn
[modify] https://crrev.com/2dbc85c3b174787fecc1aaa96d0eda2ee759ced2/chrome/browser/chrome_browser_main.cc
[delete] https://crrev.com/f743d39ebb54688d338611d43225b70cd3076350/chrome/browser/geolocation/access_token_store_browsertest.cc
[delete] https://crrev.com/f743d39ebb54688d338611d43225b70cd3076350/chrome/browser/geolocation/chrome_access_token_store.cc
[delete] https://crrev.com/f743d39ebb54688d338611d43225b70cd3076350/chrome/browser/geolocation/chrome_access_token_store.h
[delete] https://crrev.com/f743d39ebb54688d338611d43225b70cd3076350/chrome/browser/geolocation/geolocation_prefs.cc
[delete] https://crrev.com/f743d39ebb54688d338611d43225b70cd3076350/chrome/browser/geolocation/geolocation_prefs.h
[modify] https://crrev.com/2dbc85c3b174787fecc1aaa96d0eda2ee759ced2/chrome/browser/prefs/browser_prefs.cc
[modify] https://crrev.com/2dbc85c3b174787fecc1aaa96d0eda2ee759ced2/chrome/test/BUILD.gn
[modify] https://crrev.com/2dbc85c3b174787fecc1aaa96d0eda2ee759ced2/chromecast/browser/BUILD.gn
[modify] https://crrev.com/2dbc85c3b174787fecc1aaa96d0eda2ee759ced2/chromecast/browser/cast_browser_main_parts.cc
[delete] https://crrev.com/f743d39ebb54688d338611d43225b70cd3076350/chromecast/browser/geolocation/cast_access_token_store.cc
[delete] https://crrev.com/f743d39ebb54688d338611d43225b70cd3076350/chromecast/browser/geolocation/cast_access_token_store.h
[modify] https://crrev.com/2dbc85c3b174787fecc1aaa96d0eda2ee759ced2/content/shell/browser/shell_browser_main_parts.cc
[modify] https://crrev.com/2dbc85c3b174787fecc1aaa96d0eda2ee759ced2/device/geolocation/BUILD.gn
[delete] https://crrev.com/f743d39ebb54688d338611d43225b70cd3076350/device/geolocation/access_token_store.h
[delete] https://crrev.com/f743d39ebb54688d338611d43225b70cd3076350/device/geolocation/fake_access_token_store.cc
[delete] https://crrev.com/f743d39ebb54688d338611d43225b70cd3076350/device/geolocation/fake_access_token_store.h
[modify] https://crrev.com/2dbc85c3b174787fecc1aaa96d0eda2ee759ced2/device/geolocation/geolocation_delegate.cc
[modify] https://crrev.com/2dbc85c3b174787fecc1aaa96d0eda2ee759ced2/device/geolocation/geolocation_delegate.h
[modify] https://crrev.com/2dbc85c3b174787fecc1aaa96d0eda2ee759ced2/device/geolocation/geolocation_provider_impl_unittest.cc

\o/
Status: Fixed (was: Started)

Sign in to add a comment