permissions: handle request ids for permissions in permission manager

content/public/browser/permission_manager.h

Index: content/public/browser/permission_manager.h
diff --git a/content/public/browser/permission_manager.h b/content/public/browser/permission_manager.h
index 5a2e078718dab32004594a0166f442b26335da50..4f5847f2c004ee3f664da3f8cce575cb307ae1de 100644
--- a/content/public/browser/permission_manager.h
+++ b/content/public/browser/permission_manager.h
@@ -22,24 +22,23 @@ class CONTENT_EXPORT PermissionManager {
   virtual ~PermissionManager() = default;
 
   // Requests a permission on behalf of a frame identified by render_frame_host.
-  // The |request_id| is an identifier that can later be used if the request is
-  // cancelled (see CancelPermissionRequest).
   // When the permission request is handled, whether it failed, timed out or
   // succeeded, the |callback| will be run.
-  virtual void RequestPermission(
+  // Returns a request id which can be used to cancel the permission (see
+  // CancelPermissionRequest). This can be kCancelUnsubscribeNoOp if there is
+  // no further need to cancel the permission but |callback| MUST have been

[mlamouri (slow - plz ping), 2015/09/16 14:42:57]

nit: drop "but", it sounds like the caller has to worry about that. Maybe:
"// no further need to cancel the permission in which case |callback| was
invoked."

[Lalit Maganti, 2015/09/16 16:41:10]

Done.

+  // invoked in this case.
+  virtual int RequestPermission(
       PermissionType permission,
       RenderFrameHost* render_frame_host,
-      int request_id,
       const GURL& requesting_origin,
       bool user_gesture,
       const base::Callback<void(PermissionStatus)>& callback) = 0;
 
-  // Cancels a previously requested permission. The given parameter must match
-  // the ones passed to the RequestPermission call.
-  virtual void CancelPermissionRequest(PermissionType permission,
-                                       RenderFrameHost* render_frame_host,
-                                       int request_id,
-                                       const GURL& requesting_origin) = 0;
+  // Cancels a previous permission request specified by |request_id|. Cancelling
+  // an already cancelled request or providing the |request_id|
+  // kCancelUnsubscribeNoOp is a no-op.
+  virtual void CancelPermissionRequest(int request_id) = 0;
 
   // Returns the permission status of a given requesting_origin/embedding_origin
   // tuple. This is not taking a RenderFrameHost because the call might happen
@@ -63,7 +62,8 @@ class CONTENT_EXPORT PermissionManager {
 
   // Runs the given |callback| whenever the |permission| associated with the
   // pair { requesting_origin, embedding_origin } changes.
-  // Returns the subscription_id to be used to unsubscribe.
+  // Returns the subscription_id to be used to unsubscribe. Can be
+  // kCancelUnsubscribeNoOp if the subscribe was not successful.
   virtual int SubscribePermissionStatusChange(
       PermissionType permission,
       const GURL& requesting_origin,
@@ -72,8 +72,14 @@ class CONTENT_EXPORT PermissionManager {
 
   // Unregisters from permission status change notifications.
   // The |subscription_id| must match the value returned by the
-  // SubscribePermissionStatusChange call.
+  // SubscribePermissionStatusChange call. Unsubscribing
+  // an already unsubscribed |subscription_id| or providing the
+  // |subscription_id| kCancelUnsubscribeNoOp is a no-op.
   virtual void UnsubscribePermissionStatusChange(int subscription_id) = 0;
+
+  // Constant retured when registering and subscribing if
+  // cancelling/unsubscribing at a later stage would have no effect.
+  static const int kCancelUnsubscribeNoOp = -1;

[mlamouri (slow - plz ping), 2015/09/16 14:42:57]

style: kFooBar isn't right per coding style.
nit: I think the name should be NO_PENDING_REQUEST.

[Lalit Maganti, 2015/09/16 16:41:10]

style: Which style guide is this? I had a look at the Google code style and it
says to use kConstantName style and the Chromium one says use CONSTANT_NAME for
enums to keep consistency. Also in the past (like theme color invalid constant)
I did use this style. 
nit: This constant is also used for subscription though so I guess
kNoPendingRequestOrSubscription would be more correct?

 };
 
 }  // namespace content