Move GeolocationDispatcher into blink.

Move GeolocationDispatcher into blink.

This also changes the layout tests for geolocation to mock out the mojo
interface directly from the JS layout test code, removing the need for
C++ code for a mock and its control.

BUG=561879
Committed: https://crrev.com/f48785c4922df0b6aa3fe1bae756f8c54eb6c899
Cr-Commit-Position: refs/heads/master@{#393452}

[Sam McNally, 2015-09-25 05:06:33]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2015-09-25 06:23:07]

Patchset #4 (id:80001) has been deleted

[Sam McNally, 2015-09-25 06:23:16]

Patchset #4 (id:100001) has been deleted

[Sam McNally, 2015-09-25 06:23:24]

Patchset #3 (id:60001) has been deleted

[Sam McNally, 2015-09-28 03:57:10]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2015-09-28 03:57:19]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2015-09-28 07:07:26]

Patchset #1 (id:120001) has been deleted

[Sam McNally, 2015-09-28 07:07:36]

Patchset #1 (id:140001) has been deleted

[Ken Rockot(use gerrit already), 2015-09-29 19:20:44]

rockot@chromium.org changed reviewers:
+ rockot@chromium.org

[Ken Rockot(use gerrit already), 2015-09-29 19:20:45]

https://codereview.chromium.org/1367853002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/BUILD.gn (right):

https://codereview.chromium.org/1367853002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/BUILD.gn:55: "//content/common:mojo_bindings",
Looks like this is not in sync with PS1 and should refer to
//components/geolocation and //components/permission. Same with the actual
#includes.

[Sam McNally, 2015-09-30 01:23:21]

Patchset #4 (id:220001) has been deleted

[Sam McNally, 2015-09-30 01:49:52]

Patchset #4 (id:240001) has been deleted

[Sam McNally, 2015-09-30 01:51:06]

https://codereview.chromium.org/1367853002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/BUILD.gn (right):

https://codereview.chromium.org/1367853002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/BUILD.gn:55: "//content/common:mojo_bindings",
On 2015/09/29 19:20:45, Ken Rockot wrote:
> Looks like this is not in sync with PS1 and should refer to
> //components/geolocation and //components/permission. Same with the actual
> #includes.

I've split the mojom move into https://codereview.chromium.org/1373883003/ so we
can have the correct includes here without the unrelated changes.

[Marijn Kruisselbrink, 2015-09-30 19:56:54]

mek@chromium.org changed reviewers:
+ mek@chromium.org

[Marijn Kruisselbrink, 2015-09-30 19:56:55]

https://codereview.chromium.org/1367853002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/1367853002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:96: ,
m_controller(new GeolocationController(this))
Unless I'm missing something, the change to lifetime/ownership of
GeolocationController is completely unrelated to the changes in this CL?
It might definitely make sense to make such a change (and I think it might even
be mostly equivalent), but mixing the two refactors in one CL makes it a bit
hard to see which changes are for what reason.

I can think of a few reasons why you might have ended up mixing these two up,
but they're all for changes in behavior that should probably be separate CLs as
well.

Also this seems like it could be the cause of the layout tests you deleted no
longer passing?

https://codereview.chromium.org/1367853002/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/geolocation/testing/GeolocationClientMock.cpp
(right):

https://codereview.chromium.org/1367853002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/geolocation/testing/GeolocationClientMock.cpp:48:
document->setServiceProvider(supplement);
The constructor of GeolocationClientMock would have already called this; pick
which code should be responsible for calling it...

