Partially convert geolocation IPC to Mojo.

Partially convert geolocation IPC to Mojo.

This CL converts the non-permissions-related geolocation IPC to Mojo. The Mojo
GeolocationService interface allows clients to observe location updates. In
//content, the service and its client connect on a per-frame basis, eliminating
the need for the tracking of multiple frames that GeolocationDispatcherHost had
previously been doing. To handle the fact that geolocation updates can be
paused and resumed at per-WebContents granularity, we introduce a
GeolocationServiceContext class, which is used to scope the granularity of
pauses and resumes.

Currently, GeolocationDispatcherHost still handles permissions-related
geolocation IPC. This IPC will be moved to Mojo once there is resolution on
what the right model for handling permissions there is.

BUG=420497
TEST=Go to maps.google.com, allow location tracking, and check that your
location is correctly pinpointed on the map.

Committed: https://crrev.com/28e88081438dc08b06d5f05cfdd980c407db1638
Cr-Commit-Position: refs/heads/master@{#301604}

Committed: https://crrev.com/c57b93f1e66e06bbddb07348a2816aa4a7df1051
Cr-Commit-Position: refs/heads/master@{#301824}

[blundell, 2014-10-07 11:16:04]

blundell@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org, qsr@chromium.org

[blundell, 2014-10-07 11:18:17]

blundell@chromium.org changed reviewers:
+ timvolodine@chromium.org

[blundell, 2014-10-07 11:18:18]

+timvolodine@

[blundell, 2014-10-07 11:21:05]

https://codereview.chromium.org/628773003/diff/20001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.cc (left):

https://codereview.chromium.org/628773003/diff/20001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:226: high_accuracy =
true;
Note: I've introduced a behavioral change. Before, if any frame wanted high
accuracy, they all got high accuracy. Now, only frames that request high
accuracy get high accuracy. Is this change OK?

[qsr, 2014-10-07 13:03:59]

https://codereview.chromium.org/628773003/diff/20001/content/browser/geolocat...
File content/browser/geolocation/geolocation_service_impl_context.cc (right):

https://codereview.chromium.org/628773003/diff/20001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl_context.cc:30: for (auto
service : attached_services_) {
You should be using auto* to make it clear that this isn't a copy.

https://codereview.chromium.org/628773003/diff/20001/content/browser/geolocat...
File content/browser/geolocation/geolocation_service_impl_context.h (right):

https://codereview.chromium.org/628773003/diff/20001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl_context.h:32: bool paused()
{ return paused_; }
Could you tell me who is using this?

[blundell, 2014-10-07 14:35:24]

Thanks.

https://codereview.chromium.org/628773003/diff/20001/content/browser/geolocat...
File content/browser/geolocation/geolocation_service_impl_context.cc (right):

https://codereview.chromium.org/628773003/diff/20001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl_context.cc:30: for (auto
service : attached_services_) {
On 2014/10/07 13:03:58, qsr wrote:
> You should be using auto* to make it clear that this isn't a copy.

Done.

https://codereview.chromium.org/628773003/diff/20001/content/browser/geolocat...
File content/browser/geolocation/geolocation_service_impl_context.h (right):

https://codereview.chromium.org/628773003/diff/20001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl_context.h:32: bool paused()
{ return paused_; }
On 2014/10/07 13:03:59, qsr wrote:
> Could you tell me who is using this?

GeolocationServiceImpl::OnLocationUpdate(). If we move the the Android impl to
Java, the whole infrastructure around pausing/resuming would move to Java as
well - it's used only on Android and kicked off by a call into AwContents from
Java.

[blundell, 2014-10-07 14:35:54]

blundell@chromium.org changed reviewers:
+ brettw@chromium.org

[blundell, 2014-10-07 14:35:54]

+brettw

Brett, could you look at the changes to the GN files? Thanks!

[brettw, 2014-10-07 16:32:14]

.gn files lgtm. In the future, you don't need to add me on these unless there's
something in particular you have a question about or want checked.

[Michael van Ouwerkerk, 2014-10-08 12:58:57]

Could you run the try bots? There are many Geolocation tests.

[blundell, 2014-10-08 13:33:31]

On 2014/10/08 12:58:57, Michael van Ouwerkerk wrote:
> Could you run the try bots? There are many Geolocation tests.

Yep! Will ping again once I see that they're green.

[Michael van Ouwerkerk, 2014-10-08 14:13:08]

Sorry for the flurry of questions. I'm new to mojo.

https://codereview.chromium.org/628773003/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/628773003/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_delegate.h:152: virtual
GeolocationServiceImplContext* GetGeolocationContext();
Please disambiguate in method name and documentation from
GeolocationPermissionContext

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.h (left):

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.h:35: virtual void
RenderFrameDeleted(RenderFrameHost* render_frame_host) OVERRIDE;
This seems to conflict with this pending change:
https://codereview.chromium.org/625963003

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
File content/browser/geolocation/geolocation_service_impl.cc (right):

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl.cc:100: // TODO(blundell):
What does this comment even mean?
I was thinking the same. The purpose of this histogram is to check what %
geolocation requests is for high accuracy. I don't think an enable / disable
cycle affects that meaningfully, but it could be omitted as well, if we knew it
was due to e.g. page visibility going through a visible / hidden / visible
cycle.

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl.cc:104:
"Geolocation.GeolocationDispatcherHostImpl.EnableHighAccuracy",
I've chosen a terrible name for this histogram. Still I'd rather not change it,
to avoid discontinuity in the data.

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
File content/browser/geolocation/geolocation_service_impl.h (right):

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl.h:18: class
GeolocationServiceImpl : public mojo::InterfaceImpl<GeolocationService> {
Who owns this service? How many instances are there?

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl.h:24: const base::Closure&
on_location_update_callback,
Nit: drop the on_ prefix. Or even: location_callback

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl.h:43:
GeolocationServiceImplContext* context_;
I'm having a slow day. Service outlives context or vice versa?

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl.h:50: };
Does this need DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
File content/browser/geolocation/geolocation_service_impl_context.cc (right):

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl_context.cc:32: /*
?

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
File content/browser/geolocation/geolocation_service_impl_context.h (right):

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl_context.h:16: class
GeolocationServiceImplContext {
Would it be ok to just call this GeolocationServiceContext?

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl_context.h:35:
std::list<GeolocationServiceImpl*> attached_services_;
Nit: just 'services_' seems sufficient.

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl_context.h:35:
std::list<GeolocationServiceImpl*> attached_services_;
As these are raw pointers, can you document who owns the services?

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl_context.h:37: };
Does this need DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/628773003/diff/80001/content/renderer/geoloca...
File content/renderer/geolocation_dispatcher.cc (right):

https://codereview.chromium.org/628773003/diff/80001/content/renderer/geoloca...
content/renderer/geolocation_dispatcher.cc:111: void
GeolocationDispatcher::OnLocationUpdate(MojoGeopositionPtr geoposition) {
Can we not accept references from mojo services? Who deletes geoposition?

[Michael van Ouwerkerk, 2014-10-08 14:43:02]

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.h (right):

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.h:26: class
GeolocationDispatcherHost : public WebContentsObserver {
Heads up: changing this class conflicts with pending change
https://codereview.chromium.org/603323004/

[blundell, 2014-10-09 08:09:19]

Patchset #6 (id:100001) has been deleted

[blundell, 2014-10-09 08:32:52]

Hi Michael,

Thanks for the review!

https://codereview.chromium.org/628773003/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/628773003/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_delegate.h:152: virtual
GeolocationServiceImplContext* GetGeolocationContext();
On 2014/10/08 14:13:07, Michael van Ouwerkerk wrote:
> Please disambiguate in method name and documentation from
> GeolocationPermissionContext

Done.

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
File content/browser/geolocation/geolocation_service_impl.cc (right):

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl.cc:100: // TODO(blundell):
What does this comment even mean?
On 2014/10/08 14:13:07, Michael van Ouwerkerk wrote:
> I was thinking the same. The purpose of this histogram is to check what %
> geolocation requests is for high accuracy. I don't think an enable / disable
> cycle affects that meaningfully, but it could be omitted as well, if we knew
it
> was due to e.g. page visibility going through a visible / hidden / visible
> cycle.

I'm not sure what you're suggesting I do with the comment :). I'd be inclined to
just kill it.

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl.cc:104:
"Geolocation.GeolocationDispatcherHostImpl.EnableHighAccuracy",
On 2014/10/08 14:13:08, Michael van Ouwerkerk wrote:
> I've chosen a terrible name for this histogram. Still I'd rather not change
it,
> to avoid discontinuity in the data.

Acknowledged.

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
File content/browser/geolocation/geolocation_service_impl.h (right):

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl.h:18: class
GeolocationServiceImpl : public mojo::InterfaceImpl<GeolocationService> {
On 2014/10/08 14:13:08, Michael van Ouwerkerk wrote:
> Who owns this service? How many instances are there?

The service was bound to the lifetime of the pipe to which it was connected;
there is one instance of the service per connection requested by the renderer
(i.e., per call to ConnectToRemoteService() that is made from the renderer
side).

I've now changed the service ownership model so that it's owned by
GeolocationServiceImplContext, as I think that's easier to understand. The
difference to enable that is using WeakBindToRequest() as opposed to
BindToRequest().

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl.h:24: const base::Closure&
on_location_update_callback,
On 2014/10/08 14:13:08, Michael van Ouwerkerk wrote:
> Nit: drop the on_ prefix. Or even: location_callback

Changed to update_callback.

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl.h:43:
GeolocationServiceImplContext* context_;
On 2014/10/08 14:13:08, Michael van Ouwerkerk wrote:
> I'm having a slow day. Service outlives context or vice versa?

Before, either could outlive the other, which as you point out, is a confusing
model. I've changed the model so that the context owns the services.

https://codereview.chromium.org/628773003/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl.h:50: };
On 2014/10/08 14:13:08, Michael van Ouwerkerk wrote:
> Does this need DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.h (right):

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.h:26: class
GeolocationDispatcherHost : public WebContentsObserver {
On 2014/10/08 14:43:02, Michael van Ouwerkerk wrote:
> Heads up: changing this class conflicts with pending change
> https://codereview.chromium.org/603323004/

Acknowledged.

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
File content/browser/geolocation/geolocation_service_impl_context.cc (right):

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl_context.cc:32: /*
On 2014/10/08 14:13:08, Michael van Ouwerkerk wrote:
> ?

Done.

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
File content/browser/geolocation/geolocation_service_impl_context.h (right):

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl_context.h:16: class
GeolocationServiceImplContext {
On 2014/10/08 14:13:08, Michael van Ouwerkerk wrote:
> Would it be ok to just call this GeolocationServiceContext?

Done.

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl_context.h:35:
std::list<GeolocationServiceImpl*> attached_services_;
On 2014/10/08 14:13:08, Michael van Ouwerkerk wrote:
> Nit: just 'services_' seems sufficient.

Done.

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl_context.h:35:
std::list<GeolocationServiceImpl*> attached_services_;
On 2014/10/08 14:13:08, Michael van Ouwerkerk wrote:
> As these are raw pointers, can you document who owns the services?

I changed the ownership model so that this class owns the services. I think it's
cleaner and easier to understand now.

https://codereview.chromium.org/628773003/diff/80001/content/browser/geolocat...
content/browser/geolocation/geolocation_service_impl_context.h:37: };
On 2014/10/08 14:13:08, Michael van Ouwerkerk wrote:
> Does this need DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/628773003/diff/80001/content/renderer/geoloca...
File content/renderer/geolocation_dispatcher.cc (right):

https://codereview.chromium.org/628773003/diff/80001/content/renderer/geoloca...
content/renderer/geolocation_dispatcher.cc:111: void
GeolocationDispatcher::OnLocationUpdate(MojoGeopositionPtr geoposition) {
On 2014/10/08 14:13:08, Michael van Ouwerkerk wrote:
> Can we not accept references from mojo services?

Not as far as I'm aware.

> Who deletes geoposition?

It's deleted when it goes out of scope here.

[blundell, 2014-10-09 08:37:24]

Michael, note that I've introduced a behavioral change. Before, if any frame
wanted high accuracy, they all got high accuracy. Now, only frames that request
high accuracy get high accuracy. Is this change OK?

[qsr, 2014-10-09 08:58:33]

https://codereview.chromium.org/628773003/diff/120001/content/browser/geoloca...
File content/browser/geolocation/geolocation_service_impl.h (right):

https://codereview.chromium.org/628773003/diff/120001/content/browser/geoloca...
content/browser/geolocation/geolocation_service_impl.h:32: virtual void
StartUpdating(bool high_accuracy) OVERRIDE;
If this is part of the interface of the object, is there any reason to make this
private?

[Michael van Ouwerkerk, 2014-10-09 13:11:23]

https://codereview.chromium.org/628773003/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/628773003/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:208:
base::Unretained(delegate_->GetGeolocationServiceContext()),
How does this work when GetGeolocationServiceContext() returns NULL?

https://codereview.chromium.org/628773003/diff/120001/content/browser/geoloca...
File content/browser/geolocation/geolocation_service_impl.cc (right):

https://codereview.chromium.org/628773003/diff/120001/content/browser/geoloca...
content/browser/geolocation/geolocation_service_impl.cc:88: // TODO(blundell):
What does this comment even mean?
Yeah let's delete this comment.

https://codereview.chromium.org/628773003/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/628773003/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:350: this,
Are these random formatting changes due to running 'git cl format'? If so, fine
:-)

https://codereview.chromium.org/628773003/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/628773003/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1224:
scoped_ptr<GeolocationServiceContext> geolocation_context_;
Nit: geolocation_service_context_

https://codereview.chromium.org/628773003/diff/120001/content/renderer/geoloc...
File content/renderer/geolocation_dispatcher.h (right):

https://codereview.chromium.org/628773003/diff/120001/content/renderer/geoloc...
content/renderer/geolocation_dispatcher.h:51: virtual void
OnLocationUpdate(MojoGeopositionPtr geoposition) OVERRIDE;
What are the characteristics of these mojo pointers?

[Michael van Ouwerkerk, 2014-10-09 13:14:26]

On 2014/10/09 08:37:24, blundell wrote:
> Michael, note that I've introduced a behavioral change. Before, if any frame
> wanted high accuracy, they all got high accuracy. Now, only frames that
request
> high accuracy get high accuracy. Is this change OK?

I think the behavior has not really changed. GeolocationProviderImpl is a
singleton and if there are any callbacks for high accuracy positions it uses
high accuracy for all. See GeolocationProviderImpl::OnClientsChanged

[blundell, 2014-10-16 06:22:34]

On 2014/10/09 13:14:26, Michael van Ouwerkerk wrote:
> On 2014/10/09 08:37:24, blundell wrote:
> > Michael, note that I've introduced a behavioral change. Before, if any frame
> > wanted high accuracy, they all got high accuracy. Now, only frames that
> request
> > high accuracy get high accuracy. Is this change OK?
> 
> I think the behavior has not really changed. GeolocationProviderImpl is a
> singleton and if there are any callbacks for high accuracy positions it uses
> high accuracy for all. See GeolocationProviderImpl::OnClientsChanged

To provide an update here, the reason for the radio silence is that this CL has
exposed what looks like a pre-existing source of flake in the Mojo setup in
//content, which I'm trying to track down.

[blundell, 2014-10-20 11:57:23]

Patchset #7 (id:140001) has been deleted

[blundell, 2014-10-21 10:05:46]

Patchset #8 (id:180001) has been deleted

[blundell, 2014-10-21 12:27:50]

+vkoskokov, dgozman FYI, as this CL now ports
https://codereview.chromium.org/603323004/ to the Mojo impl

Thanks for the reviews! The flaky browser tests are now solved by
https://codereview.chromium.org/667683002/, and this is ready for another round
of review. The big change is that this CL now ports the geolocation override
functionality for devtools to the Mojo impl.

The meaningful diff is between the last PS and the PS before that, which is
mostly a rebase.

https://codereview.chromium.org/628773003/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/628773003/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:208:
base::Unretained(delegate_->GetGeolocationServiceContext()),
On 2014/10/09 13:11:23, Michael van Ouwerkerk wrote:
> How does this work when GetGeolocationServiceContext() returns NULL?

Done.

https://codereview.chromium.org/628773003/diff/120001/content/browser/geoloca...
File content/browser/geolocation/geolocation_service_impl.cc (right):

https://codereview.chromium.org/628773003/diff/120001/content/browser/geoloca...
content/browser/geolocation/geolocation_service_impl.cc:88: // TODO(blundell):
What does this comment even mean?
On 2014/10/09 13:11:23, Michael van Ouwerkerk wrote:
> Yeah let's delete this comment.

Done.

https://codereview.chromium.org/628773003/diff/120001/content/browser/geoloca...
File content/browser/geolocation/geolocation_service_impl.h (right):

https://codereview.chromium.org/628773003/diff/120001/content/browser/geoloca...
content/browser/geolocation/geolocation_service_impl.h:32: virtual void
StartUpdating(bool high_accuracy) OVERRIDE;
On 2014/10/09 08:58:33, qsr (OoO until Oct 27) wrote:
> If this is part of the interface of the object, is there any reason to make
this
> private?

I made it private because it should only be called from generated code that
knows about an InterfaceImpl<GeolocationService>. I don't feel strongly about it
though.

https://codereview.chromium.org/628773003/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/628773003/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:350: this,
On 2014/10/09 13:11:23, Michael van Ouwerkerk wrote:
> Are these random formatting changes due to running 'git cl format'? If so,
fine
> :-)

Yep.

https://codereview.chromium.org/628773003/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/628773003/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1224:
scoped_ptr<GeolocationServiceContext> geolocation_context_;
On 2014/10/09 13:11:23, Michael van Ouwerkerk wrote:
> Nit: geolocation_service_context_

Done.

https://codereview.chromium.org/628773003/diff/120001/content/renderer/geoloc...
File content/renderer/geolocation_dispatcher.h (right):

https://codereview.chromium.org/628773003/diff/120001/content/renderer/geoloc...
content/renderer/geolocation_dispatcher.h:51: virtual void
OnLocationUpdate(MojoGeopositionPtr geoposition) OVERRIDE;
On 2014/10/09 13:11:23, Michael van Ouwerkerk wrote:
> What are the characteristics of these mojo pointers?

Could you be more precise as to the question?

[Michael van Ouwerkerk, 2014-10-21 12:47:21]

https://codereview.chromium.org/628773003/diff/120001/content/renderer/geoloc...
File content/renderer/geolocation_dispatcher.h (right):

https://codereview.chromium.org/628773003/diff/120001/content/renderer/geoloc...
content/renderer/geolocation_dispatcher.h:51: virtual void
OnLocationUpdate(MojoGeopositionPtr geoposition) OVERRIDE;
On 2014/10/21 12:27:50, blundell wrote:
> On 2014/10/09 13:11:23, Michael van Ouwerkerk wrote:
> > What are the characteristics of these mojo pointers?
> 
> Could you be more precise as to the question?

Are they similar to one of the other smart pointers e.g. scoped_ptr or ref_ptr?
Where is this behavior documented?

[blundell, 2014-10-21 13:04:13]

On 2014/10/21 12:47:21, Michael van Ouwerkerk wrote:
>
https://codereview.chromium.org/628773003/diff/120001/content/renderer/geoloc...
> File content/renderer/geolocation_dispatcher.h (right):
> 
>
https://codereview.chromium.org/628773003/diff/120001/content/renderer/geoloc...
> content/renderer/geolocation_dispatcher.h:51: virtual void
> OnLocationUpdate(MojoGeopositionPtr geoposition) OVERRIDE;
> On 2014/10/21 12:27:50, blundell wrote:
> > On 2014/10/09 13:11:23, Michael van Ouwerkerk wrote:
> > > What are the characteristics of these mojo pointers?
> > 
> > Could you be more precise as to the question?
> 
> Are they similar to one of the other smart pointers e.g. scoped_ptr or
ref_ptr?
> Where is this behavior documented?

Ah! It's similar to scoped_ptr. MojoGeopositionPtr is a typedef for
mojo::StructPtr<MojoGeoPosition>. A mojo::StructPtr<T> is like a scoped_ptr<T>:

https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...

[Michael van Ouwerkerk, 2014-10-21 13:45:41]

Nice, thanks! lgtm

[blundell, 2014-10-21 13:56:04]

blundell@chromium.org changed reviewers:
+ creis@chromium.org, tsepez@chromium.org

[blundell, 2014-10-21 13:56:05]

Thanks!

OWNERS:
+creis@ for //content outside of //content/browser/geolocation
+tzepez@ for //content/common/geolocation_messages.h

[blundell, 2014-10-21 13:57:42]

tsepez@: please review the new .mojom files as well, as well as any other parts
of the CL that you need to look at from a security standpoint. Thanks!

[Tom Sepez, 2014-10-21 18:10:06]

I didn't follow the flow to see how the user provides confirmation that she
wants to allow this information to flow to a particular page.  Presumably that
hasn't changed much, but you could save me some time if you described how that
works in the new world.

https://codereview.chromium.org/628773003/diff/200001/content/common/geolocat...
File content/common/geolocation_service.mojom (right):

https://codereview.chromium.org/628773003/diff/200001/content/common/geolocat...
content/common/geolocation_service.mojom:17: };
Nit: are you sure you don't want to spec a StopUpdating() method while we're at
it even if its not being used at the moment?  Closing the pipe to accomplish
this feels kinda heavy handed to me.

[blundell, 2014-10-21 20:17:42]

Thanks!

This CL changes nothing about the way geolocation permissions work. As before,
it is Blink's responsibility to ensure that permissions have been obtained for a
given origin before asking GeolocationDispatcher to startUpdating() for that
origin. Neither GeolocationDispatcher nor GeolocationDispatcherHost play a role
in that from a policy point of view; they simply implement the flow that Blink
uses to request permission from the user and get information back on whether
permission was granted. This CL doesn't change anything about that flow (which
is totally independent from the flow for location updates).

(BTW, I personally was puzzled when grokking this code that the browser
*doesn't* enforce any checks here).

https://codereview.chromium.org/628773003/diff/200001/content/common/geolocat...
File content/common/geolocation_service.mojom (right):

https://codereview.chromium.org/628773003/diff/200001/content/common/geolocat...
content/common/geolocation_service.mojom:17: };
On 2014/10/21 18:10:06, Tom Sepez wrote:
> Nit: are you sure you don't want to spec a StopUpdating() method while we're
at
> it even if its not being used at the moment?  Closing the pipe to accomplish
> this feels kinda heavy handed to me.

Originally I wanted the client connecting to the service implicitly being a
request to start updating, and the connection being dropped being the way to
stop updating. I had to add the StartUpdating() method because of the
|high_accuracy| parameter and the fact that there's no way for a client to pass
params that a service should be constructed with when connecting to that
service.

I don't have a strong preference here but closing the pipe when updates should
stop feels more idiomatic to me in the Mojo world than adding a StopUpdating()
method. Would be happy to get a Mojo owner's opinion on the question.

[Tom Sepez, 2014-10-21 20:26:40]

LGTM.

> (BTW, I personally was puzzled when grokking this code that the browser
> *doesn't* enforce any checks here).
Me too.  It sounds like this might be something we'd want to keep from a
compromised renderer. Could you do me a favor and file a follow-up bug along the
lines of "Perform browser-side geolocation permission checks"?

> I don't have a strong preference here but closing the pipe when updates should
> stop feels more idiomatic to me in the Mojo world than adding a StopUpdating()
> method. Would be happy to get a Mojo owner's opinion on the question.
No worries. Do this as you'd like.  I just thought I'd call it out.

[Charlie Reis, 2014-10-21 22:28:32]

Looks pretty good, and rubber stamp on the mojom files.  One question below
about how the browser process will know who is making a request.

Also, please CC me on the browser process security check bug Tom mentioned,
since that will probably happen as part of Site Isolation.  It's mentioned in
our threat model:
http://www.chromium.org/developers/design-documents/site-isolation

https://codereview.chromium.org/628773003/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/628773003/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1585: RenderFrameHost*
top_frame = this;
We can use frame_tree_node()->frame_tree()->root() for this, rather than walking
up the tree.

https://codereview.chromium.org/628773003/diff/200001/content/browser/geoloca...
File content/browser/geolocation/geolocation_service_context.cc (right):

https://codereview.chromium.org/628773003/diff/200001/content/browser/geoloca...
content/browser/geolocation/geolocation_service_context.cc:19: new
GeolocationServiceImpl(this, update_callback);
Just want to make sure I follow how things work in the Mojo world.  The
GeolocationServiceImpl is specific to a GeolocationDispatcher in the renderer
process, right?

Does the browser process have any way of knowing which process (etc) this is
for?  I'm curious where we can enforce browser process restrictions based on the
process's site (for Site Isolation).  Let's assume for the moment that RPH ID is
sufficient for the check.

https://codereview.chromium.org/628773003/diff/200001/content/browser/geoloca...
content/browser/geolocation/geolocation_service_context.cc:35: for (auto*
service : services_) {
nit: No braces needed on one line body (here and below).

https://codereview.chromium.org/628773003/diff/200001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/628773003/diff/200001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:302: this),
Just curious-- is this a git cl format effect?  If so, that's fine, though it's
not required by the style guide at call sites.

[Aaron Boodman, 2014-10-22 00:25:37]

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

[Aaron Boodman, 2014-10-22 00:25:38]

https://codereview.chromium.org/628773003/diff/200001/content/common/geolocat...
File content/common/geolocation_service.mojom (right):

https://codereview.chromium.org/628773003/diff/200001/content/common/geolocat...
content/common/geolocation_service.mojom:17: };
On 2014/10/21 20:17:41, blundell wrote:
> On 2014/10/21 18:10:06, Tom Sepez wrote:
> > Nit: are you sure you don't want to spec a StopUpdating() method while we're
> at
> > it even if its not being used at the moment?  Closing the pipe to accomplish
> > this feels kinda heavy handed to me.
> 
> Originally I wanted the client connecting to the service implicitly being a
> request to start updating, and the connection being dropped being the way to
> stop updating. I had to add the StartUpdating() method because of the
> |high_accuracy| parameter and the fact that there's no way for a client to
pass
> params that a service should be constructed with when connecting to that
> service.
> 
> I don't have a strong preference here but closing the pipe when updates should
> stop feels more idiomatic to me in the Mojo world than adding a StopUpdating()
> method. Would be happy to get a Mojo owner's opinion on the question.

Drive-by: I agree that dropping the pipe is more idiomatic mojo.

As to StartUpdate().... is it possible to have a EnableHighAccuracy(bool) method
instead which changes whether the current service is in "high-accuracy mode" or
not?

Not a big deal, just inches closer to what you wanted the interface to be
originally.

[blundell, 2014-10-22 16:05:14]

didn't manage to get to the code changes yet but wanted to respond to the
high-level question.

https://codereview.chromium.org/628773003/diff/200001/content/browser/geoloca...
File content/browser/geolocation/geolocation_service_context.cc (right):

https://codereview.chromium.org/628773003/diff/200001/content/browser/geoloca...
content/browser/geolocation/geolocation_service_context.cc:19: new
GeolocationServiceImpl(this, update_callback);
On 2014/10/21 22:28:32, Charlie Reis wrote:
> Just want to make sure I follow how things work in the Mojo world.  The
> GeolocationServiceImpl is specific to a GeolocationDispatcher in the renderer
> process, right?
> 

Yes.

> Does the browser process have any way of knowing which process (etc) this is
> for?  I'm curious where we can enforce browser process restrictions based on
the
> process's site (for Site Isolation).  Let's assume for the moment that RPH ID
is
> sufficient for the check.

This factory function could take in a RPH ID. When RenderFrameHostImpl adds this
factory function in RFHI::RegisterMojoServices(), it could bind the RPH ID.
Would that cover what you're envisioning?

[Charlie Reis, 2014-10-22 16:11:55]

Thanks.  The rest are just nits, so LGTM once those are fixed.

https://codereview.chromium.org/628773003/diff/200001/content/browser/geoloca...
File content/browser/geolocation/geolocation_service_context.cc (right):

https://codereview.chromium.org/628773003/diff/200001/content/browser/geoloca...
content/browser/geolocation/geolocation_service_context.cc:19: new
GeolocationServiceImpl(this, update_callback);
On 2014/10/22 16:05:14, blundell wrote:
> On 2014/10/21 22:28:32, Charlie Reis wrote:
> > Just want to make sure I follow how things work in the Mojo world.  The
> > GeolocationServiceImpl is specific to a GeolocationDispatcher in the
renderer
> > process, right?
> > 
> 
> Yes.
> 
> > Does the browser process have any way of knowing which process (etc) this is
> > for?  I'm curious where we can enforce browser process restrictions based on
> the
> > process's site (for Site Isolation).  Let's assume for the moment that RPH
ID
> is
> > sufficient for the check.
> 
> This factory function could take in a RPH ID. When RenderFrameHostImpl adds
this
> factory function in RFHI::RegisterMojoServices(), it could bind the RPH ID.
> Would that cover what you're envisioning?

Yes, I think that would do.  I imagine we'll want that to be a frequent pattern,
so that code in the browser process knows which process it is talking to.

[blundell, 2014-10-23 12:27:01]

Thanks! Filed crbug.com/426384.

https://codereview.chromium.org/628773003/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/628773003/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1585: RenderFrameHost*
top_frame = this;
On 2014/10/21 22:28:32, Charlie Reis wrote:
> We can use frame_tree_node()->frame_tree()->root() for this, rather than
walking
> up the tree.

Done.

https://codereview.chromium.org/628773003/diff/200001/content/browser/geoloca...
File content/browser/geolocation/geolocation_service_context.cc (right):

https://codereview.chromium.org/628773003/diff/200001/content/browser/geoloca...
content/browser/geolocation/geolocation_service_context.cc:35: for (auto*
service : services_) {
On 2014/10/21 22:28:32, Charlie Reis wrote:
> nit: No braces needed on one line body (here and below).

I'd prefer to leave the braces...loops without braces feel alien to me :).

https://codereview.chromium.org/628773003/diff/200001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/628773003/diff/200001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:302: this),
On 2014/10/21 22:28:32, Charlie Reis wrote:
> Just curious-- is this a git cl format effect?  If so, that's fine, though
it's
> not required by the style guide at call sites.

Yes, it is.

[blundell, 2014-10-23 13:19:59]

https://codereview.chromium.org/628773003/diff/240001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/628773003/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1587: RenderFrameHost*
top_frame = frame_tree_node()->frame_tree()->GetMainFrame();
Charlie, could you just confirm that this is correct?

[blundell, 2014-10-23 15:23:50]

Hi Michael,

One more go-round: I changed the mojo interface so that the service starts
providing updates on connection, as Aaron suggested; I think it's more
idiomatically Mojo this way. Could you take a look at the diff between PS11 and
PS12? Thanks!

[blundell, 2014-10-23 15:33:51]

tsepez: Could you look at geolocation_service.mojom again? Thanks!

[Charlie Reis, 2014-10-23 17:27:17]

Still LGTM.

https://codereview.chromium.org/628773003/diff/240001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/628773003/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1587: RenderFrameHost*
top_frame = frame_tree_node()->frame_tree()->GetMainFrame();
On 2014/10/23 13:19:59, blundell wrote:
> Charlie, could you just confirm that this is correct?

Yep, that works.

https://codereview.chromium.org/628773003/diff/280001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/628773003/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1205:
GetServiceRegistry()->AddService<GeolocationService>(
nit: Can we add a TODO to add the process ID here, with a reference to
http://crbug.com/426384?

[Tom Sepez, 2014-10-23 18:02:03]

Still LGTM.

[blundell, 2014-10-24 09:57:12]

tsepez@: I added security owners for the mojom files in //content, following the
security owners for IPC message files. My understanding is that you're taking
the lead on doing security reviews for Mojo interfaces. Should I have the list
just be you?

https://codereview.chromium.org/628773003/diff/280001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/628773003/diff/280001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1205:
GetServiceRegistry()->AddService<GeolocationService>(
On 2014/10/23 17:27:17, Charlie Reis wrote:
> nit: Can we add a TODO to add the process ID here, with a reference to
> http://crbug.com/426384

Done.

[Michael van Ouwerkerk, 2014-10-24 10:25:42]

Still lgtm - just a nit about omitting virtual when override is specified

https://codereview.chromium.org/628773003/diff/300001/content/browser/geoloca...
File content/browser/geolocation/geolocation_dispatcher_host.h (right):

https://codereview.chromium.org/628773003/diff/300001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.h:32: virtual void
RenderFrameDeleted(RenderFrameHost* render_frame_host) override;
Drop the 'virtual' -
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/VTNZzizN0zo

https://codereview.chromium.org/628773003/diff/300001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.h:33: virtual void
DidNavigateAnyFrame(RenderFrameHost* render_frame_host,
And here.

https://codereview.chromium.org/628773003/diff/300001/content/browser/geoloca...
File content/browser/geolocation/geolocation_service_impl.h (right):

https://codereview.chromium.org/628773003/diff/300001/content/browser/geoloca...
content/browser/geolocation/geolocation_service_impl.h:39: virtual void
SetHighAccuracy(bool high_accuracy) override;
Omit virtual

https://codereview.chromium.org/628773003/diff/300001/content/browser/geoloca...
content/browser/geolocation/geolocation_service_impl.h:42: virtual void
OnConnectionError() override;
And here

https://codereview.chromium.org/628773003/diff/300001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/628773003/diff/300001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:385: virtual
GeolocationServiceContext* GetGeolocationServiceContext() override;
Omit virtual

https://codereview.chromium.org/628773003/diff/300001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:387: virtual
gfx::NativeViewAccessible GetParentNativeViewAccessible() override;
Might as well delete it here too

[blundell, 2014-10-24 11:45:18]

https://codereview.chromium.org/628773003/diff/300001/content/browser/geoloca...
File content/browser/geolocation/geolocation_dispatcher_host.h (right):

https://codereview.chromium.org/628773003/diff/300001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.h:32: virtual void
RenderFrameDeleted(RenderFrameHost* render_frame_host) override;
On 2014/10/24 10:25:42, Michael van Ouwerkerk wrote:
> Drop the 'virtual' -
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/VTNZzizN0zo

Done.

https://codereview.chromium.org/628773003/diff/300001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.h:33: virtual void
DidNavigateAnyFrame(RenderFrameHost* render_frame_host,
On 2014/10/24 10:25:42, Michael van Ouwerkerk wrote:
> And here.

Done.

https://codereview.chromium.org/628773003/diff/300001/content/browser/geoloca...
File content/browser/geolocation/geolocation_service_impl.h (right):

https://codereview.chromium.org/628773003/diff/300001/content/browser/geoloca...
content/browser/geolocation/geolocation_service_impl.h:39: virtual void
SetHighAccuracy(bool high_accuracy) override;
On 2014/10/24 10:25:42, Michael van Ouwerkerk wrote:
> Omit virtual

Done.

https://codereview.chromium.org/628773003/diff/300001/content/browser/geoloca...
content/browser/geolocation/geolocation_service_impl.h:42: virtual void
OnConnectionError() override;
On 2014/10/24 10:25:42, Michael van Ouwerkerk wrote:
> And here

Done.

https://codereview.chromium.org/628773003/diff/300001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/628773003/diff/300001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:385: virtual
GeolocationServiceContext* GetGeolocationServiceContext() override;
On 2014/10/24 10:25:42, Michael van Ouwerkerk wrote:
> Omit virtual

Done.

https://codereview.chromium.org/628773003/diff/300001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:387: virtual
gfx::NativeViewAccessible GetParentNativeViewAccessible() override;
On 2014/10/24 10:25:42, Michael van Ouwerkerk wrote:
> Might as well delete it here too

Done.

[Tom Sepez, 2014-10-24 16:54:13]

On 2014/10/24 09:57:12, blundell wrote:
> tsepez@: I added security owners for the mojom files in //content, following
the
> security owners for IPC message files. My understanding is that you're taking
> the lead on doing security reviews for Mojo interfaces. Should I have the list
> just be you?
> 
Just me.  Then I think jschuh is next to be added when he gets back.

[blundell, 2014-10-27 16:31:46]

On 2014/10/24 16:54:13, Tom Sepez wrote:
> On 2014/10/24 09:57:12, blundell wrote:
> > tsepez@: I added security owners for the mojom files in //content, following
> the
> > security owners for IPC message files. My understanding is that you're
taking
> > the lead on doing security reviews for Mojo interfaces. Should I have the
list
> > just be you?
> > 
> Just me.  Then I think jschuh is next to be added when he gets back.

Done.

[blundell, 2014-10-28 09:21:18]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-10-28 09:22:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/628773003/360001

[commit-bot: I haz the power, 2014-10-28 11:58:27]

Committed patchset #16 (id:360001)

[commit-bot: I haz the power, 2014-10-28 11:59:05]

Patchset 16 (id:??) landed as
https://crrev.com/28e88081438dc08b06d5f05cfdd980c407db1638
Cr-Commit-Position: refs/heads/master@{#301604}

[yurys, 2014-10-28 13:26:08]

A revert of this CL (patchset #16 id:360001) has been created in
https://codereview.chromium.org/680323002/ by yurys@chromium.org.

The reason for reverting is: Made inspector/geolocation-emulation-tests.html
layout test crash on all platforms


http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec....

[blundell, 2014-10-29 10:58:09]

Patchset #17 (id:380001) has been deleted

[blundell, 2014-10-29 11:00:56]

Michael,

This CL was reverted because it caused a LayoutTests crash, which turned out to
be because in the geolocation override case I was calling a method on client()
before binding the service, whereas client() is non-null only *after* binding
the service. Could you take a look over the diff between PS17 and PS18 for me to
reland? Thanks!

[Michael van Ouwerkerk, 2014-10-29 11:06:52]

lgtm

[blundell, 2014-10-29 11:36:49]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-10-29 11:37:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/628773003/420001

[commit-bot: I haz the power, 2014-10-29 11:42:38]

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

[commit-bot: I haz the power, 2014-10-29 11:42:41]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[blundell, 2014-10-29 12:03:51]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-10-29 12:04:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/628773003/440001

[commit-bot: I haz the power, 2014-10-29 13:20:07]

Committed patchset #19 (id:440001)

[commit-bot: I haz the power, 2014-10-29 13:21:01]

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

[jam, 2016-01-26 22:56:06]

I just saw this

why is 	content/public/common/mojo_geoposition.mojom in
content/public/common instead of content/common? since it's not used by anyone
outside content, it should be in content/common.

please move it, per
https://www.chromium.org/developers/content-module/content-api

[Ben Goodger (Google), 2016-01-26 22:58:53]

On 2016/01/26 22:56:06, jam wrote:
> I just saw this
> 
> why is 	content/public/common/mojo_geoposition.mojom in
> content/public/common instead of content/common? since it's not used by anyone
> outside content, it should be in content/common.
> 
> please move it, per
> https://www.chromium.org/developers/content-module/content-api

Some nits too:

struct MojoGeoposition -> struct Geoposition
filename mojo_geoposition.mojom -> geoposition.mojom
module content { .. } -> module content.mojom;

[blundell, 2016-01-28 09:45:22]

On 2016/01/26 22:58:53, Ben Goodger (Google) wrote:
> On 2016/01/26 22:56:06, jam wrote:
> > I just saw this
> > 
> > why is 	content/public/common/mojo_geoposition.mojom in
> > content/public/common instead of content/common? since it's not used by
anyone
> > outside content, it should be in content/common.
> > 
> > please move it, per
> > https://www.chromium.org/developers/content-module/content-api
> 
> Some nits too:
> 
> struct MojoGeoposition -> struct Geoposition
> filename mojo_geoposition.mojom -> geoposition.mojom

The answer to all three of these points is that
//content/public/common/geoposition.h defines content::Geoposition, which the
Mojo struct was intended to globally replace (see the comment in
mojo_geoposition.mojom). However, I haven't kept up with how the allowance of
C++ types in mojoms might affect this. Would it now be possible to directly use
geoposition.h in the mojom interfaces instead?

> module content { .. } -> module content.mojom;

Holding off on this pending resolution of the above question.