Implement AwPermissionManager::RequestPermissions

android_webview/browser/aw_permission_manager.cc

Index: android_webview/browser/aw_permission_manager.cc
diff --git a/android_webview/browser/aw_permission_manager.cc b/android_webview/browser/aw_permission_manager.cc
index a428250f940152f3d242bc52f2892e37674f6cf7..572ff3d294c3d0e113f4699a8a972e32e3315a0a 100644
--- a/android_webview/browser/aw_permission_manager.cc
+++ b/android_webview/browser/aw_permission_manager.cc
@@ -18,8 +18,22 @@
 using blink::mojom::PermissionStatus;
 using content::PermissionType;
 
+using RequestPermissionsCallback =
+    base::Callback<void(const std::vector<PermissionStatus>&)>;
+
 namespace android_webview {
 
+namespace {
+
+void PermissionRequestResponseCallbackWrapper(
+    const base::Callback<void(PermissionStatus)>& callback,
+    const std::vector<PermissionStatus>& vector) {
+  DCHECK_EQ(vector.size(), 1ul);
+  callback.Run(vector[0]);
+}
+
+}  // namespace
+
 class LastRequestResultCache {
  public:
   LastRequestResultCache() = default;
@@ -131,29 +145,58 @@ class LastRequestResultCache {
   DISALLOW_COPY_AND_ASSIGN(LastRequestResultCache);
 };
 
-struct AwPermissionManager::PendingRequest {
+class AwPermissionManager::PendingRequest {
  public:
-  PendingRequest(PermissionType permission,
+  PendingRequest(const std::vector<PermissionType> permissions,
                  GURL requesting_origin,
                  GURL embedding_origin,
                  content::RenderFrameHost* render_frame_host,
-                 const base::Callback<void(PermissionStatus)>& callback)
-    : permission(permission),
-      requesting_origin(requesting_origin),
-      embedding_origin(embedding_origin),
-      render_process_id(render_frame_host->GetProcess()->GetID()),
-      render_frame_id(render_frame_host->GetRoutingID()),
-      callback(callback) {
-  }
+                 const RequestPermissionsCallback callback)
+      : permissions(permissions),
+        requesting_origin(requesting_origin),
+        embedding_origin(embedding_origin),
+        render_process_id(render_frame_host->GetProcess()->GetID()),
+        render_frame_id(render_frame_host->GetRoutingID()),
+        callback(callback),
+        results(permissions.size(), PermissionStatus::DENIED),
+        remaining_results_(permissions.size()) {}
 
   ~PendingRequest() = default;
 
-  PermissionType permission;
+  void SetPermissionStatus(int index, PermissionStatus status) {
+    DCHECK(!IsComplete());
+
+    results[index] = status;
+    --remaining_results_;
+  }
+
+  void SetPermissionStatus(PermissionType type, PermissionStatus status) {
+    for (size_t i = 0; i < permissions.size(); ++i) {
+      if (permissions[i] == type)
+        SetPermissionStatus(i, status);
+    }
+  }
+
+  bool HasPermissionType(PermissionType type) {
+    for (auto permission : permissions) {
+      if (permission == type)
+        return true;
+    }
+    return false;
+  }
+
+  bool IsComplete() const { return remaining_results_ == 0; }
+
+  std::vector<PermissionType> permissions;
   GURL requesting_origin;
   GURL embedding_origin;
   int render_process_id;
   int render_frame_id;
-  base::Callback<void(PermissionStatus)> callback;
+  RequestPermissionsCallback callback;
+  std::vector<PermissionStatus> results;
+
+ private:
+  size_t remaining_results_;
 };
 
 AwPermissionManager::AwPermissionManager()
@@ -163,6 +206,21 @@ AwPermissionManager::AwPermissionManager()
 }
 
 AwPermissionManager::~AwPermissionManager() {
+  std::vector<int> request_ids;
+  for (PendingRequestsMap::Iterator<PendingRequest> it(&pending_requests_);
+       !it.IsAtEnd(); it.Advance()) {
+    request_ids.push_back(it.GetCurrentKey());
+  }
+  for (auto request_id : request_ids) {
+    PendingRequest* request = pending_requests_.Lookup(request_id);
+    if (!request)
+      continue;
+    const RequestPermissionsCallback callback = request->callback;
+    std::vector<PermissionStatus> results = request->results;
+    CancelPermissionRequest(request_id);
+    callback.Run(results);
+  }
+  DCHECK(pending_requests_.IsEmpty());
 }
 
 int AwPermissionManager::RequestPermission(
@@ -171,147 +229,159 @@ int AwPermissionManager::RequestPermission(
     const GURL& requesting_origin,
     bool user_gesture,
     const base::Callback<void(PermissionStatus)>& callback) {
+  return RequestPermissions(
+      std::vector<PermissionType>(1, permission), render_frame_host,
+      requesting_origin, user_gesture,
+      base::Bind(&PermissionRequestResponseCallbackWrapper, callback));
+}
+
+int AwPermissionManager::RequestPermissions(
+    const std::vector<PermissionType>& permissions,
+    content::RenderFrameHost* render_frame_host,
+    const GURL& requesting_origin,
+    bool user_gesture,
+    const base::Callback<void(const std::vector<PermissionStatus>&)>&
+        callback) {
+  if (permissions.empty()) {
+    callback.Run(std::vector<PermissionStatus>());
+    return kNoPendingOperation;
+  }
+
   int render_process_id = render_frame_host->GetProcess()->GetID();
   int render_frame_id = render_frame_host->GetRoutingID();
   AwBrowserPermissionRequestDelegate* delegate =
       AwBrowserPermissionRequestDelegate::FromID(render_process_id,
                                                  render_frame_id);
   if (!delegate) {
-    DVLOG(0) << "Dropping permission request for "
-             << static_cast<int>(permission);
-    callback.Run(PermissionStatus::DENIED);
+    DVLOG(0) << "Dropping permissions request";
+    callback.Run(std::vector<PermissionStatus>(permissions.size(),
+                                               PermissionStatus::DENIED));
     return kNoPendingOperation;
   }
 
-  // Do not delegate any requests which are already pending.
-  bool should_delegate_request = true;
-  for (PendingRequestsMap::Iterator<PendingRequest> it(&pending_requests_);
-       !it.IsAtEnd(); it.Advance()) {
-    if (permission == it.GetCurrentValue()->permission) {
-      should_delegate_request = false;
-      break;
-    }
-  }
-
   const GURL& embedding_origin =
       content::WebContents::FromRenderFrameHost(render_frame_host)
           ->GetLastCommittedURL().GetOrigin();
 
-  int request_id = kNoPendingOperation;
-  switch (permission) {
-    case PermissionType::GEOLOCATION:
-      request_id = pending_requests_.Add(new PendingRequest(
-          permission, requesting_origin,
-          embedding_origin, render_frame_host,
-          callback));
-      if (should_delegate_request) {
+  PendingRequest* pending_request =
+      new PendingRequest(permissions, requesting_origin, embedding_origin,
+                         render_frame_host, callback);
+  std::vector<bool> should_delegate_requests =
+      std::vector<bool>(permissions.size());
+  for (size_t i = 0; i < permissions.size(); ++i) {

[Tobias Sargeant, 2016/07/13 11:04:27]

I think there's another issue here:

Imagine you have a prior pending request for two permissions, and one has
already returned via OnRequestResponse, but the second hasn't. Then you request
a single permission equal to the one that has been completed.

You would correctly not kick off a new request, but it would never be completed
because the OnRequestResponse that completes the first request would not
complete the second request.

I think maybe what you should do here is subsequently check if
i.GetCurrentValue().IsComplete(permission_type) [would have to add this] and if
so, directly set the permission status for this request to the result from the
old request.

[Takashi Toyoshima, 2016/07/14 06:42:52]

Oh, thank you again.
That's the case I overlooked.

+    bool should_delegate_request = true;
+    for (PendingRequestsMap::Iterator<PendingRequest> it(&pending_requests_);
+         !it.IsAtEnd(); it.Advance()) {
+      if (it.GetCurrentValue()->HasPermissionType(permissions[i])) {
+        // TODO(toyoshim): Shall we check to match requesting_origin too?

[Takashi Toyoshima, 2016/07/14 06:42:52]

I would like to revisit this TODO now to understand the original code well.

The original RequestPermission() also does not check requesting_origin in the
equivalent check, but delegate->Request*Permission() looks per-requesting_origin
manner. Was this a problem?

If I understand correctly, the very right implementation should check
requesting_origin here, and if it is different, we should enqueue this request
so that we can call delegate->Request*Permission() function after the previous
call for the same permission type is resolved. That will make the PendingRequest
handling a bit complicated.

Another curious truth is that result_cache_ cares both requesting_origin and
embedding_origin. But probably this is because ResetPermission() and
GetPermissionStatus() interfaces require to aware both origins.

[Tobias Sargeant, 2016/07/14 09:24:29]

My feeling is that it's more consistent to check the requesting_origin. As you
point out, it's an argument to the delegate methods, and as such if we avoid it
we are assuming that the response for any origin is the same. However, sadly the
Java API doesn't pass this information on, so there's nothing that an app can
use to distinguish the two responses. Unfortunately the WebView API is just
broken in this regard.

It's probably not going to break any tests to make that change, but I don't have
a strong intuition about how this will affect apps in the wild.

torne@ should probably make the call about whether to be correct here and
potentially cause more permission requests to the app, or stick with the old
behaviour.

[Torne, 2016/07/14 10:02:38]

The Java code does pass the requesting origin up, just not the embedding origin.
We discussed this in person and agree that we should check the requesting
origin, and the previous code was broken.

[Takashi Toyoshima, 2016/07/14 12:40:49]

OK, let me add requesting_origin check here, and other related places.

+        should_delegate_request = false;
+        break;
+      }
+    }
+    should_delegate_requests[i] = should_delegate_request;
+  }
+
+  int request_id = pending_requests_.Add(pending_request);
+
+  for (size_t i = 0; i < permissions.size(); ++i) {
+    if (!should_delegate_requests[i])
+      continue;
+
+    switch (permissions[i]) {
+      case PermissionType::GEOLOCATION:
         delegate->RequestGeolocationPermission(
             requesting_origin,
-            base::Bind(&OnRequestResponse,
-                       weak_ptr_factory_.GetWeakPtr(), request_id,
-                       callback));
-      }
-      break;
-    case PermissionType::PROTECTED_MEDIA_IDENTIFIER:
-      request_id = pending_requests_.Add(new PendingRequest(
-          permission, requesting_origin,
-          embedding_origin, render_frame_host,
-          callback));
-      if (should_delegate_request) {
+            base::Bind(&OnRequestResponse, weak_ptr_factory_.GetWeakPtr(),
+                       request_id, i));
+        break;
+      case PermissionType::PROTECTED_MEDIA_IDENTIFIER:
         delegate->RequestProtectedMediaIdentifierPermission(
             requesting_origin,
-            base::Bind(&OnRequestResponse,
-                       weak_ptr_factory_.GetWeakPtr(), request_id,
-                       callback));
-      }
-      break;
-    case PermissionType::MIDI_SYSEX:
-      request_id = pending_requests_.Add(new PendingRequest(
-          permission, requesting_origin,
-          embedding_origin, render_frame_host,
-          callback));
-      if (should_delegate_request) {
+            base::Bind(&OnRequestResponse, weak_ptr_factory_.GetWeakPtr(),
+                       request_id, i));
+        break;
+      case PermissionType::MIDI_SYSEX:
         delegate->RequestMIDISysexPermission(
             requesting_origin,
-            base::Bind(&OnRequestResponse,
-                       weak_ptr_factory_.GetWeakPtr(), request_id,
-                       callback));
-      }
-      break;
-    case PermissionType::AUDIO_CAPTURE:
-    case PermissionType::VIDEO_CAPTURE:
-    case PermissionType::NOTIFICATIONS:
-    case PermissionType::PUSH_MESSAGING:
-    case PermissionType::DURABLE_STORAGE:
-    case PermissionType::BACKGROUND_SYNC:
-      NOTIMPLEMENTED() << "RequestPermission is not implemented for "
-                       << static_cast<int>(permission);
-      callback.Run(PermissionStatus::DENIED);
-      break;
-    case PermissionType::MIDI:
-      callback.Run(PermissionStatus::GRANTED);
-      break;
-    case PermissionType::NUM:
-      NOTREACHED() << "PermissionType::NUM was not expected here.";
-      callback.Run(PermissionStatus::DENIED);
-      break;
+            base::Bind(&OnRequestResponse, weak_ptr_factory_.GetWeakPtr(),
+                       request_id, i));
+        break;
+      case PermissionType::AUDIO_CAPTURE:
+      case PermissionType::VIDEO_CAPTURE:
+      case PermissionType::NOTIFICATIONS:
+      case PermissionType::PUSH_MESSAGING:
+      case PermissionType::DURABLE_STORAGE:
+      case PermissionType::BACKGROUND_SYNC:
+        NOTIMPLEMENTED() << "RequestPermissions is not implemented for "
+                         << static_cast<int>(permissions[i]);
+        pending_request->SetPermissionStatus(i, PermissionStatus::DENIED);
+        break;
+      case PermissionType::MIDI:
+        pending_request->SetPermissionStatus(i, PermissionStatus::GRANTED);
+        break;
+      case PermissionType::NUM:
+        NOTREACHED() << "PermissionType::NUM was not expected here.";
+        pending_request->SetPermissionStatus(i, PermissionStatus::DENIED);
+        break;
+    }
   }
-  return request_id;
-}
-
-int AwPermissionManager::RequestPermissions(
-    const std::vector<PermissionType>& permissions,
-    content::RenderFrameHost* render_frame_host,
-    const GURL& requesting_origin,
-    bool user_gesture,
-    const base::Callback<void(
-        const std::vector<PermissionStatus>&)>& callback) {
-  NOTIMPLEMENTED() << "RequestPermissions has not been implemented in WebView";
 
-  std::vector<PermissionStatus> result(permissions.size());
-  const GURL& embedding_origin =
-      content::WebContents::FromRenderFrameHost(render_frame_host)
-          ->GetLastCommittedURL().GetOrigin();
+  // If delegate resolve the permission synchronously, all requests could be
+  // already resolved here.
+  if (!pending_requests_.Lookup(request_id))
+    return kNoPendingOperation;
 
-  for (PermissionType type : permissions) {
-    result.push_back(GetPermissionStatus(
-        type, requesting_origin, embedding_origin));
+  // If requests are resolved without calling delegate functions, e.g.
+  // PermissionType::MIDI is permitted within the previous for-loop, all
+  // requests could be already resolved, but still in the |pending_requests_|
+  // without invoking the callback.
+  if (pending_request->IsComplete()) {
+    std::vector<PermissionStatus> results = pending_request->results;
+    pending_requests_.Remove(request_id);
+    callback.Run(results);
+    return kNoPendingOperation;
   }
 
-  callback.Run(result);
-  return kNoPendingOperation;
+  return request_id;
 }
 
 // static
 void AwPermissionManager::OnRequestResponse(
     const base::WeakPtr<AwPermissionManager>& manager,
     int request_id,
-    const base::Callback<void(PermissionStatus)>& callback,
+    int request_index,
     bool allowed) {
+  if (!manager.get())

[Tobias Sargeant, 2016/07/13 11:04:27]

Previously the callback would be run even if the manager had been gc'ed. I can't
tell if this is a problem in practice.

[Takashi Toyoshima, 2016/07/14 06:42:52]

So, how about changing this check to DCHECK with some comments?

[Tobias Sargeant, 2016/07/14 09:24:30]

Good.

+    return;
+
   PermissionStatus status =
       allowed ? PermissionStatus::GRANTED : PermissionStatus::DENIED;
-  if (manager.get()) {
-    PendingRequest* pending_request =
-        manager->pending_requests_.Lookup(request_id);
-
-    for (PendingRequestsMap::Iterator<PendingRequest> it(
-            &manager->pending_requests_);
-         !it.IsAtEnd(); it.Advance()) {
-      if (pending_request->permission == it.GetCurrentValue()->permission &&
-          it.GetCurrentKey() != request_id) {
-        it.GetCurrentValue()->callback.Run(status);
-        manager->pending_requests_.Remove(it.GetCurrentKey());
-      }
+  PendingRequest* pending_request =
+      manager->pending_requests_.Lookup(request_id);
+  PermissionType permission_type = pending_request->permissions[request_index];
+
+  manager->result_cache_->SetResult(permission_type,
+                                    pending_request->requesting_origin,
+                                    pending_request->embedding_origin, status);
+
+  std::vector<int> complete_request_ids;
+  std::vector<std::pair<const RequestPermissionsCallback,
+                        std::vector<PermissionStatus>>>
+      complete_request_pairs;
+  for (PendingRequestsMap::Iterator<PendingRequest> it(
+           &manager->pending_requests_);
+       !it.IsAtEnd(); it.Advance()) {
+    it.GetCurrentValue()->SetPermissionStatus(permission_type, status);
+    if (it.GetCurrentValue()->IsComplete()) {
+      complete_request_ids.push_back(it.GetCurrentKey());
+      complete_request_pairs.push_back(std::make_pair(
+          it.GetCurrentValue()->callback, it.GetCurrentValue()->results));
     }
-
-    manager->result_cache_->SetResult(
-        pending_request->permission,
-        pending_request->requesting_origin,
-        pending_request->embedding_origin,
-        status);
-    manager->pending_requests_.Remove(request_id);
   }
-  callback.Run(status);
+  for (auto id : complete_request_ids)
+    manager->pending_requests_.Remove(id);
+  for (auto pair : complete_request_pairs)
+    pair.first.Run(pair.second);
 }
 
 void AwPermissionManager::CancelPermissionRequest(int request_id) {
@@ -319,22 +389,14 @@ void AwPermissionManager::CancelPermissionRequest(int request_id) {
   if (!pending_request)
     return;
 
-  content::RenderFrameHost* render_frame_host =
-      content::RenderFrameHost::FromID(pending_request->render_process_id,
-          pending_request->render_frame_id);
-  content::WebContents* web_contents =
-      content::WebContents::FromRenderFrameHost(render_frame_host);
-  DCHECK(web_contents);
-
   // The caller is canceling (presumably) the most recent request. Assuming the
   // request did not complete, the user did not respond to the requset.
   // Thus, assume we do not know the result.
-  const GURL& embedding_origin = web_contents
-          ->GetLastCommittedURL().GetOrigin();
-  result_cache_->ClearResult(
-      pending_request->permission,
-      pending_request->requesting_origin,
-      embedding_origin);
+  const GURL& embedding_origin = pending_request->embedding_origin;

[Tobias Sargeant, 2016/07/13 11:04:27]

Is the change to how embedding_origin is calculated significant? I presume that
this code could have used pending_request->embedding_origin previously.

[Takashi Toyoshima, 2016/07/14 06:42:51]

The original code calculate the embedding_origin from WebContents again. But
could it make a different result? I just reuse cached embedding_origin here as
we do in other places. The embedding_origin in the pending_request object was
calculated through completely same functions when it was added.

Probably, we could remove comments above because we do not need any assumption
here. We can pickup right information for permission type and origins from the
request_id.

And, actually, the embedding_origin is used only for |result_cache_| query. In
my understanding, the origin pair should be the same with the one we used in
RequestPermissions(). So, if recalculation could make a different result, using
cached value is correct idea.

[Tobias Sargeant, 2016/07/14 09:24:30]

Good point about the cache.

+  const GURL& requesting_origin = pending_request->requesting_origin;
+  for (auto permission : pending_request->permissions) {
+    result_cache_->ClearResult(permission, requesting_origin, embedding_origin);
+  }
 
   AwBrowserPermissionRequestDelegate* delegate =
       AwBrowserPermissionRequestDelegate::FromID(
@@ -345,36 +407,35 @@ void AwPermissionManager::CancelPermissionRequest(int request_id) {
     return;
   }
 
-  switch (pending_request->permission) {
-    case PermissionType::GEOLOCATION:
-      delegate->CancelGeolocationPermissionRequests(
-          pending_request->requesting_origin);
-      break;
-    case PermissionType::PROTECTED_MEDIA_IDENTIFIER:
-      delegate->CancelProtectedMediaIdentifierPermissionRequests(
-          pending_request->requesting_origin);
-      break;
-    case PermissionType::MIDI_SYSEX:
-      delegate->CancelMIDISysexPermissionRequests(
-          pending_request->requesting_origin);
-      break;
-    case PermissionType::NOTIFICATIONS:
-    case PermissionType::PUSH_MESSAGING:
-    case PermissionType::DURABLE_STORAGE:
-    case PermissionType::AUDIO_CAPTURE:
-    case PermissionType::VIDEO_CAPTURE:
-    case PermissionType::BACKGROUND_SYNC:
-      NOTIMPLEMENTED() << "CancelPermission not implemented for "
-                       << static_cast<int>(pending_request->permission);
-      break;
-    case PermissionType::MIDI:
-      // There is nothing to cancel so this is simply ignored.
-      break;
-    case PermissionType::NUM:
-      NOTREACHED() << "PermissionType::NUM was not expected here.";
-      break;
+  for (auto permission : pending_request->permissions) {

[Tobias Sargeant, 2016/07/13 11:04:27]

Here, you could end up sending a cancel for a request that has completed. I
guess you could use the same IsComplete(permission_type) to avoid that if
necessary.

[Takashi Toyoshima, 2016/07/14 06:42:52]

Also I notice another issue.

We can call the actual delegate Cancel method only when the pending_request is
the only one that requests this type of permission.

I'd check all pending_request_ for each permission, and will need to introduce
additional flag to PendingRequest class to mark the request is cancelled.

+    switch (permission) {
+      case PermissionType::GEOLOCATION:
+        delegate->CancelGeolocationPermissionRequests(requesting_origin);
+        break;
+      case PermissionType::PROTECTED_MEDIA_IDENTIFIER:
+        delegate->CancelProtectedMediaIdentifierPermissionRequests(
+            requesting_origin);
+        break;
+      case PermissionType::MIDI_SYSEX:
+        delegate->CancelMIDISysexPermissionRequests(requesting_origin);
+        break;
+      case PermissionType::NOTIFICATIONS:
+      case PermissionType::PUSH_MESSAGING:
+      case PermissionType::DURABLE_STORAGE:
+      case PermissionType::AUDIO_CAPTURE:
+      case PermissionType::VIDEO_CAPTURE:
+      case PermissionType::BACKGROUND_SYNC:
+        NOTIMPLEMENTED() << "CancelPermission not implemented for "
+                         << static_cast<int>(permission);
+        break;
+      case PermissionType::MIDI:
+        // There is nothing to cancel so this is simply ignored.
+        break;
+      case PermissionType::NUM:
+        NOTREACHED() << "PermissionType::NUM was not expected here.";
+        break;
+    }
   }
-
   pending_requests_.Remove(request_id);
 }