https://codereview.chromium.org/1367853002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/geolocation/testing/GeolocationClientMock.cpp:153:
{
While there is only one feature that uses mojo from blink just overwriting the
existing service provider completely works of course, but as soon as other code
starts relying on mojo as well it seems like any mock service provider should
only intercept requests for some services, and pass through connections to other
services to whatever service provider was in place already. And ideally mock
service providers could be created and destroyed in different orders, while
still keeping things working...

Having multiple features that all want to intercept the permission service for
their own feature of course gets even more complicated, not sure what the best
thing to do for that would be...

[Sam McNally, 2015-10-01 00:43:32]

Patchset #5 (id:280001) has been deleted

[Sam McNally, 2015-10-01 01:54:50]

https://codereview.chromium.org/1367853002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/1367853002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:96: ,
m_controller(new GeolocationController(this))
On 2015/09/30 19:56:55, Marijn Kruisselbrink wrote:
> Unless I'm missing something, the change to lifetime/ownership of
> GeolocationController is completely unrelated to the changes in this CL?
> It might definitely make sense to make such a change (and I think it might
even
> be mostly equivalent), but mixing the two refactors in one CL makes it a bit
> hard to see which changes are for what reason.

For a real CL it's worth splitting this up, but I think it's useful here for
examining how much simplification switching to mojo allows.
> 
> I can think of a few reasons why you might have ended up mixing these two up,
> but they're all for changes in behavior that should probably be separate CLs
as
> well.
> 
> Also this seems like it could be the cause of the layout tests you deleted no
> longer passing?

That test assumed all subframes shared a GeolocationClient with the parent
frame, which was only true due to the test setup.

https://codereview.chromium.org/1367853002/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/geolocation/testing/GeolocationClientMock.cpp
(right):

https://codereview.chromium.org/1367853002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/geolocation/testing/GeolocationClientMock.cpp:48:
document->setServiceProvider(supplement);
On 2015/09/30 19:56:55, Marijn Kruisselbrink wrote:
> The constructor of GeolocationClientMock would have already called this; pick
> which code should be responsible for calling it...

Done.

https://codereview.chromium.org/1367853002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/geolocation/testing/GeolocationClientMock.cpp:153:
{
On 2015/09/30 19:56:55, Marijn Kruisselbrink wrote:
> While there is only one feature that uses mojo from blink just overwriting the
> existing service provider completely works of course, but as soon as other
code
> starts relying on mojo as well it seems like any mock service provider should
> only intercept requests for some services, and pass through connections to
other
> services to whatever service provider was in place already. And ideally mock
> service providers could be created and destroyed in different orders, while
> still keeping things working...
> 
This is a bit of a quick and nasty ServiceProvider mock. I think in the longer
term we'd use a common mock ServiceProvider that tests for any mojo-backed API
would use. That would forward any unhandled requests to the normal
ServiceProvider.

> Having multiple features that all want to intercept the permission service for
> their own feature of course gets even more complicated, not sure what the best
> thing to do for that would be...

Tests that use multiple mojo-backed APIs that share a backend could implement
custom-tailored mocks that can deal with both. In particular, when we get to
converting the Permission API, perhaps a general(ish)-purpose permission service
mock could be written that the geolocation tests could also use.

[Marijn Kruisselbrink, 2015-10-01 03:36:09]

https://codereview.chromium.org/1367853002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/1367853002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:96: ,
m_controller(new GeolocationController(this))
On 2015/10/01 at 01:54:50, Sam McNally wrote:
> On 2015/09/30 19:56:55, Marijn Kruisselbrink wrote:
> > Unless I'm missing something, the change to lifetime/ownership of
> > GeolocationController is completely unrelated to the changes in this CL?
> > It might definitely make sense to make such a change (and I think it might
even
> > be mostly equivalent), but mixing the two refactors in one CL makes it a bit
> > hard to see which changes are for what reason.
> 
> For a real CL it's worth splitting this up, but I think it's useful here for
examining how much simplification switching to mojo allows.

But changing the ownership and lifetime management for GeolocationController has
nothing whatsoever to do with switching to mojo. Both changes are completely
independent and by having them both in the CL it just becomes harder to see what
simplification switching to mojo allows.

> > 
> > I can think of a few reasons why you might have ended up mixing these two
up,
> > but they're all for changes in behavior that should probably be separate CLs
as
> > well.
> > 
> > Also this seems like it could be the cause of the layout tests you deleted
no
> > longer passing?
> 
> That test assumed all subframes shared a GeolocationClient with the parent
frame, which was only true due to the test setup.

Yes, and there is no reason to mix up changing that test setup with this CL. If
you keep GeolocationController to be tied to the Frame, you can keep using the
exact same test setup. No need to mix changing that part of the design and that
part of the test setup with the rest of this CL.

[Sam McNally, 2015-10-02 05:12:38]

https://codereview.chromium.org/1367853002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/1367853002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:96: ,
m_controller(new GeolocationController(this))
On 2015/10/01 03:36:09, Marijn Kruisselbrink wrote:
> On 2015/10/01 at 01:54:50, Sam McNally wrote:
> > On 2015/09/30 19:56:55, Marijn Kruisselbrink wrote:
> > > Unless I'm missing something, the change to lifetime/ownership of
> > > GeolocationController is completely unrelated to the changes in this CL?
> > > It might definitely make sense to make such a change (and I think it might
> even
> > > be mostly equivalent), but mixing the two refactors in one CL makes it a
bit
> > > hard to see which changes are for what reason.
> > 
> > For a real CL it's worth splitting this up, but I think it's useful here for
> examining how much simplification switching to mojo allows.
> 
> But changing the ownership and lifetime management for GeolocationController
has
> nothing whatsoever to do with switching to mojo. Both changes are completely
> independent and by having them both in the CL it just becomes harder to see
what
> simplification switching to mojo allows.

Changing ownership of GeolocationController depends on switching to using mojo.
In fact, switching to mojo enables removing GeolocationController (and
GeolocationPosition) entirely.
> 
> > > 
> > > I can think of a few reasons why you might have ended up mixing these two
> up,
> > > but they're all for changes in behavior that should probably be separate
CLs
> as
> > > well.
> > > 
> > > Also this seems like it could be the cause of the layout tests you deleted
> no
> > > longer passing?
> > 
> > That test assumed all subframes shared a GeolocationClient with the parent
> frame, which was only true due to the test setup.
> 
> Yes, and there is no reason to mix up changing that test setup with this CL.
If
> you keep GeolocationController to be tied to the Frame, you can keep using the
> exact same test setup. No need to mix changing that part of the design and
that
> part of the test setup with the rest of this CL.

The test change is independent of the change to GeolocationController ownership.
That test relied on the GeolocationClientMock being shared by
GeolocationControllers from different frames and being able to return cached
results synchronously. With the removal of GeolocationClient, this is no longer
possible.

[Sam McNally, 2016-01-12 02:34:07]

Description was changed from

==========
Mojo in blink prototype: geolocation.

tl;dr:
- use mojo from Geolocation.{cpp,h}
- expose mojo::ServiceProvider to blink::ExecutionContext via WebFrameClient
- move mojoms into components: https://codereview.chromium.org/1373883003

This changes Geolocation to use mojo to communicate directly with its
backend instead of the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation. This removes all WebGeolocation*.

To expose mojo to blink, WebFrameClient provides a mojo::ServiceProvider
getter; once the Document is created, the ServiceProvider is installed
on the ExecutionContext so Geolocation can access it. The
ServiceProvider forwards to the RenderFrame ServiceRegistry, which
forwards to services registered in the browser process. The same pattern
could be used in WebSharedWorkerClient and WebServiceWorkerContextClient
to expose mojo services to workers.

To avoid circular dependencies between blink and content, the mojom for
geolocation and permissions have been moved into components. In this
change each component only contains the public mojo interfaces, but in
the future, the implementations could also move into the components.
Due to the large number of unrelated changes this involes, this is
actually in https://codereview.chromium.org/1373883003.

Due to incompatibility between mojo::Callback and WTF::Function, the
mojo calls currently use lambdas to forward responses to their handling
methods; in this case this is safe since the "this" bound into the
lambda owns the service, so the lambda can never be called with a
dangling this.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationClientMock now replaces the
ServiceProvider during the test and implements the mojo interfaces
consumed by Geolocation. This mock is still controlled by
InternalsGeolocation.

Given that geolocation consists of geolocation watch registrations and
receiving bundles of doubles and is not performance critical, it seems
like a reasonable candidate for blink-in-JS. This has not been
investigated with this change, but this option should be considered.
==========

to

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
==========

[Sam McNally, 2016-01-13 23:57:32]

Patchset #14 (id:480001) has been deleted

[Sam McNally, 2016-01-19 02:26:29]

Description was changed from

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
==========

to

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Sam McNally, 2016-01-25 03:01:17]

Patchset #6 (id:320001) has been deleted

[Sam McNally, 2016-01-25 03:04:09]

Patchset #6 (id:340001) has been deleted

[Sam McNally, 2016-01-25 03:04:12]

Patchset #7 (id:360001) has been deleted

[Sam McNally, 2016-01-25 03:04:18]

Patchset #8 (id:380001) has been deleted

[Sam McNally, 2016-01-25 03:04:20]

Patchset #9 (id:400001) has been deleted

[Sam McNally, 2016-01-25 03:04:33]

Patchset #8 (id:420001) has been deleted

[Sam McNally, 2016-01-25 03:04:42]

Patchset #8 (id:440001) has been deleted

[Sam McNally, 2016-01-25 03:04:50]

Patchset #8 (id:460001) has been deleted

[Sam McNally, 2016-01-25 03:04:56]

Patchset #8 (id:500001) has been deleted

[Sam McNally, 2016-01-25 03:31:41]

Patchset #7 (id:360001) has been deleted

[Sam McNally, 2016-01-25 03:31:44]

Patchset #8 (id:520001) has been deleted

[Sam McNally, 2016-01-25 03:31:47]

Patchset #9 (id:540001) has been deleted

[Sam McNally, 2016-01-25 03:31:50]

Patchset #9 (id:560001) has been deleted

[Sam McNally, 2016-01-25 03:31:54]

Patchset #9 (id:580001) has been deleted

[Sam McNally, 2016-01-25 03:31:59]

Patchset #9 (id:600001) has been deleted

[Sam McNally, 2016-01-25 03:32:02]

Patchset #10 (id:620001) has been deleted

[Sam McNally, 2016-01-25 03:32:06]

Patchset #9 (id:640001) has been deleted

[Sam McNally, 2016-01-25 03:32:10]

Patchset #7 (id:360001) has been deleted

[Sam McNally, 2016-01-25 03:32:15]

Patchset #8 (id:560001) has been deleted

[Sam McNally, 2016-01-26 23:06:31]

Patchset #6 (id:340001) has been deleted

[Sam McNally, 2016-01-26 23:08:15]

Patchset #6 (id:660001) has been deleted

[Sam McNally, 2016-01-26 23:08:18]

Patchset #7 (id:680001) has been deleted

[Sam McNally, 2016-01-26 23:08:21]

Patchset #7 (id:700001) has been deleted

[Sam McNally, 2016-01-26 23:08:24]

Patchset #7 (id:720001) has been deleted

[Sam McNally, 2016-01-26 23:08:27]

Patchset #7 (id:740001) has been deleted

[Sam McNally, 2016-01-26 23:08:30]

Patchset #7 (id:760001) has been deleted

[Sam McNally, 2016-01-26 23:08:34]

Patchset #7 (id:780001) has been deleted

[Sam McNally, 2016-01-26 23:08:38]

Patchset #7 (id:800001) has been deleted

[Sam McNally, 2016-01-26 23:08:42]

Patchset #6 (id:660001) has been deleted

[Sam McNally, 2016-01-27 00:33:14]

Description was changed from

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
==========

[Sam McNally, 2016-01-27 00:38:33]

sammc@chromium.org changed reviewers:
+ haraken@chromium.org, yukishiino@chromium.org

[Sam McNally, 2016-01-27 00:38:34]

+yukishiino, haraken

[Sam McNally, 2016-02-17 00:15:26]

Description was changed from

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
==========

to

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Sam McNally, 2016-02-17 00:18:18]

