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..424166644df4de27a5f3a176a79e4516ea52692a 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,82 @@ class LastRequestResultCache {
   DISALLOW_COPY_AND_ASSIGN(LastRequestResultCache);
 };
 
-struct AwPermissionManager::PendingRequest {
+class AwPermissionManager::PendingRequest {

[Tobias Sargeant, 2016/07/12 12:43:23]

I understand the desire to clean up, but this class is internal to this file,
and reordering arguments/initializers/members and adding a lot of getters
complicates the diff and doesn't appear to add to the readability. Would you
please consider trying to minimise the size of this change and do cleanup in a
different CL?

[Takashi Toyoshima, 2016/07/13 06:00:10]

My intention was to minimize difference between this AwPermissionManager and the
original PermissionManager (chrome/browser/permissions/permission_manager.cc).

Anyway, this is in android_webview/. As you said, keeping the original style
would make this file consistent to other WebView code. Let me revert unnecessary
changes.

  public:
-  PendingRequest(PermissionType permission,
+  PendingRequest(content::RenderFrameHost* render_frame_host,
+                 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)
+    : render_process_id_(render_frame_host->GetProcess()->GetID()),
+      render_frame_id_(render_frame_host->GetRoutingID()),
+      callback_(callback),
+      permissions_(permissions),
+      results_(permissions.size(), PermissionStatus::DENIED),
+      remaining_results_(permissions.size()),
+      requesting_origin_(requesting_origin),
+      embedding_origin_(embedding_origin) {}
 
   ~PendingRequest() = default;
 
-  PermissionType permission;
-  GURL requesting_origin;
-  GURL embedding_origin;
-  int render_process_id;
-  int render_frame_id;
-  base::Callback<void(PermissionStatus)> callback;
+  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) {
+        results_[i] = status;

[Tobias Sargeant, 2016/07/12 12:43:23]

Call SetPermissionStatus(i, status) here.

[Takashi Toyoshima, 2016/07/13 06:00:10]

Done.

+        --remaining_results_;
+      }
+    }
+  }
+
+  bool HasPermissionType(PermissionType type) {
+    for (auto permission : permissions_) {
+      if (permission == type)
+        return true;
+    }
+    return false;
+  }
+
+  bool IsComplete() const {
+    return remaining_results_ == 0;
+  }
+
+  int render_process_id() const { return render_process_id_; }
+  int render_frame_id() const { return render_frame_id_; }
+
+  const RequestPermissionsCallback callback()  const { return callback_; }
+
+  std::vector<PermissionType> permissions() const {
+    return permissions_;
+  }
+
+  std::vector<PermissionStatus> results() const {
+    return results_;
+  }
+
+  const GURL& requesting_origin() const {
+    return requesting_origin_;
+  }
+
+  const GURL& embedding_origin() const {
+    return embedding_origin_;
+  }
+
+ private:
+  int render_process_id_;
+  int render_frame_id_;
+  RequestPermissionsCallback callback_;
+  std::vector<PermissionType> permissions_;
+  std::vector<PermissionStatus> results_;
+  size_t remaining_results_;
+  GURL requesting_origin_;
+  GURL embedding_origin_;
 };
 
 AwPermissionManager::AwPermissionManager()
@@ -163,6 +230,10 @@ AwPermissionManager::AwPermissionManager()
 }
 
 AwPermissionManager::~AwPermissionManager() {
+  for (PendingRequestsMap::Iterator<PendingRequest> it(&pending_requests_);

[Takashi Toyoshima, 2016/07/13 06:00:11]

Should I call CancelPermissionRequest too?

+       !it.IsAtEnd(); it.Advance()) {
+    it.GetCurrentValue()->callback().Run(it.GetCurrentValue()->results());
+  }
 }
 
 int AwPermissionManager::RequestPermission(
@@ -171,147 +242,150 @@ 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(
+        render_frame_host,
+        permissions,
+        requesting_origin,
+        embedding_origin,
+        callback);
+  std::vector<bool> should_delegate_requests =
+      std::vector<bool>(permissions.size());
+  for (size_t i = 0; i < permissions.size(); ++i) {
+    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?
+        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) {
+                       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) {
+                       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;
+                       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:

[Tobias Sargeant, 2016/07/12 12:43:23]

After your change, |callback| will not be called until something calls
OnRequestResponse, which might not happen in a timely fashion (or at all).

Maybe you could only add pending_request to pending_requests_ at the end of the
function, if it's not already complete (and otherwise call the callback). This
would also make it easier to make pending_request a unique_ptr.

[Takashi Toyoshima, 2016/07/13 06:00:10]

Thank you for catching this bug.

request_id is needed to call delegate functions, and it could be obtained by
pending_requests_.Add(pending_request).

So probably, I should check IsComplete() after this for-loop, then kick the
callback if it is needed.

By the way, chrome's PermissionManager also cares for the case the callback was
invoked synchronously and pending_request was removed from the
pending_request_at the end of the for-loop. I'd also add the same check just in
case.

+        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();
-
-  for (PermissionType type : permissions) {
-    result.push_back(GetPermissionStatus(
-        type, requesting_origin, embedding_origin));
-  }
-
-  callback.Run(result);
-  return kNoPendingOperation;
-}
-
 // 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())
+    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,63 +393,59 @@ 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();
+  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(
-          pending_request->render_process_id,
-          pending_request->render_frame_id);
+          pending_request->render_process_id(),
+          pending_request->render_frame_id());
   if (!delegate) {
     pending_requests_.Remove(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()) {
+    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);

[Tobias Sargeant, 2016/07/12 12:43:23]

Should this call be outside the loop?

[Takashi Toyoshima, 2016/07/13 06:00:10]

Right. Thank you for catching this bug too.

   }
-
-  pending_requests_.Remove(request_id);
 }
 
 void AwPermissionManager::ResetPermission(PermissionType permission,