Get the WebGeolocationClient from WebFrameClient instead of WebViewClient.

Get the WebGeolocationClient from WebFrameClient instead of WebViewClient.

BUG=304341
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172912

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175404

[jam, 2014-04-25 21:04:55]

actually, hold off on the review. webkit_unit_tests failures brought my
attention to a problem.

[jam, 2014-04-28 18:08:39]

changing reviewers to pfeldman

Pavel: I've change the patch so that GeolocationInspectorAgent is per page as
before, per our email thread.

[pfeldman, 2014-04-28 19:09:15]

thanks, lgtm

[jam, 2014-04-29 00:16:09]

On 2014/04/28 19:09:15, pfeldman wrote:
> thanks, lgtm

ptal, I've fixed a crash that webkit_unit_tests exposed on shutdown, since the
frame (and its GeolocatoinController) outlives the Page so we need a null check.

Since the internals object for geolocation matches the controller, I've updated
fast/dom/Geolocation/disconnected-frame-permission-denied.html to mock the
geolocation on the iframe.

I've removed fast/dom/Geolocation/cached-position-iframe.html since it only made
sense when we had one GeolocationController that is shared across frames.
However this area of the code is tested in browser tests, i.e.
GeolocationBrowserTest.IFramesWithCachedPosition.

[Michael van Ouwerkerk, 2014-04-29 15:06:19]

Thanks John! lgtm with a minor nit

https://codereview.chromium.org/256843004/diff/60001/Source/modules/geolocati...
File Source/modules/geolocation/Geolocation.h (right):

https://codereview.chromium.org/256843004/diff/60001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.h:47: class Page;
Please delete this.

[jam, 2014-04-29 15:46:01]

https://codereview.chromium.org/256843004/diff/60001/Source/modules/geolocati...
File Source/modules/geolocation/Geolocation.h (right):

https://codereview.chromium.org/256843004/diff/60001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.h:47: class Page;
On 2014/04/29 15:06:19, Michael van Ouwerkerk wrote:
> Please delete this.

Done.

[jam, 2014-04-29 15:47:07]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2014-04-29 15:47:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jam@chromium.org/256843004/80001

[commit-bot: I haz the power, 2014-04-29 16:48:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 16:48:33]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink

[jam, 2014-04-29 17:05:00]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2014-04-29 17:05:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jam@chromium.org/256843004/80001

[commit-bot: I haz the power, 2014-04-30 04:13:26]

Change committed as 172912

[falken, 2014-04-30 05:51:06]

On 2014/04/30 04:13:26, I haz the power (commit-bot) wrote:
> Change committed as 172912

Sorry to revert this change. It looks like it broke several Geolocation browser
tests:
CancelPermissionForFrame
IFramesWithCachedPosition
IFramesWithFreshPosition
NoInfoBarBeforeStart
GeolocationAPIEmbedderHasAccessMultipleBridgeIdAllow

See
http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/36368

[jam, 2014-05-30 22:01:31]

mvanouwerkerk: ptal at the changes in Source/modules/geolocation. This change
had gotten reverted because it ended up breaking browser tests on the chromium
side. My original plan was to land the blink side first, then update content.
But it's not going to work here. So first I'm going to land
https://codereview.chromium.org/303503008/ to make content-side handle per-frame
geolocation callbacks (which also is blocked on this tiny blink change
https://codereview.chromium.org/301183005/). Then I will land this cl.

The main difference from the reverted cl is that I ensure that there is only one
GeolocationClientMock per page now. This brokeness would have been obvious
before had I landed patchset 4 first and then the chrome side, since the layout
tests would fail. The reason is that the tests expect to change the geolocation
settings once and have that impact all the frames.

[Michael van Ouwerkerk, 2014-06-02 21:18:00]

lgtm Thanks John!

[jam, 2014-06-02 22:12:46]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2014-06-02 22:13:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jam@chromium.org/256843004/120001

[jam, 2014-06-02 22:54:19]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2014-06-02 22:54:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jam@chromium.org/256843004/140001

[commit-bot: I haz the power, 2014-06-03 00:18:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9948)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/11975)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/15063)

[commit-bot: I haz the power, 2014-06-03 00:46:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-03 00:46:22]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[jam, 2014-06-03 17:45:44]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2014-06-03 17:46:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jam@chromium.org/256843004/160001

[commit-bot: I haz the power, 2014-06-03 18:58:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10109)

[commit-bot: I haz the power, 2014-06-03 20:23:59]

The CQ bit was unchecked by commit-bot@chromium.org

[jam, 2014-06-03 20:35:07]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2014-06-03 20:35:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jam@chromium.org/256843004/160001

[commit-bot: I haz the power, 2014-06-03 20:37:40]

Change committed as 175404

[kenrb, 2014-06-04 20:28:48]

jam: I ran into this with my FrameTree changes...

https://codereview.chromium.org/256843004/diff/160001/Source/modules/geolocat...
File Source/modules/geolocation/testing/InternalsGeolocation.cpp (right):

https://codereview.chromium.org/256843004/diff/160001/Source/modules/geolocat...
Source/modules/geolocation/testing/InternalsGeolocation.cpp:50: for (LocalFrame*
childFrame = document->page()->mainFrame(); childFrame; childFrame =
childFrame->tree().nextSibling())
This loop looks to be entirely redundant, since childFrame is not referenced in
the body. Is it not supposed to be here, or is the loop body wrong?