Description was changed from

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
==========

[Sam McNally, 2016-02-21 22:55:07]

Description was changed from

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
==========

to

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Sam McNally, 2016-02-21 22:56:05]

Description was changed from

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
==========

[Sam McNally, 2016-02-24 07:11:43]

Description was changed from

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
==========

to

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Sam McNally, 2016-02-25 03:06:36]

Description was changed from

==========
Mojo in blink prototype: geolocation.

tl;dr:
- Rewrite content/renderer/geolocation_dispatcher.{cc,h} as
  WebKit/Source/platform/geolocation/GeolocationDispatcher.{cpp,h}.
- Use GeolocationDispatcher directly from Geolocation.
- Add ConnectToRemoteService to blink::Platform.
- Thread an opaque routing-ID from content to frames to use for
  connecting to per-frame browser services from the platform layer.

This changes Geolocation to directly use GeolocationDispatcher, which
has been rewritten in the blink platform layer (from content/renderer).
This replaces the chain of Geolocation -> GeolocationController ->
GeolocationClient (implemented by GeolocationClientProxy) ->
WebGeolocationClient (implemented by content::GeolocationDispatcher) and
back via content::GeolocationDispatcher -> WebGeolocationController ->
GeolocationController -> Geolocation with a simpler Geolocation ->
GeolocationDispatcher. This removes all WebGeolocation*.

To expose mojo to blink, blink::Platform provides a templated
ConnectToRemoteService method for connecting to remote services. Since
the platform layer doesn't have a concept for frames, this uses opaque
IDs to communicate this information from the WebFrameClient to the
platform layer.

Layout tests for geolocation have been changed to use the mojo interface
as the layer to mock out. GeolocationDispatcher uses an interface that
can be mocked for tests for connecting to the service implementations.
GeolocationClientMock is now GeolocationServiceMock and provides an
implementation for the connector interface that redirects them to
itself. This mock is still controlled by InternalsGeolocation.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Move GeolocationDispatcher into blink.

This also changes the layout tests for geolocation to mock out the mojo
interface directly from the JS layout test code, removing the need for
C++ code for a mock and its control.

BUG=561879
==========

[Sam McNally, 2016-02-25 04:55:25]

Description was changed from

==========
Move GeolocationDispatcher into blink.

This also changes the layout tests for geolocation to mock out the mojo
interface directly from the JS layout test code, removing the need for
C++ code for a mock and its control.

BUG=561879
==========

to

==========
Move GeolocationDispatcher into blink.

This also changes the layout tests for geolocation to mock out the mojo
interface directly from the JS layout test code, removing the need for
C++ code for a mock and its control.

BUG=561879
==========

[Sam McNally, 2016-05-03 00:06:10]

Patchset #5 (id:300001) has been deleted

[Sam McNally, 2016-05-03 00:06:40]

Patchset #5 (id:820001) has been deleted

[Sam McNally, 2016-05-03 00:07:11]

Patchset #5 (id:840001) has been deleted

[Sam McNally, 2016-05-03 00:08:18]

Patchset #5 (id:860001) has been deleted

[Sam McNally, 2016-05-03 00:08:21]

Patchset #6 (id:880001) has been deleted

[Sam McNally, 2016-05-03 00:08:23]

Patchset #7 (id:900001) has been deleted

[Sam McNally, 2016-05-03 00:08:25]

