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

Issue 788298 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Jun 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 709301



Sign in to add a comment

Change testing clients of Geolocation from using the Gelocation::OverrideLocationForTesting()

Project Member Reported by ke...@intel.com, Nov 24 2017

Issue description

Some clients of Geolocation directly call the 
Gelocation::OverrideLocationForTesting() in their browser-test or Layout-test.

We should make them avoid calling OverrideLocationForTesting() since the Geolocation will be moved to //service/device.

 

Comment 1 by ke...@intel.com, Nov 24 2017

Blocking: 709301

Comment 2 by ke...@intel.com, Dec 14 2017

The test case of GeolocationBrowserTestWithOutOverrider::UrlWithApiKey tries to connect the real geolocation implementation instead of the FakeGeolocation.
So we leave a TODO that to remove the class GeolocationBrowserTestWithOutOverrider and rewrite the test case of UrlWithApiKey as a services_unittest. Also remove the
ui_test_utils::OverrideGeolocation() then.

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 15 2017

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

commit 94a5b9187d708be29c6c59767f4d3640ba3ea7a2
Author: Ke He <ke.he@intel.com>
Date: Fri Dec 15 03:38:55 2017

Remove OverrideLocationForTesting() from test clients

Tests shouldn't call the OverrideLocationForTesting() since the Geolocation
core will be moved to //service/device.

In this CL a common ScopedGeolocationOverrider implementation is moved to
//device/geolocation/public/cpp, so it can be reused by multiple clients.

The ui_test_utils::OverrideGeolocation() is not removed in this CL because
it is still needed by UrlWithApiKey in GeolocationBrowserTest. We'll rewrite
it as a Service-Unittest when moving geolocation-core into //service/device
folder, and remove the ui_test_utils::OverrideGeolocation() then.

BUG= 788298 

Change-Id: I43c029bb5e82058673e738138cdadf139a076ff2
Reviewed-on: https://chromium-review.googlesource.com/778488
Commit-Queue: Ke He <ke.he@intel.com>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524312}
[modify] https://crrev.com/94a5b9187d708be29c6c59767f4d3640ba3ea7a2/chrome/browser/BUILD.gn
[modify] https://crrev.com/94a5b9187d708be29c6c59767f4d3640ba3ea7a2/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/94a5b9187d708be29c6c59767f4d3640ba3ea7a2/chrome/browser/extensions/extension_geolocation_apitest.cc
[modify] https://crrev.com/94a5b9187d708be29c6c59767f4d3640ba3ea7a2/chrome/browser/geolocation/geolocation_browsertest.cc
[modify] https://crrev.com/94a5b9187d708be29c6c59767f4d3640ba3ea7a2/components/BUILD.gn
[modify] https://crrev.com/94a5b9187d708be29c6c59767f4d3640ba3ea7a2/components/autofill/content/browser/BUILD.gn
[modify] https://crrev.com/94a5b9187d708be29c6c59767f4d3640ba3ea7a2/components/autofill/content/browser/risk/fingerprint_browsertest.cc
[modify] https://crrev.com/94a5b9187d708be29c6c59767f4d3640ba3ea7a2/content/shell/BUILD.gn
[modify] https://crrev.com/94a5b9187d708be29c6c59767f4d3640ba3ea7a2/content/shell/browser/DEPS
[modify] https://crrev.com/94a5b9187d708be29c6c59767f4d3640ba3ea7a2/content/shell/browser/layout_test/layout_test_browser_context.cc
[modify] https://crrev.com/94a5b9187d708be29c6c59767f4d3640ba3ea7a2/content/shell/browser/layout_test/layout_test_browser_context.h
[modify] https://crrev.com/94a5b9187d708be29c6c59767f4d3640ba3ea7a2/content/test/BUILD.gn
[modify] https://crrev.com/94a5b9187d708be29c6c59767f4d3640ba3ea7a2/device/geolocation/public/cpp/BUILD.gn
[add] https://crrev.com/94a5b9187d708be29c6c59767f4d3640ba3ea7a2/device/geolocation/public/cpp/scoped_geolocation_overrider.cc
[add] https://crrev.com/94a5b9187d708be29c6c59767f4d3640ba3ea7a2/device/geolocation/public/cpp/scoped_geolocation_overrider.h

Project Member

Comment 4 by bugdroid1@chromium.org, Feb 6 2018

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

commit d1283d42e78475619eefd8e507bc058198c162f8
Author: Ke He <ke.he@intel.com>
Date: Tue Feb 06 05:17:49 2018

Change the Geolocation browsertest UrlWithApiKey to a service-unittest.

The Geolocation browsertest UrlWithApiKey is the only one which uses the
ui_test_utils::OverrideGeolocation().

After changing it to service-unittest and move it into //services/, we can
remove both the ui_test_util::OverrideGeolocation() and geolocation_provider.h
in next step.

BUG= 788298 

Change-Id: I95b579211107acd64c594a5011641a61c2c1b2de
Reviewed-on: https://chromium-review.googlesource.com/880141
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Commit-Queue: Ke He <ke.he@intel.com>
Cr-Commit-Position: refs/heads/master@{#534635}
[modify] https://crrev.com/d1283d42e78475619eefd8e507bc058198c162f8/chrome/browser/geolocation/geolocation_browsertest.cc
[modify] https://crrev.com/d1283d42e78475619eefd8e507bc058198c162f8/services/device/BUILD.gn
[modify] https://crrev.com/d1283d42e78475619eefd8e507bc058198c162f8/services/device/device_service_test_base.cc
[modify] https://crrev.com/d1283d42e78475619eefd8e507bc058198c162f8/services/device/device_service_test_base.h
[add] https://crrev.com/d1283d42e78475619eefd8e507bc058198c162f8/services/device/geolocation/geolocation_service_unittest.cc
[modify] https://crrev.com/d1283d42e78475619eefd8e507bc058198c162f8/services/device/manifest.json
[modify] https://crrev.com/d1283d42e78475619eefd8e507bc058198c162f8/services/device/unittest_manifest.json

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 7 2018

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

commit e283c62ead24ab19f80254a8e18bdb60491d175f
Author: Ke He <ke.he@intel.com>
Date: Wed Mar 07 03:09:28 2018

Remove OverrideGeolocation() from ui_tests_utils

After we eliminated all the usage of OverrideLocationForTesting() outside
of Device Service, the OverrideGeolocation() should be removed from
ui_tests_utils.

BUG= 788298 

Change-Id: I1d3138bafa7bd638e3c4dc381581239594cf0427
Reviewed-on: https://chromium-review.googlesource.com/903274
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Ke He <ke.he@intel.com>
Cr-Commit-Position: refs/heads/master@{#541306}
[modify] https://crrev.com/e283c62ead24ab19f80254a8e18bdb60491d175f/chrome/test/DEPS
[modify] https://crrev.com/e283c62ead24ab19f80254a8e18bdb60491d175f/chrome/test/base/ui_test_utils.cc
[modify] https://crrev.com/e283c62ead24ab19f80254a8e18bdb60491d175f/chrome/test/base/ui_test_utils.h

Comment 6 by ke...@intel.com, Jun 29 2018

Status: Fixed (was: Started)

Sign in to add a comment