Group the different permission related methods in the content api.

android_webview/browser/aw_content_browser_client.cc

Index: android_webview/browser/aw_content_browser_client.cc
diff --git a/android_webview/browser/aw_content_browser_client.cc b/android_webview/browser/aw_content_browser_client.cc
index 6c0d6f503eb95fc15aec6cb2135670944d0b3972..7d2955ffd7418d972988b962ae9091924c653851 100644
--- a/android_webview/browser/aw_content_browser_client.cc
+++ b/android_webview/browser/aw_content_browser_client.cc
@@ -140,17 +140,6 @@ class AwAccessTokenStore : public content::AccessTokenStore {
   DISALLOW_COPY_AND_ASSIGN(AwAccessTokenStore);
 };
 
-void CancelProtectedMediaIdentifierPermissionRequests(
-    int render_process_id,
-    int render_view_id,
-    const GURL& origin) {
-  AwBrowserPermissionRequestDelegate* delegate =
-      AwBrowserPermissionRequestDelegate::FromID(render_process_id,
-                                                 render_view_id);
-  if (delegate)
-    delegate->CancelProtectedMediaIdentifierPermissionRequests(origin);
-}
-
 }  // namespace
 
 std::string AwContentBrowserClient::GetAcceptLangsImpl() {
@@ -402,7 +391,8 @@ void AwContentBrowserClient::ShowDesktopNotification(
   NOTREACHED() << "Android WebView does not support desktop notifications.";
 }
 
-void AwContentBrowserClient::RequestGeolocationPermission(
+void AwContentBrowserClient::RequestPermission(
+    content::PermissionType permission,
     content::WebContents* web_contents,
     int bridge_id,
     const GURL& requesting_frame,
@@ -410,66 +400,66 @@ void AwContentBrowserClient::RequestGeolocationPermission(
     const base::Callback<void(bool)>& result_callback) {
   int render_process_id = web_contents->GetRenderProcessHost()->GetID();
   int render_view_id = web_contents->GetRenderViewHost()->GetRoutingID();
-  AwBrowserPermissionRequestDelegate* delegate =
-      AwBrowserPermissionRequestDelegate::FromID(render_process_id,
-                                                 render_view_id);
-  if (delegate == NULL) {
-    DVLOG(0) << "Dropping GeolocationPermission request";
-    result_callback.Run(false);
-    return;
-  }
-
   GURL origin = requesting_frame.GetOrigin();
-  delegate->RequestGeolocationPermission(origin, result_callback);
-}
-
-void AwContentBrowserClient::CancelGeolocationPermissionRequest(
-    content::WebContents* web_contents,
-    int bridge_id,
-    const GURL& requesting_frame) {
-  int render_process_id = web_contents->GetRenderProcessHost()->GetID();
-  int render_view_id = web_contents->GetRenderViewHost()->GetRoutingID();
   AwBrowserPermissionRequestDelegate* delegate =
       AwBrowserPermissionRequestDelegate::FromID(render_process_id,
                                                  render_view_id);
-  if (delegate)
-    delegate->CancelGeolocationPermissionRequests(requesting_frame);
-}
-
-void AwContentBrowserClient::RequestMidiSysExPermission(
+  switch (permission) {
+    case content::PERMISSION_MIDI_SYSEX:
+      break;

[Bernhard Bauer, 2014/10/20 14:57:23]

Do you want to fall through here?

[Miguel Garcia, 2014/10/21 17:17:11]

Acknowledged.

+    case content::PERMISSION_NOTIFICATIONS:
+      LOG(WARNING) << "Permission Request not implemented for "
+      << permission;

[Bernhard Bauer, 2014/10/20 14:57:24]

Nit: Align << signs.

[Miguel Garcia, 2014/10/21 17:17:11]

Acknowledged.

+      break;

[Peter Beverloo, 2014/10/20 17:32:16]

Can we just handle these cases in the |default| clause of this switch, rather
than doing the fall-through trampoline? I also think we can be more strict,
since we shouldn't trigger a permission request at all if we know a feature
isn't going to be available.

[Miguel Garcia, 2014/10/21 17:17:11]

Acknowledged.

+    case content::PERMISSION_GEOLOCATION:
+      if (delegate == NULL) {

[Bernhard Bauer, 2014/10/20 14:57:24]

Nit: Just !delegate

[Miguel Garcia, 2014/10/21 17:17:11]

Done.

+        DVLOG(0) << "Dropping GeolocationPermission request";
+        result_callback.Run(false);
+        return;
+      }
+      delegate->RequestGeolocationPermission(origin, result_callback);
+      break;
+    case content::PERMISSION_PROTECTED_MEDIA:
+      if (delegate == NULL) {
+        DVLOG(0) << "Dropping ProtectedMediaIdentifierPermission request";
+        result_callback.Run(false);
+        return;
+      }
+      delegate->RequestProtectedMediaIdentifierPermission(
+          origin, result_callback);
+      break;
+    default:
+      LOG(WARNING) << "Permission not available " << permission;

[Bernhard Bauer, 2014/10/20 14:57:23]

Is there a difference between "Permission not available" and "Permission request
not implemented"?

Also, if you explicitly list the remaining enum values, the compiler will
complain if someone should add another value and forget updating this place.

[Peter Beverloo, 2014/10/20 17:32:16]

nit: break; after this line for consistency.

[Peter Beverloo, 2014/10/20 17:32:16]

+1 to merging them.

[Miguel Garcia, 2014/10/21 17:17:11]

I merged them and explicitly listed all values as requested.

On 2014/10/20 14:57:23, Bernhard Bauer wrote:

[Miguel Garcia, 2014/10/21 17:17:11]

Acknowledged.

[Miguel Garcia, 2014/10/21 17:17:11]

Acknowledged.

+   }
+}
+
+void AwContentBrowserClient::CancelPermissionRequest(
+    content::PermissionType permission,
     content::WebContents* web_contents,
     int bridge_id,
-    const GURL& requesting_frame,
-    bool user_gesture,
-    base::Callback<void(bool)> result_callback,
-    base::Closure* cancel_callback) {
-  // TODO(toyoshim): Android WebView is not supported yet.
-  // See http://crbug.com/339767.
-  result_callback.Run(false);
-}
-
-void AwContentBrowserClient::RequestProtectedMediaIdentifierPermission(
-    content::WebContents* web_contents,
-    const GURL& origin,
-    base::Callback<void(bool)> result_callback,
-    base::Closure* cancel_callback) {
+    const GURL& origin) {
   int render_process_id = web_contents->GetRenderProcessHost()->GetID();
   int render_view_id = web_contents->GetRenderViewHost()->GetRoutingID();
   AwBrowserPermissionRequestDelegate* delegate =
       AwBrowserPermissionRequestDelegate::FromID(render_process_id,
                                                  render_view_id);
-  if (delegate == NULL) {
-    DVLOG(0) << "Dropping ProtectedMediaIdentifierPermission request";
-    result_callback.Run(false);
+  if (!delegate)

[Peter Beverloo, 2014/10/20 17:32:16]

This check should be done in AwContentBrowserClient::RequestPermission as well.

Assuming we have a WebContents*, there should be an AwContents instance as well
(which is what AwBrowserPermissionRequestDelegate::FromID returns). Can we
DCHECK instead of doing an if-clause here?

[Miguel Garcia, 2014/10/21 17:17:11]

Actually in RequestPermission there are already checks for the delegate and the
behaviour is different since the callback is run and a Logging statement is
printed. It seems a bit out of scope to try to change this in the same cl.



On 2014/10/20 17:32:16, Peter Beverloo wrote:

     return;
+  switch (permission) {
+    case content::PERMISSION_MIDI_SYSEX:
+    case content::PERMISSION_NOTIFICATIONS:
+      LOG(WARNING) << "Cancel Permission request not implemented for "
+      << permission;
+      break;
+    case content::PERMISSION_GEOLOCATION:
+      delegate->CancelGeolocationPermissionRequests(origin);
+      break;
+    case content::PERMISSION_PROTECTED_MEDIA:
+      delegate->CancelProtectedMediaIdentifierPermissionRequests(origin);
+      break;
+    default:
+      LOG(WARNING) << "Permission not available " << permission;
   }
-
-  if (cancel_callback) {
-    *cancel_callback = base::Bind(
-        CancelProtectedMediaIdentifierPermissionRequests,
-        render_process_id, render_view_id, origin);
-  }
-  delegate->RequestProtectedMediaIdentifierPermission(origin, result_callback);
 }
 
 bool AwContentBrowserClient::CanCreateWindow(