Patchset #8 (id:920001) has been deleted

[Sam McNally, 2016-05-03 00:08:27]

Patchset #9 (id:940001) has been deleted

[Sam McNally, 2016-05-03 00:08:30]

Patchset #10 (id:960001) has been deleted

[Sam McNally, 2016-05-03 00:08:32]

Patchset #11 (id:980001) has been deleted

[Sam McNally, 2016-05-03 00:08:35]

Patchset #12 (id:1000001) has been deleted

[Sam McNally, 2016-05-03 00:08:38]

Patchset #12 (id:1020001) has been deleted

[Sam McNally, 2016-05-03 00:08:41]

Patchset #12 (id:1040001) has been deleted

[Sam McNally, 2016-05-03 00:08:43]

Patchset #13 (id:1060001) has been deleted

[Sam McNally, 2016-05-03 00:08:47]

Patchset #13 (id:1080001) has been deleted

[Sam McNally, 2016-05-03 00:08:51]

Patchset #13 (id:1100001) has been deleted

[Sam McNally, 2016-05-03 00:09:45]

Patchset #5 (id:860001) has been deleted

[Sam McNally, 2016-05-03 00:10:30]

Patchset #5 (id:880001) has been deleted

[Sam McNally, 2016-05-03 00:10:32]

Patchset #6 (id:900001) has been deleted

[Sam McNally, 2016-05-03 00:10:38]

Patchset #6 (id:960001) has been deleted

[Sam McNally, 2016-05-03 00:10:38]

Patchset #7 (id:980001) has been deleted

[Sam McNally, 2016-05-03 00:10:40]

Patchset #8 (id:1000001) has been deleted

[Sam McNally, 2016-05-03 00:10:42]

Patchset #9 (id:1020001) has been deleted

[Sam McNally, 2016-05-03 00:10:46]

Patchset #9 (id:1060001) has been deleted

[Sam McNally, 2016-05-03 00:12:08]

Patchset #5 (id:900001) has been deleted

[Sam McNally, 2016-05-03 00:12:13]

Patchset #6 (id:960001) has been deleted

[Sam McNally, 2016-05-03 00:12:16]

Patchset #6 (id:1000001) has been deleted

[Sam McNally, 2016-05-03 00:12:25]

Patchset #5 (id:1020001) has been deleted

[Sam McNally, 2016-05-03 00:12:46]

Patchset #5 (id:1120001) has been deleted

[Sam McNally, 2016-05-03 00:12:49]

Patchset #5 (id:1140001) has been deleted

[Sam McNally, 2016-05-03 00:13:47]

sammc@chromium.org changed reviewers:
- haraken@chromium.org, mek@chromium.org, rockot@chromium.org,
yukishiino@chromium.org

[Sam McNally, 2016-05-03 00:42:20]

Patchset #6 (id:1180001) has been deleted

[Sam McNally, 2016-05-03 00:42:31]

Patchset #6 (id:1180001) has been deleted

[Sam McNally, 2016-05-03 01:43:19]

Patchset #7 (id:1220001) has been deleted

[Sam McNally, 2016-05-03 01:46:16]

Patchset #7 (id:1240001) has been deleted

[Sam McNally, 2016-05-03 01:47:13]

Patchset #5 (id:1160001) has been deleted

[Sam McNally, 2016-05-03 01:49:27]

Patchset #5 (id:1200001) has been deleted

[Sam McNally, 2016-05-03 08:01:04]

sammc@chromium.org changed reviewers:
+ esprehn@chromium.org, mvanouwerkerk@chromium.org

[Sam McNally, 2016-05-03 08:01:06]

I think this is ready for a full review now. Old reviewers to cc.

+mvanouwerkerk for geolocation.

+esprehn for third_party/WebKit/Source/web/, third_party/WebKit/public/,
content/renderer and blink structure.

[Michael van Ouwerkerk, 2016-05-03 08:57:15]

Thanks Sam! This seems to be nearly 5000 lines of changes, so it might take a
while to digest. I'll take a look.

[Michael van Ouwerkerk, 2016-05-04 13:52:17]

Thanks Sam, so good to see so much crufty plumbing deleted! This CL is hard to
review, I'm not confident I've covered it well enough. Is there a way to break
it into smaller pieces?

