Implement basic mojo Permission service and use it for Geolocation.

content/browser/permissions/permission_service_impl.cc

Index: content/browser/permissions/permission_service_impl.cc
diff --git a/content/browser/permissions/permission_service_impl.cc b/content/browser/permissions/permission_service_impl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e6ebfbf2909eaae7b86b976489d14bb462bf030c
--- /dev/null
+++ b/content/browser/permissions/permission_service_impl.cc
@@ -0,0 +1,136 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/permissions/permission_service_impl.h"
+
+#include "content/browser/geolocation/geolocation_provider_impl.h"
+#include "content/public/browser/content_browser_client.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_process_host.h"
+
+namespace content {
+
+namespace {
+
+PermissionType PermissionNameToPermissionType(PermissionName name) {
+  switch(name) {
+    case PERMISSION_NAME_GEOLOCATION:

[nasko, 2014/11/13 23:57:37]

Where is this defined? Is it auto-generated somehow?

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

Auto-generated from the .mojom file.

+      return PERMISSION_GEOLOCATION;
+  }
+
+  NOTREACHED();
+  return PERMISSION_NUM;

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

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]

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?

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

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.

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

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

+}
+
+} // anonymous namespace
+
+PermissionServiceImpl::PendingRequest::PendingRequest(PermissionType permission,
+                                                      const GURL& origin)
+    : permission(permission)
+    , origin(origin) {

[nasko, 2014/11/13 23:57:37]

nit: Chromium style puts commas at the end of the line.

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

Acknowledged.

+}
+
+void PermissionServiceImpl::Create(
+    scoped_ptr<PermissionServiceContext> context,
+    mojo::InterfaceRequest<PermissionService> request) {
+  BindToRequest(new PermissionServiceImpl(context.Pass()), &request);

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

nit: prefix with mojo:: to be clear that this isn't part of this class or the
content module.

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

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

+}
+
+PermissionServiceImpl::PermissionServiceImpl(
+    scoped_ptr<PermissionServiceContext> context)
+    : context_(context.Pass())
+    , weak_factory_(this) {
+  context_->set_service(this);
+}
+
+PermissionServiceImpl::~PermissionServiceImpl() {
+}
+
+void PermissionServiceImpl::RequestPermission(
+    PermissionPtr permission,

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

I assume PermissionPtr is a generated pointer type to the Permission struct in
the mojom file, correct?

+    const mojo::String& origin,
+    const mojo::Callback<void(PermissionStatus)>& callback) {
+  if (!context_->render_frame_host()) {

[nasko, 2014/11/13 23:57:37]

When is this valid?

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

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).

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

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.

+    // There is no way to show a UI so the call will simply return the current
+    // permission.
+    HasPermission(permission.Clone(), origin, callback);
+    return;
+  }
+
+  PermissionType permission_type =
+      PermissionNameToPermissionType(permission->name);
+  int request_id = pending_requests_.Add(
+      new PendingRequest(permission_type, GURL(origin)));
+
+  GetContentClient()->browser()->RequestPermission(
+      permission_type,
+      context_->web_contents(),
+      request_id,
+      GURL(origin),
+      true, // TODO(mlamouri): should be removed, see http://crbug.com/423770

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

Do we plan to remove user gesture indicator in general? Why isn't it present in
the Permission struct?

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

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.

+      base::Bind(&PermissionServiceImpl::OnRequestPermissionResponse,
+                 weak_factory_.GetWeakPtr(),
+                 callback,
+                 request_id));
+}
+
+void PermissionServiceImpl::OnRequestPermissionResponse(
+    const mojo::Callback<void(PermissionStatus)>& callback,
+    int request_id,
+    bool allowed) {
+  // TODO(mlamouri): we might want a generic way to handle those things.
+  if (pending_requests_.Lookup(request_id)->permission ==
+      PERMISSION_GEOLOCATION) {
+    GeolocationProviderImpl::GetInstance()->UserDidOptIntoLocationServices();

[nasko, 2014/11/13 23:57:37]

Why are we calling this without checking the value of "allowed"?

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

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

Will fix.

+  }
+
+  pending_requests_.Remove(request_id);
+
+  // TODO(mlamouri): for now, we only get a boolean back, but we would ideally
+  // need a ContentSetting, see http://crbug.com/432978
+  callback.Run(allowed ? PERMISSION_STATUS_GRANTED : PERMISSION_STATUS_ASK);
+}
+
+void PermissionServiceImpl::CancelPendingRequests() {
+  DCHECK(context_->web_contents());
+
+  for (RequestsMap::Iterator<PendingRequest> it(&pending_requests_);
+       !it.IsAtEnd(); it.Advance()) {
+    GetContentClient()->browser()->CancelPermissionRequest(
+        it.GetCurrentValue()->permission,
+        context_->web_contents(),
+        it.GetCurrentKey(),
+        it.GetCurrentValue()->origin);
+  }
+
+  pending_requests_.Clear();
+}
+
+void PermissionServiceImpl::RevokePermission(
+    PermissionPtr permission,
+    const mojo::String& origin,
+    const mojo::Callback<void(PermissionStatus)>& callback) {
+  NOTIMPLEMENTED();
+}
+
+void PermissionServiceImpl::HasPermission(
+    PermissionPtr permission,
+    const mojo::String& origin,
+    const mojo::Callback<void(PermissionStatus)>& callback) {
+  NOTIMPLEMENTED();
+}
+
+void PermissionServiceImpl::StartObserving(
+    PermissionPtr permission,
+    const mojo::String& origin) {
+  NOTIMPLEMENTED();
+}
+
+void PermissionServiceImpl::StopObserving(
+    PermissionPtr permission,
+    const mojo::String& origin) {
+  NOTIMPLEMENTED();
+}
+
+}  // namespace content