Implement basic mojo Permission service and use it for Geolocation.

Implement basic mojo Permission service and use it for Geolocation.

This is implementing one side of the mojo Permission service: requesting
a permission. This is being used by the Geolocation code in the renderer
process.

This CL is creating all the required hooks for the permission service
and can be used as a base to increment on top of.

BUG=430238, 420497
Committed: https://crrev.com/4ebe69f4f611e1eef0d8886b97abdae37687a14b
Cr-Commit-Position: refs/heads/master@{#304227}

[mlamouri (slow - plz ping), 2014-11-13 22:05:30]

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

[mlamouri (slow - plz ping), 2014-11-13 22:05:30]

mlamouri@chromium.org changed required reviewers:
+ creis@chromium.org, tsepez@chromium.org

[mlamouri (slow - plz ping), 2014-11-13 22:05:31]

tsepez@chromium.org: Please review changes in:
- content/common/

creis@chromium.org: Please review changes in:
- content/ (OWNER review)

blundell@chromium.org: Please review changes in:
- *

I will ask blundell@ to do a mojo specific review but it would be great to have
an owner review for content/ too :)

[mlamouri (slow - plz ping), 2014-11-13 22:09:54]

mlamouri@chromium.org changed reviewers:
+ nasko@chromium.org

[mlamouri (slow - plz ping), 2014-11-13 22:09:54]

nasko@, if you happen to have some time to have a look, it will be really
appreciated :)

(I remember that creis@ said that he was a bit behind with reviews.)

[Tom Sepez, 2014-11-13 22:51:42]

Messages themselves LGTM.

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:23: return
PERMISSION_NUM;
nit: name doesn't make sense to me - did you want NONE or NULL or some such?

[mlamouri (slow - plz ping), 2014-11-13 22:58:42]

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:23: return
PERMISSION_NUM;
On 2014/11/13 22:51:42, Tom Sepez wrote:
> nit: name doesn't make sense to me - did you want NONE or NULL or some such?

I'm just returning something because I have to - the switch will otherwise catch
all the possible cases (one for now). I only have this return statement at the
end because some compilers might complain about it missing. _NUM is the
non-meaningful value I found in the PermissionType enum so I picked that one.
Not sure if it's worth creating a new one for a code branch that should not
happen.

What would you suggest?

[Tom Sepez, 2014-11-13 23:17:35]

On 2014/11/13 22:58:42, Mounir Lamouri wrote:
>
https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
> File content/browser/permissions/permission_service_impl.cc (right):
> 
>
https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
> content/browser/permissions/permission_service_impl.cc:23: return
> PERMISSION_NUM;
> On 2014/11/13 22:51:42, Tom Sepez wrote:
> > nit: name doesn't make sense to me - did you want NONE or NULL or some such?
> 
> I'm just returning something because I have to - the switch will otherwise
catch
> all the possible cases (one for now). I only have this return statement at the
> end because some compilers might complain about it missing. _NUM is the
> non-meaningful value I found in the PermissionType enum so I picked that one.
> Not sure if it's worth creating a new one for a code branch that should not
> happen.
> 
> What would you suggest?\
yeah, ok. seems hard to justify adding something like an _INVALID value just for
this.

[nasko, 2014-11-13 23:57:38]

Apologies for the amount of questions, but this is the first time I see mojo
used in practice, so I want to make sure I understand what is going on.

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_context.h (right):

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_context.h:17: //
PermissionService to handle request permission UI.
Since this class has a member pointer to RenderFrameHost, it will be useful to
put in the description what is the relationship and lifetime of this object. Is
it one per RFH? Who creates/destroys it?

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:18: case
PERMISSION_NAME_GEOLOCATION:
Where is this defined? Is it auto-generated somehow?

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:23: return
PERMISSION_NUM;
On 2014/11/13 22:58:42, Mounir Lamouri wrote:
> On 2014/11/13 22:51:42, Tom Sepez wrote:
> > nit: name doesn't make sense to me - did you want NONE or NULL or some such?
> 
> I'm just returning something because I have to - the switch will otherwise
catch
> all the possible cases (one for now). I only have this return statement at the
> end because some compilers might complain about it missing. _NUM is the
> non-meaningful value I found in the PermissionType enum so I picked that one.
> Not sure if it's worth creating a new one for a code branch that should not
> happen.
> 
> What would you suggest?

