Consider making OverrideSystemLocationProvider return std::unique_ptr<LocationProvider> |
||||||
Issue descriptionLocationProviders are created for Geolocation via ContentBrowserClient's method virtual LocationProvider* OverrideSystemLocationProvider() its implementations in BlimpContentBrowserClient [2], unittests [3] does not really override anything, but: - returns a pointer to a local member object [2,3] that is subsequently base::WrapUnique()d [4] !! Consider returning an std::uniqueptr<> to clarify ownership, and perhaps rename. wez@ suggested a signature: std::unique_ptr<> GetCustomSystemLocationProvider() [1] https://cs.chromium.org/chromium/src/content/public/browser/content_browser_client.h?q=virtual+LocationProvider*+OverrideSystemLocationProvider%5C(%5C)&sq=package:chromium&l=631&dr=C [2] https://cs.chromium.org/chromium/src/blimp/engine/app/blimp_content_browser_client.cc?sq=package:chromium&dr=C&rcl=1466767076&l=43 [3] https://cs.chromium.org/chromium/src/content/browser/geolocation/location_arbitrator_impl_unittest.cc?sq=package:chromium&dr=C&rcl=1466767076&l=75 [4] https://cs.chromium.org/chromium/src/content/browser/geolocation/location_arbitrator_impl.cc?sq=package:chromium&dr=C&rcl=1466767076&l=126
,
Jul 8 2016
,
Jul 8 2016
,
Jul 11 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f6da183fd9aa098f57e04f55c589db3eb31786f5 commit f6da183fd9aa098f57e04f55c589db3eb31786f5 Author: mcasas <mcasas@chromium.org> Date: Mon Jul 11 18:28:07 2016 Geolocation cleanup: make GeolocationDelegate::OverrideSystemLocationProvider return unique_ptr This CL changes the signature of GeolocationDelegate's virtual LocationProvider* OverrideSystemLocationProvider(); to virtual std::unique_ptr<LocationProvider> OverrideSystemLocationProvider(); Most of the code changes are trivial except in the unittests where the lifetime of the generated LocationProvider needs to be taken into account. BUG= 623132 TEST=all relevant unittests and browser_tests working, in particular ./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*" ./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2126893003 Cr-Commit-Position: refs/heads/master@{#404693} [modify] https://crrev.com/f6da183fd9aa098f57e04f55c589db3eb31786f5/blimp/engine/app/blimp_content_browser_client.cc [modify] https://crrev.com/f6da183fd9aa098f57e04f55c589db3eb31786f5/content/browser/geolocation/location_arbitrator_impl.cc [modify] https://crrev.com/f6da183fd9aa098f57e04f55c589db3eb31786f5/content/browser/geolocation/location_arbitrator_impl.h [modify] https://crrev.com/f6da183fd9aa098f57e04f55c589db3eb31786f5/content/browser/geolocation/location_arbitrator_impl_unittest.cc [modify] https://crrev.com/f6da183fd9aa098f57e04f55c589db3eb31786f5/content/public/browser/geolocation_delegate.cc [modify] https://crrev.com/f6da183fd9aa098f57e04f55c589db3eb31786f5/content/public/browser/geolocation_delegate.h
,
Jul 11 2016
,
Sep 22 2017
,
Sep 22 2017
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by mcasas@chromium.org
, Jun 24 2016