https://codereview.chromium.org/1367853002/diff/1260001/content/browser/permi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/1367853002/diff/1260001/content/browser/permi...
content/browser/permissions/permission_service_impl.cc:110:
CancelPendingOperations();
nit: This might make sense in a separate patch for mlamouri (though it seems
like a good change)

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
File
third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/geolocation-mock.js
(right):

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/geolocation-mock.js:32:
this.allowed_ = null;
Please document the type of this field, and indicate the meaning of it being
nullable. I kind of expected this to be a normal two-state boolean until I saw
line 119. Alternatively, consider renaming it to |permissionStatus_| and make it
of type nullable PermissionStatus.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/geolocation-mock.js:58:
setHighAccuracy(highAccuracy) {
Why doesn't this method need to be implemented? Are we missing test coverage?

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
File
third_party/WebKit/LayoutTests/fast/dom/Geolocation/script-tests/invalid-internals-arguments.js
(right):

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/fast/dom/Geolocation/script-tests/invalid-internals-arguments.js:1:
description("Verify that using invalid or detached documents on internal test
methods does not crash.");
Please rename this file to invalid-mock-arguments.js

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
File third_party/WebKit/LayoutTests/http/tests/resources/geolocation-mock.js
(right):

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/http/tests/resources/geolocation-mock.js:18:
class GeolocationServiceMock {
Duplicating this mock seems less than ideal. Could you make it shared?
Alternatively, consider moving everything to http/tests/geolocation. You're
touching everything already anyway :-)

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
File third_party/WebKit/LayoutTests/http/tests/resources/mojo-helpers.js
(right):

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/http/tests/resources/mojo-helpers.js:30:
define('Mojo Helpers', [
Is there no way to avoid duplicating this whole file? If not, we should at least
have a comment from one to the other to help avoid them forking.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:74: default:
Doesn't clang fail the build when a switch fails to handle all enum values? The
default clause should probably be removed.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:123: if
(m_permissionService)
Why not also stop m_geolocationService?

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:126: // The frame
may be moving to a new page and we want to get the permissions from the new
page's client.
Please update this comment, there is no client anymore.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:134: Geoposition*
Geolocation::lastPosition()
This can be in the header now, or removed entirely.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:200: else if
(isAllowed() && haveSuitableCachedPosition(notifier->options()))
Why add the isAllowed call? Is it for readability? Because
haveSuitableCachedPosition should not return true if isAllowed returns false.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:226:
DCHECK(isAllowed());
This DCHECK is not equivalent to the isDenied() branch it replaces, which sets
an error. So in prod builds we would now fail to set the error.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:283: // While we
iterate through the list, we need not worry about list being modified as the
permission
s/list/the list/

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:287: // start all
pending notification requests as permission granted.
nit: s/start/Start/

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:420:
m_permissionService.set_connection_error_handler(sameThreadBindForMojo(&Geolocation::onPermissionConnectionError,
this));
Under what circumstances does this get called?

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:484: if (!page()
|| page()->visibilityState() != PageVisibilityStateVisible || !m_updating) {
nit: use page()->isPageVisible()

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.h (right):

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.h:43: #include
"third_party/WebKit/public/platform/modules/geolocation/geolocation.mojom-blink.h"
Can we omit "third_party/WebKit" here and below?

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.h:55: : public
GarbageCollectedFinalized<Geolocation>
This class should probably use "USING_PRE_FINALIZER(Geolocation, stop);"

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.h:194:
mojom::blink::GeolocationServicePtr m_geolocationService;
When is this destroyed? The owner is GC'd at some point, but I think we need
some hooks to shut down the mojo service in a timely manner. Perhaps in
willDetachFrameHost() from LocalFrameLifecycleObserver and didCommitLoad from
PageLifeCycleObserver?

Same question for the permission service, but that does not keep an expensive
subsystem alive in the browser process, scanning wifi access points, doing
network requests, and using GPS.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.h:198: bool m_updating
= false;
Please document the distinction between m_updating and m_queryInProgress.

[Sam McNally, 2016-05-05 08:25:52]

Patchset #10 (id:1360001) has been deleted

[Sam McNally, 2016-05-05 08:26:32]

Patchset #9 (id:1340001) has been deleted

[Sam McNally, 2016-05-05 11:50:24]

On 2016/05/04 13:52:17, Michael van Ouwerkerk wrote:
> Thanks Sam, so good to see so much crufty plumbing deleted! This CL is hard to
> review, I'm not confident I've covered it well enough. Is there a way to break
> it into smaller pieces?
> 
I've split out most of the test changes into
https://codereview.chromium.org/1948033003/.

https://codereview.chromium.org/1367853002/diff/1260001/content/browser/permi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/1367853002/diff/1260001/content/browser/permi...
content/browser/permissions/permission_service_impl.cc:110:
CancelPendingOperations();
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> nit: This might make sense in a separate patch for mlamouri (though it seems
> like a good change)

Done.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
File
third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/geolocation-mock.js
(right):

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/geolocation-mock.js:32:
this.allowed_ = null;
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> Please document the type of this field, and indicate the meaning of it being
> nullable. I kind of expected this to be a normal two-state boolean until I saw
> line 119. Alternatively, consider renaming it to |permissionStatus_| and make
it
> of type nullable PermissionStatus.

Changed to a PermissionStatus, using PermissionStatus.ASK instead of null.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/geolocation-mock.js:58:
setHighAccuracy(highAccuracy) {
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> Why doesn't this method need to be implemented? Are we missing test coverage?

We're missing test coverage. I've copy-pasted the FIXME from the C++ mock now.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
File
third_party/WebKit/LayoutTests/fast/dom/Geolocation/script-tests/invalid-internals-arguments.js
(right):

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/fast/dom/Geolocation/script-tests/invalid-internals-arguments.js:1:
description("Verify that using invalid or detached documents on internal test
methods does not crash.");
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> Please rename this file to invalid-mock-arguments.js

This isn't used anymore and should have been deleted. The mock is implemented in
JS so this test isn't necessary.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
File third_party/WebKit/LayoutTests/http/tests/resources/geolocation-mock.js
(right):

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/http/tests/resources/geolocation-mock.js:18:
class GeolocationServiceMock {
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> Duplicating this mock seems less than ideal. Could you make it shared?
> Alternatively, consider moving everything to http/tests/geolocation. You're
> touching everything already anyway :-)

I found the file of magical path hacks so we can share these now.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
File third_party/WebKit/LayoutTests/http/tests/resources/mojo-helpers.js
(right):

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/http/tests/resources/mojo-helpers.js:30:
define('Mojo Helpers', [
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> Is there no way to avoid duplicating this whole file? If not, we should at
least
> have a comment from one to the other to help avoid them forking.

Removed this duplicate.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:74: default:
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> Doesn't clang fail the build when a switch fails to handle all enum values?
The
> default clause should probably be removed.

Done.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:123: if
(m_permissionService)
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> Why not also stop m_geolocationService?

stopUpdating() stops it.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:126: // The frame
may be moving to a new page and we want to get the permissions from the new
page's client.
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> Please update this comment, there is no client anymore.

Done.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:134: Geoposition*
Geolocation::lastPosition()
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> This can be in the header now, or removed entirely.

Done.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:200: else if
(isAllowed() && haveSuitableCachedPosition(notifier->options()))
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> Why add the isAllowed call? Is it for readability? Because
> haveSuitableCachedPosition should not return true if isAllowed returns false.

It was for readability. Removed it and added a DCHECK in
haveSuitableCachedPosition instead.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:226:
DCHECK(isAllowed());
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> This DCHECK is not equivalent to the isDenied() branch it replaces, which sets
> an error. So in prod builds we would now fail to set the error.

This should never be reached unless permission has already been allowed.
Previously, GeolocationController maintained a cached last position that could
be reused by a new Geolocation for a particular frame. Now, Geolocation
communicates directly with the service in the browser so nothing is shared
between different documents.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:283: // While we
iterate through the list, we need not worry about list being modified as the
permission
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> s/list/the list/

Done.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:287: // start all
pending notification requests as permission granted.
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> nit: s/start/Start/

Done.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:420:
m_permissionService.set_connection_error_handler(sameThreadBindForMojo(&Geolocation::onPermissionConnectionError,
this));
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> Under what circumstances does this get called?

Normally, this would only happen if the browser doesn't provide a permission
service.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:484: if (!page()
|| page()->visibilityState() != PageVisibilityStateVisible || !m_updating) {
On 2016/05/04 13:52:16, Michael van Ouwerkerk wrote:
> nit: use page()->isPageVisible()

Done.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.h (right):

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.h:43: #include
"third_party/WebKit/public/platform/modules/geolocation/geolocation.mojom-blink.h"
On 2016/05/04 13:52:17, Michael van Ouwerkerk wrote:
> Can we omit "third_party/WebKit" here and below?

No, only the root of the generated files tree is added to the include path.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.h:55: : public
GarbageCollectedFinalized<Geolocation>
On 2016/05/04 13:52:17, Michael van Ouwerkerk wrote:
> This class should probably use "USING_PRE_FINALIZER(Geolocation, stop);"

Done.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.h:194:
mojom::blink::GeolocationServicePtr m_geolocationService;
On 2016/05/04 13:52:17, Michael van Ouwerkerk wrote:
> When is this destroyed? The owner is GC'd at some point, but I think we need
> some hooks to shut down the mojo service in a timely manner. Perhaps in
> willDetachFrameHost() from LocalFrameLifecycleObserver and didCommitLoad from
> PageLifeCycleObserver?
> 
> Same question for the permission service, but that does not keep an expensive
> subsystem alive in the browser process, scanning wifi access points, doing
> network requests, and using GPS.

A mojo InterfacePtr isn't always connected to something else. In this case,
m_geolocationService is only connected while a geolocation request is active and
the page is visible. stop() cancels any active requests too so this would be
disconnected when DOM objects are stopped. Is this sufficient?

m_permissionService is connected when a permission request is needed and
disconnected once it's completed.

https://codereview.chromium.org/1367853002/diff/1260001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.h:198: bool m_updating
= false;
On 2016/05/04 13:52:17, Michael van Ouwerkerk wrote:
> Please document the distinction between m_updating and m_queryInProgress.

Done.

[Marijn Kruisselbrink, 2016-05-06 20:12:15]

drive-by nit

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:76:
ASSERT_NOT_REACHED();
ASSERT_NOT_REACHED() is deprecated in favor of NOTREACHED()

[Michael van Ouwerkerk, 2016-05-09 17:22:32]

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/La...
File third_party/WebKit/LayoutTests/fast/dom/Geolocation/maximum-age.html
(right):

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/fast/dom/Geolocation/maximum-age.html:6: <script
src="resources/geolocation-mock.js"></script>
geolocation-mock.js was removed from this CL

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:124: void
Geolocation::stop()
I think this should also stop observing Page:
"PageLifecycleObserver::clearContext();"

While PageLifecycleObserver only holds a WeakMember, it will happily continue
calling pageVisibilityChanged until it's really destroyed by the GC.

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:129: // The frame
may be moving to a new page and we want to get the permissions from the new
page's permission service.
I think there's more that has changed than just GeolocationClient being deleted.
The GeolocationController was a supplement of LocalFrame, which could load sites
of different origins. And in the past, the GeolocationController was a
supplement of Page, with lived even longer. The controller is gone in this CL.
The remaining supplement is NavigatorGeolocation which should not be reused
across navigations, so this entire comment no longer makes sense. Do you think
we even need to reset m_geolocationPermission anymore? Under what circumstances
would any method of this class be called after stop is called? It is garbage
collected, so it might stay alive for a while after stop is called.

We should probably call ActiveDOMObject::clearContext() here, as it is not
currently automatically cleared. See:
https://docs.google.com/document/d/1ji_010aI_OCQon08VATF_0dthK_GCHfPZ8CHblkao...

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:462: // This may
be called multiple times, though stopUpdating() is called only
There are many calls to stopUpdating(). What did you mean to express here?

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:482: if (!page()
|| !page()->isPageVisible() || !m_updating) {
This should also check that getExecutionContext() does not return null.

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.h (right):

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.h:57: , public
ActiveDOMObject
Sorry to comment on existing code you didn't change, but for this CL it seems
helpful to clarify the lifecycles. As only stop() is used, I think
ActiveDOMObject can be replaced by ContextLifecycleObserver and stop become
contextDestroyed().

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.h:67: void stop()
override;
Let's add some documentation for stop(): "Inherited from ActiveDOMObject AND
used as pre-finalizer."

[dcheng, 2016-05-09 18:22:28]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-05-09 18:22:29]

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:129: // The frame
may be moving to a new page and we want to get the permissions from the new
page's permission service.
On 2016/05/09 at 17:22:31, Michael van Ouwerkerk wrote:
> I think there's more that has changed than just GeolocationClient being
deleted. The GeolocationController was a supplement of LocalFrame, which could
load sites of different origins. And in the past, the GeolocationController was
a supplement of Page, with lived even longer. The controller is gone in this CL.
The remaining supplement is NavigatorGeolocation which should not be reused
across navigations, so this entire comment no longer makes sense. Do you think
we even need to reset m_geolocationPermission anymore? Under what circumstances
would any method of this class be called after stop is called? It is garbage
collected, so it might stay alive for a while after stop is called.
> 
> We should probably call ActiveDOMObject::clearContext() here, as it is not
currently automatically cleared. See:
https://docs.google.com/document/d/1ji_010aI_OCQon08VATF_0dthK_GCHfPZ8CHblkao...

I don't understand this comment. What does it mean 'frame might be moving to a
new Page'?

[Sam McNally, 2016-05-10 10:06:09]

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/La...
File third_party/WebKit/LayoutTests/fast/dom/Geolocation/maximum-age.html
(right):

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/fast/dom/Geolocation/maximum-age.html:6: <script
src="resources/geolocation-mock.js"></script>
On 2016/05/09 17:22:31, Michael van Ouwerkerk wrote:
> geolocation-mock.js was removed from this CL

This CL depends on the CL that updates the other tests.

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:76:
ASSERT_NOT_REACHED();
On 2016/05/06 20:12:15, Marijn Kruisselbrink wrote:
> ASSERT_NOT_REACHED() is deprecated in favor of NOTREACHED()

Done.

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:124: void
Geolocation::stop()
On 2016/05/09 17:22:31, Michael van Ouwerkerk wrote:
> I think this should also stop observing Page:
> "PageLifecycleObserver::clearContext();"
> 
> While PageLifecycleObserver only holds a WeakMember, it will happily continue
> calling pageVisibilityChanged until it's really destroyed by the GC.

Done.

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:129: // The frame
may be moving to a new page and we want to get the permissions from the new
page's permission service.
On 2016/05/09 17:22:31, Michael van Ouwerkerk wrote:
> I think there's more that has changed than just GeolocationClient being
deleted.
> The GeolocationController was a supplement of LocalFrame, which could load
sites
> of different origins. And in the past, the GeolocationController was a
> supplement of Page, with lived even longer. The controller is gone in this CL.
> The remaining supplement is NavigatorGeolocation which should not be reused
> across navigations, so this entire comment no longer makes sense. Do you think
> we even need to reset m_geolocationPermission anymore? Under what
circumstances
> would any method of this class be called after stop is called? It is garbage
> collected, so it might stay alive for a while after stop is called.

I didn't think that Geolocation objects would be reused, but I don't have a
great understanding of lifetimes in blink. It does appear to be necessary to
change the permission from requested as there is an assert in the destructor.
I've changed it to denied.
> 
> We should probably call ActiveDOMObject::clearContext() here, as it is not
> currently automatically cleared. See:
>
https://docs.google.com/document/d/1ji_010aI_OCQon08VATF_0dthK_GCHfPZ8CHblkao...

Done.

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:462: // This may
be called multiple times, though stopUpdating() is called only
On 2016/05/09 17:22:31, Michael van Ouwerkerk wrote:
> There are many calls to stopUpdating(). What did you mean to express here?

It was copy-pasted from GeolocationController and changed slightly. I don't
think it's useful anymore.

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:482: if (!page()
|| !page()->isPageVisible() || !m_updating) {
On 2016/05/09 17:22:31, Michael van Ouwerkerk wrote:
> This should also check that getExecutionContext() does not return null.

Done.

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.h (right):

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.h:57: , public
ActiveDOMObject
On 2016/05/09 17:22:31, Michael van Ouwerkerk wrote:
> Sorry to comment on existing code you didn't change, but for this CL it seems
> helpful to clarify the lifecycles. As only stop() is used, I think
> ActiveDOMObject can be replaced by ContextLifecycleObserver and stop become
> contextDestroyed().

Done.

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.h:67: void stop()
override;
On 2016/05/09 17:22:31, Michael van Ouwerkerk wrote:
> Let's add some documentation for stop(): "Inherited from ActiveDOMObject AND
> used as pre-finalizer."

Done.

[dcheng, 2016-05-10 23:55:29]

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:124: void
Geolocation::stop()
On 2016/05/10 at 10:06:08, Sam McNally wrote:
> On 2016/05/09 17:22:31, Michael van Ouwerkerk wrote:
> > I think this should also stop observing Page:
> > "PageLifecycleObserver::clearContext();"
> > 
> > While PageLifecycleObserver only holds a WeakMember, it will happily
continue
> > calling pageVisibilityChanged until it's really destroyed by the GC.
> 
> Done.

I don't think a swept Page should be doing any visibility changes, that would be
pretty broken.

[dcheng, 2016-05-10 23:55:55]

On 2016/05/10 at 23:55:29, dcheng wrote:
>
https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
> File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):
> 
>
https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
> third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:124: void
Geolocation::stop()
> On 2016/05/10 at 10:06:08, Sam McNally wrote:
> > On 2016/05/09 17:22:31, Michael van Ouwerkerk wrote:
> > > I think this should also stop observing Page:
> > > "PageLifecycleObserver::clearContext();"
> > > 
> > > While PageLifecycleObserver only holds a WeakMember, it will happily
continue
> > > calling pageVisibilityChanged until it's really destroyed by the GC.
> > 
> > Done.
> 
> I don't think a swept Page should be doing any visibility changes, that would
be pretty broken.

(Well, not swept since it's not yet destroyed. Marked for destruction?)

[haraken, 2016-05-11 00:05:04]

On 2016/05/10 23:55:55, dcheng wrote:
> On 2016/05/10 at 23:55:29, dcheng wrote:
> >
>
https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
> > File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
> > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:124: void
> Geolocation::stop()
> > On 2016/05/10 at 10:06:08, Sam McNally wrote:
> > > On 2016/05/09 17:22:31, Michael van Ouwerkerk wrote:
> > > > I think this should also stop observing Page:
> > > > "PageLifecycleObserver::clearContext();"
> > > > 
> > > > While PageLifecycleObserver only holds a WeakMember, it will happily
> continue
> > > > calling pageVisibilityChanged until it's really destroyed by the GC.
> > > 
> > > Done.
> > 
> > I don't think a swept Page should be doing any visibility changes, that
would
> be pretty broken.
> 
> (Well, not swept since it's not yet destroyed. Marked for destruction?)

pageVisibilityChanged should not be called.

Things happen in the following order:

1) Page becomes unreachable.
2) GC is triggered. The Page is not marked.
3) Lazy sweeping starts. Blink may run during the lazy sweeping.
4) At some point, the Page gets destroyed.

Given that the Page lost all references to it at 1), pageVisibilityChanged
should not be called after 1).

A problem happens if someone (in common cases, someone in content/) holds a raw
pointer to a Blink object and keeps invoking some callback using the raw
pointer. In this case, we need to clear the raw pointer in 2) using a
pre-finalizer. Otherwise, the callback may be called during 3) and cause a
use-after-free.

[Michael van Ouwerkerk, 2016-05-11 12:38:30]

lgtm with nits

Thanks Sam!

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/1367853002/diff/1380001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:124: void
Geolocation::stop()
On 2016/05/10 23:55:29, dcheng wrote:
> On 2016/05/10 at 10:06:08, Sam McNally wrote:
> > On 2016/05/09 17:22:31, Michael van Ouwerkerk wrote:
> > > I think this should also stop observing Page:
> > > "PageLifecycleObserver::clearContext();"
> > > 
> > > While PageLifecycleObserver only holds a WeakMember, it will happily
> continue
> > > calling pageVisibilityChanged until it's really destroyed by the GC.
> > 
> > Done.
> 
> I don't think a swept Page should be doing any visibility changes, that would
be
> pretty broken.

The issue seems to be that while ContextLifecycleObserver might notify
contextDestroyed, Page outlives it. So if Geolocation is still observing Page,
it may continue to receive calls to pageVisibilityChanged until the GC gets
around to actually destroying Geolocation.

https://codereview.chromium.org/1367853002/diff/1460001/third_party/WebKit/La...
File
third_party/WebKit/LayoutTests/fast/dom/Geolocation/script-tests/error-service-connection-error.js
(left):

https://codereview.chromium.org/1367853002/diff/1460001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/fast/dom/Geolocation/script-tests/error-service-connection-error.js:24:
shouldBe('error.TIMEOUT', '3');
Why drop the coverage for these constants?

https://codereview.chromium.org/1367853002/diff/1460001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.h (right):

https://codereview.chromium.org/1367853002/diff/1460001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.h:67: // Inherited
from ContextLifecycleObserver AND used as a pre-finalizer.
Thinking about it some more, I think we can rely on contextDestroyed from
ContextLifecycleObserver to do all the cleanup we need (and it does that now).
So, we probably don't need to call this as a pre-finalizer. However, this is
also an override for contextDestroyed from PageLifecycleObserver, so the comment
would become: "Inherited from ContextLifecycleObserver AND
PageLifecycleObserver."

[Sam McNally, 2016-05-12 07:17:20]

https://codereview.chromium.org/1367853002/diff/1460001/third_party/WebKit/La...
File
third_party/WebKit/LayoutTests/fast/dom/Geolocation/script-tests/error-service-connection-error.js
(left):

https://codereview.chromium.org/1367853002/diff/1460001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/fast/dom/Geolocation/script-tests/error-service-connection-error.js:24:
shouldBe('error.TIMEOUT', '3');
On 2016/05/11 12:38:30, Michael van Ouwerkerk wrote:
> Why drop the coverage for these constants?

git cl upload is detecting this as a copy of another test. They're still covered
by the original version.

https://codereview.chromium.org/1367853002/diff/1460001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.h (right):

https://codereview.chromium.org/1367853002/diff/1460001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.h:67: // Inherited
from ContextLifecycleObserver AND used as a pre-finalizer.
On 2016/05/11 12:38:30, Michael van Ouwerkerk wrote:
> Thinking about it some more, I think we can rely on contextDestroyed from
> ContextLifecycleObserver to do all the cleanup we need (and it does that now).
> So, we probably don't need to call this as a pre-finalizer. However, this is
> also an override for contextDestroyed from PageLifecycleObserver, so the
comment
> would become: "Inherited from ContextLifecycleObserver AND
> PageLifecycleObserver."

Done.

[Sam McNally, 2016-05-13 02:49:50]

esprehn: ping

[esprehn, 2016-05-13 02:59:08]

lgtm

https://codereview.chromium.org/1367853002/diff/1480001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.h (right):

https://codereview.chromium.org/1367853002/diff/1480001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.h:195:
mojom::blink::PermissionServicePtr m_permissionService;
you don't need to change it in this patch, but I think we're moving to
blink::mojom::Foo ?

[dcheng, 2016-05-13 03:42:55]

https://codereview.chromium.org/1367853002/diff/1480001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/1367853002/diff/1480001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:52:
position.altitude > -10000.,
These checks are kind of opaque. Why -10000, etc?

https://codereview.chromium.org/1367853002/diff/1480001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:125:
m_permissionService.reset();
I think it's OK to just unconditionally call m_permissionService.reset() here
(this is currently called twice, but won't be soon... or that's the hope).

https://codereview.chromium.org/1367853002/diff/1480001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:133:
PageLifecycleObserver::clearContext();
I guess that's an argument switching visibility changes back to document level.
Thanks fo r explaining why this is necessary.

[dcheng, 2016-05-13 03:43:32]

LGTM

https://codereview.chromium.org/1367853002/diff/1480001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/1367853002/diff/1480001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:52:
position.altitude > -10000.,
On 2016/05/13 at 03:42:55, dcheng wrote:
> These checks are kind of opaque. Why -10000, etc?

Oh I see, this is from the old Geolocation code. Maybe copy some of the comments
over though.

[Sam McNally, 2016-05-13 05:00:30]

https://codereview.chromium.org/1367853002/diff/1480001/third_party/WebKit/So...
File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/1367853002/diff/1480001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:52:
position.altitude > -10000.,
On 2016/05/13 03:43:32, dcheng wrote:
> On 2016/05/13 at 03:42:55, dcheng wrote:
> > These checks are kind of opaque. Why -10000, etc?
> 
> Oh I see, this is from the old Geolocation code. Maybe copy some of the
comments
> over though.

Done.

https://codereview.chromium.org/1367853002/diff/1480001/third_party/WebKit/So...
third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:125:
m_permissionService.reset();
On 2016/05/13 03:42:55, dcheng wrote:
> I think it's OK to just unconditionally call m_permissionService.reset() here
> (this is currently called twice, but won't be soon... or that's the hope).

Done.

[Sam McNally, 2016-05-13 05:13:54]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-05-13 05:13:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvanouwerkerk@chromium.org, esprehn@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1367853002/#ps1500001
(title: " ")

[commit-bot: I haz the power, 2016-05-13 05:14:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1367853002/1500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1367853002/1500001

[commit-bot: I haz the power, 2016-05-13 06:01:11]

Description was changed from

==========
Move GeolocationDispatcher into blink.

This also changes the layout tests for geolocation to mock out the mojo
interface directly from the JS layout test code, removing the need for
C++ code for a mock and its control.

BUG=561879
==========

to

==========
Move GeolocationDispatcher into blink.

This also changes the layout tests for geolocation to mock out the mojo
interface directly from the JS layout test code, removing the need for
C++ code for a mock and its control.

BUG=561879
==========

[commit-bot: I haz the power, 2016-05-13 06:01:14]

Committed patchset #15 (id:1500001)

[commit-bot: I haz the power, 2016-05-13 06:02:29]

Description was changed from

==========
Move GeolocationDispatcher into blink.

This also changes the layout tests for geolocation to mock out the mojo
interface directly from the JS layout test code, removing the need for
C++ code for a mock and its control.

BUG=561879
==========

to

==========
Move GeolocationDispatcher into blink.

This also changes the layout tests for geolocation to mock out the mojo
interface directly from the JS layout test code, removing the need for
C++ code for a mock and its control.

BUG=561879

Committed: https://crrev.com/f48785c4922df0b6aa3fe1bae756f8c54eb6c899
Cr-Commit-Position: refs/heads/master@{#393452}
==========

[commit-bot: I haz the power, 2016-05-13 06:02:31]

Patchset 15 (id:??) landed as
https://crrev.com/f48785c4922df0b6aa3fe1bae756f8c54eb6c899
Cr-Commit-Position: refs/heads/master@{#393452}