Why not rename the constant, since the name doesn't really make much sense.
_INVALID or some other equivalent is a lot more meaningful. I won't mind if it
is in a different patch if you want to keep this one smaller.

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:31: , origin(origin) {
nit: Chromium style puts commas at the end of the line.

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:37: BindToRequest(new
PermissionServiceImpl(context.Pass()), &request);
nit: prefix with mojo:: to be clear that this isn't part of this class or the
content module.

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:51: PermissionPtr
permission,
I assume PermissionPtr is a generated pointer type to the Permission struct in
the mojom file, correct?

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:54: if
(!context_->render_frame_host()) {
When is this valid?

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:71: true, //
TODO(mlamouri): should be removed, see http://crbug.com/423770
Do we plan to remove user gesture indicator in general? Why isn't it present in
the Permission struct?

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:85:
GeolocationProviderImpl::GetInstance()->UserDidOptIntoLocationServices();
Why are we calling this without checking the value of "allowed"?

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.h (right):

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.h:17: // Implements the
PermissionService Mojo interface.
For devs like me, who aren't familiar with Mojo semantics, it will be useful to
put a bit more information about lifetime of this object and relationship to
other concepts such as process/page/frame.

https://codereview.chromium.org/722153003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/722153003/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:882: new
PermissionServiceContext(nullptr));
Why is it valid to register a context with null RFH?

[mlamouri (slow - plz ping), 2014-11-14 00:16:34]

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_context.h (right):

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_context.h:17: //
PermissionService to handle request permission UI.
On 2014/11/13 23:57:37, nasko wrote:
> Since this class has a member pointer to RenderFrameHost, it will be useful to
> put in the description what is the relationship and lifetime of this object.
Is
> it one per RFH? Who creates/destroys it?

There is one per RFH. Created when the RFH is created, then owned by the
PermissionService. I will add a comment explaining that.

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:18: case
PERMISSION_NAME_GEOLOCATION:
On 2014/11/13 23:57:37, nasko wrote:
> Where is this defined? Is it auto-generated somehow?

Auto-generated from the .mojom file.

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:23: return
PERMISSION_NUM;
On 2014/11/13 23:57:38, nasko wrote:
> On 2014/11/13 22:58:42, Mounir Lamouri wrote:
> > On 2014/11/13 22:51:42, Tom Sepez wrote:
> > > nit: name doesn't make sense to me - did you want NONE or NULL or some
such?
> > 
> > I'm just returning something because I have to - the switch will otherwise
> catch
> > all the possible cases (one for now). I only have this return statement at
the
> > end because some compilers might complain about it missing. _NUM is the
> > non-meaningful value I found in the PermissionType enum so I picked that
one.
> > Not sure if it's worth creating a new one for a code branch that should not
> > happen.
> > 
> > What would you suggest?
> 
> Why not rename the constant, since the name doesn't really make much sense.
> _INVALID or some other equivalent is a lot more meaningful. I won't mind if it
> is in a different patch if you want to keep this one smaller.

Sounds reasonable. I think miguelg@ wrote that. I will check with him.

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:31: , origin(origin) {
On 2014/11/13 23:57:37, nasko wrote:
> nit: Chromium style puts commas at the end of the line.

Acknowledged.

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:37: BindToRequest(new
PermissionServiceImpl(context.Pass()), &request);
On 2014/11/13 23:57:38, nasko wrote:
> nit: prefix with mojo:: to be clear that this isn't part of this class or the
> content module.

Done in my local copy. Actually, I wonder if the win compile failures are
related.

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:54: if
(!context_->render_frame_host()) {
On 2014/11/13 23:57:37, nasko wrote:
> When is this valid?

The condition is valid if the call is coming from a ChildThread instead of a
RenderFrame. Some callers like Push or Notifications run in workers or documents
and we want the service to still return something if they end up requesting
permissions from a worker context (ie. ChildThread).

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:71: true, //
TODO(mlamouri): should be removed, see http://crbug.com/423770
On 2014/11/13 23:57:38, nasko wrote:
> Do we plan to remove user gesture indicator in general? Why isn't it present
in
> the Permission struct?

Yes, we want to remove it, see the linked bug. The user gesture indicator is
only used for a behind a flag feature that is no longer actively worked on.
There is someone assigned (on vacation, back Monday) to move the logic of the
user_gesture to the renderer. See the bug for more details.

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:85:
GeolocationProviderImpl::GetInstance()->UserDidOptIntoLocationServices();
On 2014/11/13 23:57:37, nasko wrote:
> Why are we calling this without checking the value of "allowed"?

Because I added it at the last minute when I realised this was called by
geolocation_dispatcher_host :)

Will fix.

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.h (right):

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.h:17: // Implements the
PermissionService Mojo interface.
On 2014/11/13 23:57:38, nasko wrote:
> For devs like me, who aren't familiar with Mojo semantics, it will be useful
to
> put a bit more information about lifetime of this object and relationship to
> other concepts such as process/page/frame.

Acknowledged.

https://codereview.chromium.org/722153003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/722153003/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:882: new
PermissionServiceContext(nullptr));
On 2014/11/13 23:57:38, nasko wrote:
> Why is it valid to register a context with null RFH?

The service on the RenderProcessHost can't show any UI (RFH is needed for that)
but we still want a service to be available for the rest of the features it
provides.

I've decided to pass a null RFH so the service always have a context but will
have to check whether its context has a RFH/WebContents. Does that sound sane to
you? Alternatively, I could pass a null context.

[mlamouri (slow - plz ping), 2014-11-14 00:29:28]

mlamouri@chromium.org changed required reviewers:
+ blundell@chromium.org, nasko@chromium.org
- creis@chromium.org

[mlamouri (slow - plz ping), 2014-11-14 00:29:29]

nasko@, I've applied the comments. PTAL.

Regarding mojo specificities, blundell@, in the reviewer list, knows about mojo
in Chromium and he should be able to review that side of the CL.

[nasko, 2014-11-14 00:42:50]

LGTM once the comments are addressed.

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/722153003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:54: if
(!context_->render_frame_host()) {
On 2014/11/14 00:16:33, Mounir Lamouri wrote:
> On 2014/11/13 23:57:37, nasko wrote:
> > When is this valid?
> 
> The condition is valid if the call is coming from a ChildThread instead of a
> RenderFrame. Some callers like Push or Notifications run in workers or
documents
> and we want the service to still return something if they end up requesting
> permissions from a worker context (ie. ChildThread).

What you just described to me will fit very well as a comment above this block.
It isn't self-describing, so the extra context will help readers of the code
later.

https://codereview.chromium.org/722153003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/722153003/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:882: new
PermissionServiceContext(nullptr));
On 2014/11/14 00:16:33, Mounir Lamouri wrote:
> On 2014/11/13 23:57:38, nasko wrote:
> > Why is it valid to register a context with null RFH?
> 
> The service on the RenderProcessHost can't show any UI (RFH is needed for
that)
> but we still want a service to be available for the rest of the features it
> provides.
> 
> I've decided to pass a null RFH so the service always have a context but will
> have to check whether its context has a RFH/WebContents. Does that sound sane
to
> you? Alternatively, I could pass a null context.

