Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2127973002:
Geolocation: change GeolocationDelegate to injected by content/ (Closed)
|
Created:
4 years, 5 months ago by mcasas Modified:
4 years, 4 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, mlamouri+watch-geolocation_chromium.org, Michael van Ouwerkerk, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGeolocation: change GeolocationDelegate to injected by content/
GeolocationProviderImpl is a singleton that directly references
GetContentClient()::browser() on ctor, to retrieve an embedder-
provider GeolocationDelegate. This blocks moving Geolocation
out of content/
This CL changes that mechanism: the GeolocationDelegate
is injected in BrowserMainLoop::PostMainMessageLoopStart()
(so it won't affect startup time IIUC), and kept in a variable
until the singleton construction.
This CL is the last blocking factor before taking Geolocation
out of content/browser and into device.
BUG=612334
TEST=./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
PS:
A note about shell_content_browser_client.cc: Changing
GeolocationDelegate to injected, causes component's
AutofillRiskFingerprintTest to start failing because
ShellGeolocationDelegate was initialized too early
and could not get a correct ShellBrowserContext*.
This CL also changes the ShellGeolocationDelegate to get
the browser_context() when is needed.
Committed: https://crrev.com/19677cb9f1901f0eb7881380ac85a0255195e06d
Cr-Commit-Position: refs/heads/master@{#407015}
Patch Set 1 : #
Total comments: 7
Patch Set 2 : rockot@ comments and sorting out ShellGeolocationDelegate. Rebased. #
Total comments: 2
Patch Set 3 : simplified shell_content_browser_client.cc #
Messages
Total messages: 31 (18 generated)
Description was changed from ========== [WIP]: Inject GeolocationDelegate to GeolocationProviderImpl BUG=612334 ========== to ========== [WIP]: Inject GeolocationDelegate to GeolocationProviderImpl BUG=612334 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== [WIP]: Inject GeolocationDelegate to GeolocationProviderImpl BUG=612334 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Geolocation: change GeolocationDelegate to injected by content/ GeolocationProviderImpl is a singleton that directly references GetContentClient()::browser() on ctor, to retrieve an embedder- provider GeolocationDelegate. This blocks moving Geolocation out of content/ This CL changes that mechanism: the GeolocationDelegate is injected in BrowserMainLoop::PostMainMessageLoopStart() and kept in a variable until the singleton construction. This CL is the last blocking factor before taking Geolocation out of content/browser and into device. BUG=612334 ==========
Description was changed from ========== Geolocation: change GeolocationDelegate to injected by content/ GeolocationProviderImpl is a singleton that directly references GetContentClient()::browser() on ctor, to retrieve an embedder- provider GeolocationDelegate. This blocks moving Geolocation out of content/ This CL changes that mechanism: the GeolocationDelegate is injected in BrowserMainLoop::PostMainMessageLoopStart() and kept in a variable until the singleton construction. This CL is the last blocking factor before taking Geolocation out of content/browser and into device. BUG=612334 ========== to ========== Geolocation: change GeolocationDelegate to injected by content/ GeolocationProviderImpl is a singleton that directly references GetContentClient()::browser() on ctor, to retrieve an embedder- provider GeolocationDelegate. This blocks moving Geolocation out of content/ This CL changes that mechanism: the GeolocationDelegate is injected in BrowserMainLoop::PostMainMessageLoopStart() (so it won't affect startup time IIUC), and kept in a variable until the singleton construction. This CL is the last blocking factor before taking Geolocation out of content/browser and into device. BUG=612334 ==========
mcasas@chromium.org changed reviewers: + rockot@chromium.org
rockot@: following up our discussion of changing a direct dependency on content/ with an injected variation, can you plz have a first look? (I understand this needs other OWNERS review).
https://codereview.chromium.org/2127973002/diff/80001/content/browser/geoloca... File content/browser/geolocation/geolocation_provider_impl.cc (right): https://codereview.chromium.org/2127973002/diff/80001/content/browser/geoloca... content/browser/geolocation/geolocation_provider_impl.cc:36: if (!g_delegate.Pointer()) g_delegate.Pointer() is always true. It's the address of the lazy unique_ptr object. You want to test g_delegate.Get(). And really this should be a DCHECK since it's almost definitely an error to call SetGeolocationDelegate more than once in a process. https://codereview.chromium.org/2127973002/diff/80001/content/browser/geoloca... content/browser/geolocation/geolocation_provider_impl.cc:37: g_delegate.Pointer()->reset(delegate); optional nit: I think the above is evidence that using Pointer() on a LazyInstance<unique_ptr<T>> is confusing. IMHO you should just use Get() everywhere. e.g.: g_delegate.Get().reset(delegate);
Description was changed from ========== Geolocation: change GeolocationDelegate to injected by content/ GeolocationProviderImpl is a singleton that directly references GetContentClient()::browser() on ctor, to retrieve an embedder- provider GeolocationDelegate. This blocks moving Geolocation out of content/ This CL changes that mechanism: the GeolocationDelegate is injected in BrowserMainLoop::PostMainMessageLoopStart() (so it won't affect startup time IIUC), and kept in a variable until the singleton construction. This CL is the last blocking factor before taking Geolocation out of content/browser and into device. BUG=612334 ========== to ========== Geolocation: change GeolocationDelegate to injected by content/ GeolocationProviderImpl is a singleton that directly references GetContentClient()::browser() on ctor, to retrieve an embedder- provider GeolocationDelegate. This blocks moving Geolocation out of content/ This CL changes that mechanism: the GeolocationDelegate is injected in BrowserMainLoop::PostMainMessageLoopStart() (so it won't affect startup time IIUC), and kept in a variable until the singleton construction. This CL is the last blocking factor before taking Geolocation out of content/browser and into device. BUG=612334 TEST=./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*" ==========
Patchset #2 (id:100001) has been deleted
Description was changed from ========== Geolocation: change GeolocationDelegate to injected by content/ GeolocationProviderImpl is a singleton that directly references GetContentClient()::browser() on ctor, to retrieve an embedder- provider GeolocationDelegate. This blocks moving Geolocation out of content/ This CL changes that mechanism: the GeolocationDelegate is injected in BrowserMainLoop::PostMainMessageLoopStart() (so it won't affect startup time IIUC), and kept in a variable until the singleton construction. This CL is the last blocking factor before taking Geolocation out of content/browser and into device. BUG=612334 TEST=./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*" ========== to ========== Geolocation: change GeolocationDelegate to injected by content/ GeolocationProviderImpl is a singleton that directly references GetContentClient()::browser() on ctor, to retrieve an embedder- provider GeolocationDelegate. This blocks moving Geolocation out of content/ This CL changes that mechanism: the GeolocationDelegate is injected in BrowserMainLoop::PostMainMessageLoopStart() (so it won't affect startup time IIUC), and kept in a variable until the singleton construction. This CL is the last blocking factor before taking Geolocation out of content/browser and into device. BUG=612334 TEST=./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*" PS: A note about shell_content_browser_client.cc: After changing GeolocationDelegate to injected, component's test AutofillRiskFingerprintTest started failing because the ShellGeolocationDelegate was initialized too early and could not get a correct ShellBrowserContext*. This was OK before for the test, since it would cause a fallback to no-op GeolocationDelegate, but now it would erroneously fetch a ShellGeolocationDelegate with a null ShellBrowserContext*. Change the ShellGeolocationDelegate to get a Callback, hence delaying the moment to fetch |context_| to the appropriate moment, and all use cases are happy. ==========
Description was changed from ========== Geolocation: change GeolocationDelegate to injected by content/ GeolocationProviderImpl is a singleton that directly references GetContentClient()::browser() on ctor, to retrieve an embedder- provider GeolocationDelegate. This blocks moving Geolocation out of content/ This CL changes that mechanism: the GeolocationDelegate is injected in BrowserMainLoop::PostMainMessageLoopStart() (so it won't affect startup time IIUC), and kept in a variable until the singleton construction. This CL is the last blocking factor before taking Geolocation out of content/browser and into device. BUG=612334 TEST=./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*" PS: A note about shell_content_browser_client.cc: After changing GeolocationDelegate to injected, component's test AutofillRiskFingerprintTest started failing because the ShellGeolocationDelegate was initialized too early and could not get a correct ShellBrowserContext*. This was OK before for the test, since it would cause a fallback to no-op GeolocationDelegate, but now it would erroneously fetch a ShellGeolocationDelegate with a null ShellBrowserContext*. Change the ShellGeolocationDelegate to get a Callback, hence delaying the moment to fetch |context_| to the appropriate moment, and all use cases are happy. ========== to ========== Geolocation: change GeolocationDelegate to injected by content/ GeolocationProviderImpl is a singleton that directly references GetContentClient()::browser() on ctor, to retrieve an embedder- provider GeolocationDelegate. This blocks moving Geolocation out of content/ This CL changes that mechanism: the GeolocationDelegate is injected in BrowserMainLoop::PostMainMessageLoopStart() (so it won't affect startup time IIUC), and kept in a variable until the singleton construction. This CL is the last blocking factor before taking Geolocation out of content/browser and into device. BUG=612334 TEST=./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*" PS: A note about shell_content_browser_client.cc: Changing GeolocationDelegate to injected, causes component's AutofillRiskFingerprintTest to start failing because ShellGeolocationDelegate was initialized too early and could not get a correct ShellBrowserContext*. This CL also changes the ShellGeolocationDelegate to get a Callback, hence delaying the moment to fetch |context_| to the appropriate moment, and all use cases are happy. ==========
https://codereview.chromium.org/2127973002/diff/120001/content/shell/browser/... File content/shell/browser/shell_content_browser_client.cc (right): https://codereview.chromium.org/2127973002/diff/120001/content/shell/browser/... content/shell/browser/shell_content_browser_client.cc:376: &ShellContentBrowserClient::browser_context, base::Unretained(this))); This is pretty weird. Why not just get rid of the ShellGeolocationDelegate constructor arg altogether and have CreateAccessTokenStore call ShellGeolocationDelegate::Get()->browser_context()
https://codereview.chromium.org/2127973002/diff/80001/content/browser/geoloca... File content/browser/geolocation/geolocation_provider_impl.cc (right): https://codereview.chromium.org/2127973002/diff/80001/content/browser/geoloca... content/browser/geolocation/geolocation_provider_impl.cc:36: if (!g_delegate.Pointer()) On 2016/07/20 17:47:52, Ken Rockot wrote: > g_delegate.Pointer() is always true. It's the address of the lazy unique_ptr > object. > > You want to test g_delegate.Get(). And really this should be a DCHECK since it's > almost definitely an error to call SetGeolocationDelegate more than once in a > process. Done. https://codereview.chromium.org/2127973002/diff/80001/content/browser/geoloca... content/browser/geolocation/geolocation_provider_impl.cc:37: g_delegate.Pointer()->reset(delegate); On 2016/07/20 17:47:52, Ken Rockot wrote: > optional nit: I think the above is evidence that using Pointer() on a > LazyInstance<unique_ptr<T>> is confusing. IMHO you should just use Get() > everywhere. e.g.: > > g_delegate.Get().reset(delegate); Done.
Description was changed from ========== Geolocation: change GeolocationDelegate to injected by content/ GeolocationProviderImpl is a singleton that directly references GetContentClient()::browser() on ctor, to retrieve an embedder- provider GeolocationDelegate. This blocks moving Geolocation out of content/ This CL changes that mechanism: the GeolocationDelegate is injected in BrowserMainLoop::PostMainMessageLoopStart() (so it won't affect startup time IIUC), and kept in a variable until the singleton construction. This CL is the last blocking factor before taking Geolocation out of content/browser and into device. BUG=612334 TEST=./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*" PS: A note about shell_content_browser_client.cc: Changing GeolocationDelegate to injected, causes component's AutofillRiskFingerprintTest to start failing because ShellGeolocationDelegate was initialized too early and could not get a correct ShellBrowserContext*. This CL also changes the ShellGeolocationDelegate to get a Callback, hence delaying the moment to fetch |context_| to the appropriate moment, and all use cases are happy. ========== to ========== Geolocation: change GeolocationDelegate to injected by content/ GeolocationProviderImpl is a singleton that directly references GetContentClient()::browser() on ctor, to retrieve an embedder- provider GeolocationDelegate. This blocks moving Geolocation out of content/ This CL changes that mechanism: the GeolocationDelegate is injected in BrowserMainLoop::PostMainMessageLoopStart() (so it won't affect startup time IIUC), and kept in a variable until the singleton construction. This CL is the last blocking factor before taking Geolocation out of content/browser and into device. BUG=612334 TEST=./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*" PS: A note about shell_content_browser_client.cc: Changing GeolocationDelegate to injected, causes component's AutofillRiskFingerprintTest to start failing because ShellGeolocationDelegate was initialized too early and could not get a correct ShellBrowserContext*. This CL also changes the ShellGeolocationDelegate to get the browser_context() when is needed. ==========
mcasas@chromium.org changed reviewers: + jam@chromium.org
jam@ PTAL/RS rockot@: all done, thanks! https://codereview.chromium.org/2127973002/diff/120001/content/shell/browser/... File content/shell/browser/shell_content_browser_client.cc (right): https://codereview.chromium.org/2127973002/diff/120001/content/shell/browser/... content/shell/browser/shell_content_browser_client.cc:376: &ShellContentBrowserClient::browser_context, base::Unretained(this))); On 2016/07/20 20:47:04, Ken Rockot wrote: > This is pretty weird. Why not just get rid of the ShellGeolocationDelegate > constructor arg altogether and have CreateAccessTokenStore call > ShellGeolocationDelegate::Get()->browser_context() Indeed why not! Done.
Let's remove the ContentBrowserClient method to set GeolocationDelegate though.
lgtm (as we discussed in person, this will be done in follow-ups)
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Geolocation: change GeolocationDelegate to injected by content/ GeolocationProviderImpl is a singleton that directly references GetContentClient()::browser() on ctor, to retrieve an embedder- provider GeolocationDelegate. This blocks moving Geolocation out of content/ This CL changes that mechanism: the GeolocationDelegate is injected in BrowserMainLoop::PostMainMessageLoopStart() (so it won't affect startup time IIUC), and kept in a variable until the singleton construction. This CL is the last blocking factor before taking Geolocation out of content/browser and into device. BUG=612334 TEST=./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*" PS: A note about shell_content_browser_client.cc: Changing GeolocationDelegate to injected, causes component's AutofillRiskFingerprintTest to start failing because ShellGeolocationDelegate was initialized too early and could not get a correct ShellBrowserContext*. This CL also changes the ShellGeolocationDelegate to get the browser_context() when is needed. ========== to ========== Geolocation: change GeolocationDelegate to injected by content/ GeolocationProviderImpl is a singleton that directly references GetContentClient()::browser() on ctor, to retrieve an embedder- provider GeolocationDelegate. This blocks moving Geolocation out of content/ This CL changes that mechanism: the GeolocationDelegate is injected in BrowserMainLoop::PostMainMessageLoopStart() (so it won't affect startup time IIUC), and kept in a variable until the singleton construction. This CL is the last blocking factor before taking Geolocation out of content/browser and into device. BUG=612334 TEST=./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*" PS: A note about shell_content_browser_client.cc: Changing GeolocationDelegate to injected, causes component's AutofillRiskFingerprintTest to start failing because ShellGeolocationDelegate was initialized too early and could not get a correct ShellBrowserContext*. This CL also changes the ShellGeolocationDelegate to get the browser_context() when is needed. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Geolocation: change GeolocationDelegate to injected by content/ GeolocationProviderImpl is a singleton that directly references GetContentClient()::browser() on ctor, to retrieve an embedder- provider GeolocationDelegate. This blocks moving Geolocation out of content/ This CL changes that mechanism: the GeolocationDelegate is injected in BrowserMainLoop::PostMainMessageLoopStart() (so it won't affect startup time IIUC), and kept in a variable until the singleton construction. This CL is the last blocking factor before taking Geolocation out of content/browser and into device. BUG=612334 TEST=./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*" PS: A note about shell_content_browser_client.cc: Changing GeolocationDelegate to injected, causes component's AutofillRiskFingerprintTest to start failing because ShellGeolocationDelegate was initialized too early and could not get a correct ShellBrowserContext*. This CL also changes the ShellGeolocationDelegate to get the browser_context() when is needed. ========== to ========== Geolocation: change GeolocationDelegate to injected by content/ GeolocationProviderImpl is a singleton that directly references GetContentClient()::browser() on ctor, to retrieve an embedder- provider GeolocationDelegate. This blocks moving Geolocation out of content/ This CL changes that mechanism: the GeolocationDelegate is injected in BrowserMainLoop::PostMainMessageLoopStart() (so it won't affect startup time IIUC), and kept in a variable until the singleton construction. This CL is the last blocking factor before taking Geolocation out of content/browser and into device. BUG=612334 TEST=./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*" PS: A note about shell_content_browser_client.cc: Changing GeolocationDelegate to injected, causes component's AutofillRiskFingerprintTest to start failing because ShellGeolocationDelegate was initialized too early and could not get a correct ShellBrowserContext*. This CL also changes the ShellGeolocationDelegate to get the browser_context() when is needed. Committed: https://crrev.com/19677cb9f1901f0eb7881380ac85a0255195e06d Cr-Commit-Position: refs/heads/master@{#407015} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/19677cb9f1901f0eb7881380ac85a0255195e06d Cr-Commit-Position: refs/heads/master@{#407015}
Message was sent while issue was closed.
wez@chromium.org changed reviewers: + wez@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2127973002/diff/80001/content/browser/geoloca... File content/browser/geolocation/geolocation_provider_impl.cc (right): https://codereview.chromium.org/2127973002/diff/80001/content/browser/geoloca... content/browser/geolocation/geolocation_provider_impl.cc:25: LAZY_INSTANCE_INITIALIZER; Why do we need a LazyInstance here, rather than a bare-pointer?
Message was sent while issue was closed.
https://codereview.chromium.org/2127973002/diff/80001/content/browser/geoloca... File content/browser/geolocation/geolocation_provider_impl.cc (right): https://codereview.chromium.org/2127973002/diff/80001/content/browser/geoloca... content/browser/geolocation/geolocation_provider_impl.cc:25: LAZY_INSTANCE_INITIALIZER; On 2016/07/25 22:25:35, Wez wrote: > Why do we need a LazyInstance here, rather than a bare-pointer? Essentially, because I wanted the |g_delegate| to clean itself up during shutdown. Using a naked pointer would have implied making it a member variable of GeolocationProvider to clean it up, but that'd have forced the creation of the (evil?) singleton...
Message was sent while issue was closed.
https://codereview.chromium.org/2127973002/diff/80001/content/browser/geoloca... File content/browser/geolocation/geolocation_provider_impl.cc (right): https://codereview.chromium.org/2127973002/diff/80001/content/browser/geoloca... content/browser/geolocation/geolocation_provider_impl.cc:25: LAZY_INSTANCE_INITIALIZER; On 2016/07/26 18:46:34, mcasas wrote: > On 2016/07/25 22:25:35, Wez wrote: > > Why do we need a LazyInstance here, rather than a bare-pointer? > > Essentially, because I wanted the |g_delegate| to clean > itself up during shutdown. Using a naked pointer would > have implied making it a member variable of > GeolocationProvider to clean it up, but that'd have forced > the creation of the (evil?) singleton... If you want it cleaned up on shutdown then you either want a Singleton or a non-Leaky LazyInstance. What you have right now won't clean up on shutdown, AFAIK. Also I'm not sure that we actually instantiate AtExitManager in the Browser process; I think we may deliberately skip destructing Singletons etc on browser teardown because it usually just generates unnecessary memory paging, to no purpose. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
