Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
603323004:
DevTools: Add geolocation override in browser (Closed)
|
Created:
6 years, 2 months ago by vkuzkokov Modified:
6 years, 2 months ago CC:
chromium-reviews, vsevik, jam, yurys, paulirish+reviews_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org, pfeldman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Add geolocation override in browser
Geolocation overrides moves from renderer to browser side.
BUG=404995
Committed: https://crrev.com/e15021e8a8dab4f367629c24e29e2e8d23d38c51
Cr-Commit-Position: refs/heads/master@{#298854}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments. Fix in PauseOrResume #
Total comments: 2
Patch Set 3 : Rebased. Expanded SetOverride description. #
Total comments: 3
Messages
Total messages: 25 (7 generated)
vkuzkokov@chromium.org changed reviewers: + dgozman@chromium.org
This looks good. The only problem I see is that you don't immediately get the right geoposition when clearing override, but that's fine. https://codereview.chromium.org/603323004/diff/1/content/browser/devtools/pro... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/603323004/diff/1/content/browser/devtools/pro... content/browser/devtools/protocol/page_handler.cc:67: GeolocationDispatcherHost* geolocation_host = if (!web_contents) return Response::InternalError("No WebContents to override"); https://codereview.chromium.org/603323004/diff/1/content/browser/devtools/pro... content/browser/devtools/protocol/page_handler.cc:78: geolocation_host->SetOverride(geoposition.Pass()); Could |geolocation_host| be NULL? https://codereview.chromium.org/603323004/diff/1/content/browser/devtools/pro... content/browser/devtools/protocol/page_handler.cc:83: WebContentsImpl* web_contents = static_cast<WebContentsImpl*>( Check |web_contents| and maybe |geolocation_host|. https://codereview.chromium.org/603323004/diff/1/content/browser/devtools/pro... File content/browser/devtools/protocol/page_handler.h (right): https://codereview.chromium.org/603323004/diff/1/content/browser/devtools/pro... content/browser/devtools/protocol/page_handler.h:41: Response SetGeolocationOverride(double* latitude, nit: either retain empty lines between methods or group them by semantics.
mvanouwerkerk@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
https://codereview.chromium.org/603323004/diff/1/content/browser/devtools/pro... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/603323004/diff/1/content/browser/devtools/pro... content/browser/devtools/protocol/page_handler.cc:78: geolocation_host->SetOverride(geoposition.Pass()); On 2014/10/06 11:03:26, dgozman wrote: > Could |geolocation_host| be NULL? It should really never be, it is instantiated in WebContentsImpl::Init https://codereview.chromium.org/603323004/diff/1/content/browser/geolocation/... File content/browser/geolocation/geolocation_dispatcher_host.h (right): https://codereview.chromium.org/603323004/diff/1/content/browser/geolocation/... content/browser/geolocation/geolocation_dispatcher_host.h:34: void SetOverride(scoped_ptr<Geoposition> geoposition); Why do you need to introduce a new way to override geolocation? Why is GeolocationProviderImpl::OverrideLocationForTesting not good enough?
https://codereview.chromium.org/603323004/diff/1/content/browser/devtools/pro... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/603323004/diff/1/content/browser/devtools/pro... content/browser/devtools/protocol/page_handler.cc:67: GeolocationDispatcherHost* geolocation_host = On 2014/10/06 11:03:26, dgozman wrote: > if (!web_contents) > return Response::InternalError("No WebContents to override"); Done. https://codereview.chromium.org/603323004/diff/1/content/browser/devtools/pro... content/browser/devtools/protocol/page_handler.cc:83: WebContentsImpl* web_contents = static_cast<WebContentsImpl*>( On 2014/10/06 11:03:26, dgozman wrote: > Check |web_contents| and maybe |geolocation_host|. Done. https://codereview.chromium.org/603323004/diff/1/content/browser/devtools/pro... File content/browser/devtools/protocol/page_handler.h (right): https://codereview.chromium.org/603323004/diff/1/content/browser/devtools/pro... content/browser/devtools/protocol/page_handler.h:41: Response SetGeolocationOverride(double* latitude, On 2014/10/06 11:03:26, dgozman wrote: > nit: either retain empty lines between methods or group them by semantics. Done. https://codereview.chromium.org/603323004/diff/1/content/browser/geolocation/... File content/browser/geolocation/geolocation_dispatcher_host.h (right): https://codereview.chromium.org/603323004/diff/1/content/browser/geolocation/... content/browser/geolocation/geolocation_dispatcher_host.h:34: void SetOverride(scoped_ptr<Geoposition> geoposition); On 2014/10/06 13:38:23, Michael van Ouwerkerk wrote: > Why do you need to introduce a new way to override geolocation? Why is > GeolocationProviderImpl::OverrideLocationForTesting not good enough? OverrideLocationForTesting works on all browser. We need to override on per client basis.
devtools lgtm. We should add "omitting all the parameters emulates position unavailable" to protocol.json - that's not obvious.
vkuzkokov@chromium.org changed reviewers: + pfeldman@chromium.org
On 2014/10/07 07:22:51, dgozman wrote: > devtools lgtm. > > We should add "omitting all the parameters emulates position unavailable" to > protocol.json - that's not obvious. Pavel, PTAL content/browser/devtools/protocol/
> https://codereview.chromium.org/603323004/diff/1/content/browser/geolocation/... > File content/browser/geolocation/geolocation_dispatcher_host.h (right): > > https://codereview.chromium.org/603323004/diff/1/content/browser/geolocation/... > content/browser/geolocation/geolocation_dispatcher_host.h:34: void > SetOverride(scoped_ptr<Geoposition> geoposition); > On 2014/10/06 13:38:23, Michael van Ouwerkerk wrote: > > Why do you need to introduce a new way to override geolocation? Why is > > GeolocationProviderImpl::OverrideLocationForTesting not good enough? > > OverrideLocationForTesting works on all browser. We need to override on per > client basis. Michael, just making sure you didn't miss answer to your question.
On 2014/10/07 11:43:31, vkuzkokov wrote: > > > https://codereview.chromium.org/603323004/diff/1/content/browser/geolocation/... > > File content/browser/geolocation/geolocation_dispatcher_host.h (right): > > > > > https://codereview.chromium.org/603323004/diff/1/content/browser/geolocation/... > > content/browser/geolocation/geolocation_dispatcher_host.h:34: void > > SetOverride(scoped_ptr<Geoposition> geoposition); > > On 2014/10/06 13:38:23, Michael van Ouwerkerk wrote: > > > Why do you need to introduce a new way to override geolocation? Why is > > > GeolocationProviderImpl::OverrideLocationForTesting not good enough? > > > > OverrideLocationForTesting works on all browser. We need to override on per > > client basis. > > Michael, just making sure you didn't miss answer to your question. Yep, I saw that. Thanks.
lgtm https://codereview.chromium.org/603323004/diff/20001/content/browser/geolocat... File content/browser/geolocation/geolocation_dispatcher_host.h (right): https://codereview.chromium.org/603323004/diff/20001/content/browser/geolocat... content/browser/geolocation/geolocation_dispatcher_host.h:33: // Enables geolocation override. Please document who is expected / allowed to call this method.
https://codereview.chromium.org/603323004/diff/20001/content/browser/geolocat... File content/browser/geolocation/geolocation_dispatcher_host.h (right): https://codereview.chromium.org/603323004/diff/20001/content/browser/geolocat... content/browser/geolocation/geolocation_dispatcher_host.h:33: // Enables geolocation override. On 2014/10/08 14:40:41, Michael van Ouwerkerk wrote: > Please document who is expected / allowed to call this method. Done.
The CQ bit was checked by vkuzkokov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/603323004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by vkuzkokov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/603323004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as 68479c8943e5e6e83041c7a62f970f2217bdf158
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e15021e8a8dab4f367629c24e29e2e8d23d38c51 Cr-Commit-Position: refs/heads/master@{#298854}
Message was sent while issue was closed.
blundell@chromium.org changed reviewers: + blundell@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/603323004/diff/40001/content/browser/geolocat... File content/browser/geolocation/geolocation_dispatcher_host.cc (right): https://codereview.chromium.org/603323004/diff/40001/content/browser/geolocat... content/browser/geolocation/geolocation_dispatcher_host.cc:232: if (geoposition_override_.get()) drive-by: Naively, it seems like the code at lines 232 and 233 should execute only when |!paused_|. Is that correct? If so, no need to do a follow-up CL; I'll take care of it when porting this flow to the Mojo impl in https://codereview.chromium.org/628773003/.
Message was sent while issue was closed.
https://codereview.chromium.org/603323004/diff/40001/content/browser/geolocat... File content/browser/geolocation/geolocation_dispatcher_host.cc (right): https://codereview.chromium.org/603323004/diff/40001/content/browser/geolocat... content/browser/geolocation/geolocation_dispatcher_host.cc:232: if (geoposition_override_.get()) On 2014/10/21 08:44:20, blundell wrote: > drive-by: Naively, it seems like the code at lines 232 and 233 should execute > only when |!paused_|. Is that correct? If so, no need to do a follow-up CL; I'll > take care of it when porting this flow to the Mojo impl in > https://codereview.chromium.org/628773003/. There is a check in OnLocationUpdate. This doesn't seem ot matter for your CL though.
Message was sent while issue was closed.
Thanks! https://codereview.chromium.org/603323004/diff/40001/content/browser/geolocat... File content/browser/geolocation/geolocation_dispatcher_host.cc (right): https://codereview.chromium.org/603323004/diff/40001/content/browser/geolocat... content/browser/geolocation/geolocation_dispatcher_host.cc:232: if (geoposition_override_.get()) On 2014/10/21 11:58:27, vkuzkokov wrote: > On 2014/10/21 08:44:20, blundell wrote: > > drive-by: Naively, it seems like the code at lines 232 and 233 should execute > > only when |!paused_|. Is that correct? If so, no need to do a follow-up CL; > I'll > > take care of it when porting this flow to the Mojo impl in > > https://codereview.chromium.org/628773003/. > > There is a check in OnLocationUpdate. This doesn't seem ot matter for your CL > though. Ah right, I had forgotten about that. It matters for my CL only because I wanted to ensure that I'm correctly porting the override behavior to the Mojo impl. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