I like always having a context and it including the indicators on how it should
be used. I think what you have here is fine, it was more of me trying to get the
bigger picture of how this works.

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:43: , weak_factory_(this)
{
Another instance of the comma on new line :).

[blundell, 2014-11-14 08:29:20]

This looks good to me overall, nice work!

A couple high-level comments:

- If the WebContents goes away while there are live PermissionServiceImpl
instances, you'll have dangling pointers. I suggest moving to the ownership
relationship of GeolocationService{Impl, Context}, where the Context owns the
Impls and the RFHI owns the Context. To do this, use WeakBindToPipe rather than
BindToPipe.

- We're eventually going to want PermissionService to live in //mojo rather than
//content so that it can be used at lower levels than //content (e.g., so that
GeolocationServiceImpl can verify that permissions were obtained on the browser
side even after GeolocationServiceImpl is moved from //content to //device). To
enable that, we would want PermissionServiceContext to be an interface that has
methods like RequestPermission() and CancelPermissionRequests() [there would
then be a //content impl of that interface]. While we don't need to worry too
much about this at this point, I do suggest hiding all the //content
dependencies from PermissionServiceImpl, instead just having
PermissionServiceImpl call methods on PermissionServiceContext.

https://codereview.chromium.org/722153003/diff/40001/content/common/permissio...
File content/common/permission_service.mojom (right):

https://codereview.chromium.org/722153003/diff/40001/content/common/permissio...
content/common/permission_service.mojom:18: PermissionName name;
What is the reason for the existence of the Permission struct? If you're
anticipating adding more things besides the name, I would suggest delaying until
it's actually needed unless that's in a short-term followup.

[blundell, 2014-11-14 08:31:13]

https://codereview.chromium.org/722153003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/722153003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:884:
base::Bind(&PermissionServiceImpl::Create,
I think you should delay adding this until you have a consumer of it.

[qsr, 2014-11-14 09:20:25]

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

[qsr, 2014-11-14 09:20:26]

https://codereview.chromium.org/722153003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/722153003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1277:
base::Passed(&permission_service_context)));
This callback will be run each time something connect to the permission service.
Given that the first call will pass the context to the first instance, all other
instance will have a null context.

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
File content/browser/permissions/permission_service_context.h (right):

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
content/browser/permissions/permission_service_context.h:33: void
set_service(PermissionServiceImpl* service);
This seems weird. There is a single context for multiple PermissionServiceImpl.

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:111: void
PermissionServiceImpl::RevokePermission(
I wouldn't add those methods in the mojom until you have both a client and an
implementation.

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.h (right):

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
content/browser/permissions/permission_service_impl.h:71: typedef
IDMap<PendingRequest, IDMapOwnPointer> RequestsMap;
Declaration order seems weird:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

I would put the struct and typedef at the start of the private section.

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

https://codereview.chromium.org/722153003/diff/40001/content/renderer/geoloca...
content/renderer/geolocation_dispatcher.cc:89: permission.Clone(),
You should pass the permission, instead of cloning it, you do not need it
afterwards.

[mlamouri (slow - plz ping), 2014-11-14 11:37:11]

I applied the comments.

blundell@ and qsr@, there is only the question of the Context/Service
relationship and lifetime that needs to be resolved but I'm not sure how I can
resolve your comments because it seems that I had false assumptions about how
things work and I'm not sure I follow all of your comments. For example, my
vision was that I would have one PermissionServiceContext per
PermissionServiceImpl. I will not have a dangling pointer because the
PermissionServiceContext is a WebContentsObserver and will simply return null
for web_contents() when the WebContents is removed. Though, my understanding of
the lifetime of a service might be wrong.

https://codereview.chromium.org/722153003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/722153003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1277:
base::Passed(&permission_service_context)));
On 2014/11/14 09:20:26, qsr wrote:
> This callback will be run each time something connect to the permission
service.
> Given that the first call will pass the context to the first instance, all
other
> instance will have a null context.

