Group the different permission related methods in the content api.

android_webview/browser/aw_content_browser_client.cc

 // Copyright (c) 2012 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 "android_webview/browser/aw_content_browser_client.h"
 
 #include "android_webview/browser/aw_browser_context.h"
 #include "android_webview/browser/aw_browser_main_parts.h"
 #include "android_webview/browser/aw_browser_permission_request_delegate.h"
 #include "android_webview/browser/aw_contents_client_bridge_base.h"
@@ skipping 122 matching lines @@
   }
   virtual void SaveAccessToken(const GURL& server_url,
                                const base::string16& access_token) override { }
 
  private:
   virtual ~AwAccessTokenStore() { }
 
   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() {
   // Start with the currnet locale.
   std::string langs = l10n_util::GetDefaultLocale();
 
   // If we're not en-US, add in en-US which will be
   // used with a lower q-value.
   if (base::StringToLowerASCII(langs) != "en-us") {
     langs += ",en-US";
@@ skipping 231 matching lines @@
 }
 
 void AwContentBrowserClient::ShowDesktopNotification(
     const content::ShowDesktopNotificationHostMsgParams& params,
     content::RenderFrameHost* render_frame_host,
     scoped_ptr<content::DesktopNotificationDelegate> delegate,
     base::Closure* cancel_callback) {
   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,
     bool user_gesture,
     const base::Callback<void(bool)>& result_callback) {
   int render_process_id = web_contents->GetRenderProcessHost()->GetID();
   int render_view_id = web_contents->GetRenderViewHost()->GetRoutingID();
+  GURL origin = requesting_frame.GetOrigin();
+  AwBrowserPermissionRequestDelegate* delegate =
+      AwBrowserPermissionRequestDelegate::FromID(render_process_id,
+                                                 render_view_id);
+  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& 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 GeolocationPermission 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;
   }
-
-  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(
-    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) {
-  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);
-    return;
-  }
-
-  if (cancel_callback) {
-    *cancel_callback = base::Bind(
-        CancelProtectedMediaIdentifierPermissionRequests,
-        render_process_id, render_view_id, origin);
-  }
-  delegate->RequestProtectedMediaIdentifierPermission(origin, result_callback);
 }
 
 bool AwContentBrowserClient::CanCreateWindow(
     const GURL& opener_url,
     const GURL& opener_top_level_frame_url,
     const GURL& source_origin,
     WindowContainerType container_type,
     const GURL& target_url,
     const content::Referrer& referrer,
     WindowOpenDisposition disposition,
@@ skipping 92 matching lines @@
   return native_factory_->CreateExternalVideoSurfaceContainer(web_contents);
 }
 #endif
 
 content::DevToolsManagerDelegate*
 AwContentBrowserClient::GetDevToolsManagerDelegate() {
   return new AwDevToolsManagerDelegate();
 }
 
 }  // namespace android_webview