I might need some deeper understanding on how things work. My understanding was
that there would be one service per RFH. Do you mean that there is one service
instance per clients? Maybe in our case it doesn't change much because there is
a 1:1 relationship betweer RF/RFH?

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
File content/browser/permissions/permission_service_context.h (right):

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
content/browser/permissions/permission_service_context.h:33: void
set_service(PermissionServiceImpl* service);
On 2014/11/14 09:20:26, qsr wrote:
> This seems weird. There is a single context for multiple
PermissionServiceImpl.

That's not the model I had in mind. Contrary to the Geolocation service, I
really need to know about the RFH for the permissions to be able to dismiss
permissions prompts that come from a frame when it is navigated. I wonder if I
then need one context per service or simply pass the RFH to the service?

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:43: , weak_factory_(this)
{
On 2014/11/14 00:42:50, nasko wrote:
> Another instance of the comma on new line :).

Damn coding style :)

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:111: void
PermissionServiceImpl::RevokePermission(
On 2014/11/14 09:20:26, qsr wrote:
> I wouldn't add those methods in the mojom until you have both a client and an
> implementation.

Done. Except for HasPermission() because I use that in the RequestPermission()
logic and it will be added in a short term future anyway.

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.h (right):

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
content/browser/permissions/permission_service_impl.h:71: typedef
IDMap<PendingRequest, IDMapOwnPointer> RequestsMap;
On 2014/11/14 09:20:26, qsr wrote:
> Declaration order seems weird:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
> 
> I would put the struct and typedef at the start of the private section.

Done.

https://codereview.chromium.org/722153003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/722153003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:884:
base::Bind(&PermissionServiceImpl::Create,
On 2014/11/14 08:31:13, blundell wrote:
> I think you should delay adding this until you have a consumer of it.

I will use it in a short term future. Push, Geolocation and Notifications are
the three features that will use the Permission service as soon as it is
available and only Geolocation is per-frame only. The others are running in
workers.

https://codereview.chromium.org/722153003/diff/40001/content/common/permissio...
File content/common/permission_service.mojom (right):

https://codereview.chromium.org/722153003/diff/40001/content/common/permissio...
content/common/permission_service.mojom:18: PermissionName name;
On 2014/11/14 08:29:20, blundell wrote:
> What is the reason for the existence of the Permission struct? If you're
> anticipating adding more things besides the name, I would suggest delaying
until
> it's actually needed unless that's in a short-term followup.

Done.

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

https://codereview.chromium.org/722153003/diff/40001/content/renderer/geoloca...
content/renderer/geolocation_dispatcher.cc:89: permission.Clone(),
On 2014/11/14 09:20:26, qsr wrote:
> You should pass the permission, instead of cloning it, you do not need it
> afterwards.

For some reasons, I had to Clone(), it wouldn't compile otherwise. I wonder if
that was a copy constructor missing (can't remember exactly the error). It's
moot now anyway because I removed the struct.

[qsr, 2014-11-14 11:49:27]

https://codereview.chromium.org/722153003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/722153003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1277:
base::Passed(&permission_service_context)));
On 2014/11/14 11:37:11, Mounir Lamouri wrote:
> On 2014/11/14 09:20:26, qsr wrote:
> > This callback will be run each time something connect to the permission
> service.
> > Given that the first call will pass the context to the first instance, all
> other
> > instance will have a null context.
> 
> I might need some deeper understanding on how things work. My understanding
was
> that there would be one service per RFH. Do you mean that there is one service
> instance per clients? Maybe in our case it doesn't change much because there
is
> a 1:1 relationship betweer RF/RFH?

 What you register is a factory. Each time something will request a connection
to the service, the factory will be called. So you need to consider that it will
be called more than once. That's why you should share the context between all
the instances. What Collin was suggesting (and I agree) is to have the context
owned by the RFH. It can then have the factory method itself, and each time it
creates a service, it owns it (using WeakBind instead of Bind), and delete all
of those when it is killed. It should also monitor the service for connection
error and delete those that go into an error state.

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/722153003/diff/40001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:111: void
PermissionServiceImpl::RevokePermission(
On 2014/11/14 11:37:11, Mounir Lamouri wrote:
> On 2014/11/14 09:20:26, qsr wrote:
> > I wouldn't add those methods in the mojom until you have both a client and
an
> > implementation.
> 
> Done. Except for HasPermission() because I use that in the RequestPermission()
> logic and it will be added in a short term future anyway.

 If you use it, shouldn't you implement it? With the current state, your
callback will never be called, so the client will never have any answer.

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

https://codereview.chromium.org/722153003/diff/40001/content/renderer/geoloca...
content/renderer/geolocation_dispatcher.cc:89: permission.Clone(),
On 2014/11/14 11:37:11, Mounir Lamouri wrote:
> On 2014/11/14 09:20:26, qsr wrote:
> > You should pass the permission, instead of cloning it, you do not need it
> > afterwards.
> 
> For some reasons, I had to Clone(), it wouldn't compile otherwise. I wonder if
> that was a copy constructor missing (can't remember exactly the error). It's
> moot now anyway because I removed the struct.

 When I say pass, I mean calling the Pass() method as for a scoped_ptr

https://codereview.chromium.org/722153003/diff/80001/content/common/permissio...
File content/common/permission_service.mojom (right):

https://codereview.chromium.org/722153003/diff/80001/content/common/permissio...
content/common/permission_service.mojom:20: // [Client=PermissionServiceClient]
Just delete this instead of commenting it.

[blundell, 2014-11-14 12:11:52]

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

https://codereview.chromium.org/722153003/diff/80001/content/renderer/geoloca...
content/renderer/geolocation_dispatcher.cc:91: weak_ptr_factory_.GetWeakPtr(),
This doesn't need to be a weak ptr, as |permission_service_| is owned by this
instance.

[mlamouri (slow - plz ping), 2014-11-14 12:38:48]

I think I applied all your comments in this new patch.

PTAL and let me know if I missed something.

[blundell, 2014-11-14 12:48:08]

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

https://codereview.chromium.org/722153003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1255:
permission_service_context_.reset(new PermissionServiceContext(this));
nit: I think this might as well be a plain object initialized in the initializer
list.

https://codereview.chromium.org/722153003/diff/100001/content/browser/permiss...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/722153003/diff/100001/content/browser/permiss...
content/browser/permissions/permission_service_impl.cc:8: #include
"content/public/browser/content_browser_client.h"
I would still prefer that you push these //content dependencies into
PermissionServiceContext, as I described in my first review.

[mlamouri (slow - plz ping), 2014-11-14 13:39:42]

PTAL

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

https://codereview.chromium.org/722153003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1255:
permission_service_context_.reset(new PermissionServiceContext(this));
On 2014/11/14 12:48:08, blundell wrote:
> nit: I think this might as well be a plain object initialized in the
initializer
> list.

The issue is that I use the parameter to access the WebContents. It's usually
not a great thing to start using |this| in a ctor and I also wouldn't like to
assume the WebContents is set at that time (it might or might not but I don't
think it's something we should rely on).

Though, that doesn't apply for RenderProcessHost, will change it there.

https://codereview.chromium.org/722153003/diff/100001/content/browser/permiss...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/722153003/diff/100001/content/browser/permiss...
content/browser/permissions/permission_service_impl.cc:8: #include
"content/public/browser/content_browser_client.h"
On 2014/11/14 12:48:08, blundell wrote:
> I would still prefer that you push these //content dependencies into
> PermissionServiceContext, as I described in my first review.

If we want to do something like that, we could rename the context a delegate and
move the entire logic there. I suggest to do that later based on need.

[qsr, 2014-11-14 14:28:18]

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_impl.cc:117: NOTIMPLEMENTED();
I still do not understand that. If you reach that point, you will have a
callback that will never receive a response. Can you either implement that or
remove it from this CL and make sure you do not call it. (You can move the
NOTIMPLEMENTED(); to line 60).

https://codereview.chromium.org/722153003/diff/120001/content/common/permissi...
File content/common/permission_service.mojom (right):

https://codereview.chromium.org/722153003/diff/120001/content/common/permissi...
content/common/permission_service.mojom:25: //   RevokePermission(PermissionName
permission, string origin)
Can you delete the lines instead of using comments.

[blundell, 2014-11-14 14:42:00]

Given that PermissionServiceImpl is //content-specific at this time (which I'm
OK with), I think that PermissionServiceContext isn't serving much purpose
compared to folding that logic into PermissionServiceImpl itself and worrying
about generalizing/refactoring whenever the time comes. But I don't feel
strongly enough about it to insist on it.

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
File content/browser/permissions/permission_service_context.h (right):

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_context.h:21: // The
RenderFrameHost creates an instance when setting up the Permission mojo
This comment is out-of-date.

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_context.h:32: explicit
PermissionServiceContext(RenderFrameHost* render_frame_host);
these should be at the top, and you can kill the duplicate "public:".

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_context.h:36: void
RenderFrameDeleted(RenderFrameHost* render_frame_host) override;
nit: I would make these private to make it clear they're not part of the public
interface.

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_impl.cc:65: int request_id =
pending_requests_.Add(
Not for this CL, but when would a PermissionService receive more than 1 incoming
request at a time?

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
File content/browser/permissions/permission_service_impl.h (right):

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_impl.h:19: // It receives at
PermissionContext instance when created (that it then owns)
This comment is out of date.

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_impl.h:45: void QueryPermission(
hmm, QueryPermission and RequestPermission sound confusingly similar.  I liked
HasPermission better, tbh. Or maybe QueryPermissionStatus?

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_impl.h:57: void
OnRequestPermissionResponse(
nit: I think I'd be inclined to call this just OnPermissionResponse.

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_impl.h:63:
PermissionServiceContext* context_;
// weak; owns this object.

https://codereview.chromium.org/722153003/diff/120001/content/common/permissi...
File content/common/permission_service.mojom (right):

https://codereview.chromium.org/722153003/diff/120001/content/common/permissi...
content/common/permission_service.mojom:23: RequestPermission(PermissionName
permission, string origin)
You might want to specify that this returns the current permission status if the
service is not in a context where it can ask the user for permissions?

https://codereview.chromium.org/722153003/diff/120001/content/common/permissi...
content/common/permission_service.mojom:25: //   RevokePermission(PermissionName
permission, string origin)
I would kill all of this rather than land commented-out code.

[mlamouri (slow - plz ping), 2014-11-14 16:14:44]

mlamouri@chromium.org changed required reviewers:
- blundell@chromium.org

[mlamouri (slow - plz ping), 2014-11-14 16:14:45]

Comments applied.

blundell@ told me offline that his comments were the last he had and the design
questions could be addressed in a follow-up. Given that I have to catch a train
and go on holidays for a week, I will land this in order to not block
timvolodine@.

Hopefully Tim will have enough cycles to work on HasPermission() when I'm off.
Otherwise I will do that as soon as I'm back.

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
File content/browser/permissions/permission_service_context.h (right):

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_context.h:21: // The
RenderFrameHost creates an instance when setting up the Permission mojo
On 2014/11/14 14:41:59, blundell wrote:
> This comment is out-of-date.

Acknowledged.

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_context.h:32: explicit
PermissionServiceContext(RenderFrameHost* render_frame_host);
On 2014/11/14 14:41:59, blundell wrote:
> these should be at the top, and you can kill the duplicate "public:".

Oups :)

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_context.h:36: void
RenderFrameDeleted(RenderFrameHost* render_frame_host) override;
On 2014/11/14 14:41:59, blundell wrote:
> nit: I would make these private to make it clear they're not part of the
public
> interface.

Done.

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_impl.cc:65: int request_id =
pending_requests_.Add(
On 2014/11/14 14:41:59, blundell wrote:
> Not for this CL, but when would a PermissionService receive more than 1
incoming
> request at a time?

This is definitely possible. Have you ever seen those infobar stacks? They can
all come from the same RenderFrame. In the current implementation, aware of only
Geolocation, it's quite unlikely though ;)

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_impl.cc:117: NOTIMPLEMENTED();
On 2014/11/14 14:28:18, qsr wrote:
> I still do not understand that. If you reach that point, you will have a
> callback that will never receive a response. Can you either implement that or
> remove it from this CL and make sure you do not call it. (You can move the
> NOTIMPLEMENTED(); to line 60).

The difference between having NOTIMPLEMENTED() in line 60 or here is small and
HasPermission() is the next thing that will be implemented for that service. It
has the advantage of making ::RequestPermission() flow clear.

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
File content/browser/permissions/permission_service_impl.h (right):

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_impl.h:19: // It receives at
PermissionContext instance when created (that it then owns)
On 2014/11/14 14:41:59, blundell wrote:
> This comment is out of date.

Acknowledged.

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_impl.h:45: void QueryPermission(
On 2014/11/14 14:42:00, blundell wrote:
> hmm, QueryPermission and RequestPermission sound confusingly similar.  I liked
> HasPermission better, tbh. Or maybe QueryPermissionStatus?

Hmm... HasPermission() has the disadvantage to look like a boolean but meh...
Back to Has.

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_impl.h:57: void
OnRequestPermissionResponse(
On 2014/11/14 14:42:00, blundell wrote:
> nit: I think I'd be inclined to call this just OnPermissionResponse.

Hmm, I prefer not to. For two reasons:
- The pattern Foo() -> OnFooResponse() has the advantage of being clear;
- Theere will likely be a callback for getting the current permission status
which might make |OnPermissionResponse| confusing ;)

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_impl.h:63:
PermissionServiceContext* context_;
On 2014/11/14 14:42:00, blundell wrote:
> // weak; owns this object.

Done.

https://codereview.chromium.org/722153003/diff/120001/content/common/permissi...
File content/common/permission_service.mojom (right):

https://codereview.chromium.org/722153003/diff/120001/content/common/permissi...
content/common/permission_service.mojom:23: RequestPermission(PermissionName
permission, string origin)
On 2014/11/14 14:42:00, blundell wrote:
> You might want to specify that this returns the current permission status if
the
> service is not in a context where it can ask the user for permissions?

I would consider this an implementation detail. As far as the caller is
concerned, the new permission status is returned.

https://codereview.chromium.org/722153003/diff/120001/content/common/permissi...
content/common/permission_service.mojom:25: //   RevokePermission(PermissionName
permission, string origin)
On 2014/11/14 14:42:00, blundell wrote:
> I would kill all of this rather than land commented-out code.

Done.

[mlamouri (slow - plz ping), 2014-11-14 16:15:39]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-11-14 16:16:34]

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

[commit-bot: I haz the power, 2014-11-14 17:25:30]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-14 17:26:13]

Patchset 8 (id:??) landed as
https://crrev.com/4ebe69f4f611e1eef0d8886b97abdae37687a14b
Cr-Commit-Position: refs/heads/master@{#304227}

[blundell, 2014-11-17 13:14:01]

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/722153003/diff/120001/content/browser/permiss...
content/browser/permissions/permission_service_impl.cc:65: int request_id =
pending_requests_.Add(
On 2014/11/14 16:14:45, Mounir Lamouri wrote:
> On 2014/11/14 14:41:59, blundell wrote:
> > Not for this CL, but when would a PermissionService receive more than 1
> incoming
> > request at a time?
> 
> This is definitely possible. Have you ever seen those infobar stacks? They can
> all come from the same RenderFrame. In the current implementation, aware of
only
> Geolocation, it's quite unlikely though ;)

More than 1 incoming request per RenderFrame at a time is not the same thing as
a given PermissionsServiceImpl instance receiving more than 1 incoming request
at a time. Keep in mind that each different client will connect to a different
PermissionServiceImpl instance.