Handle media access permission request

android_webview/native/permission/aw_permission_request.h

+// Copyright 2014 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.
+
+#ifndef ANDROID_WEBVIEW_NATIVE_PERMISSION_AW_PERMISSION_REQUEST_H
+#define ANDROID_WEBVIEW_NATIVE_PERMISSION_AW_PERMISSION_REQUEST_H
+
+#include "base/android/jni_helper.h"
+#include "base/android/scoped_java_ref.h"
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "url/gurl.h"
+
+namespace android_webview {
+
+class AwPermissionRequestDelegate;
+
+// This class wraps a permission request, it works with PermissionRequestHandler
+// and its' Java peer to represent the request to AwContentsClient.
+// The specific permission request should implement the
+// AwPermissionRequestDelegate interface, See MediaPermissionRequest.
+class AwPermissionRequest : public base::RefCounted<AwPermissionRequest> {

[mkosiba (inactive), 2014/04/17 10:34:06]

do these need to be refcounted? could PermissionRequestHandler store weak
pointers instead?

[michaelbai, 2014/04/22 20:53:51]

Yes, PermissionRequestHandler will hold the AwPermissionRequest when it sent to
the client; when the request result is sent back from java side,
AwPermissionRequest will call back into PermissionRequestHandler which remove
the AwPermissionRequest from its list, at that time we still need
AwPermissionRequest to hold itself.

On 2014/04/17 10:34:06, mkosiba wrote:

[mkosiba (inactive), 2014/04/23 18:04:40]

Why does the AwPermissionRequest need to "hold itself"? After the embedder calls
either grant() or deny() the "connection" between the Java object and the C++
object is broken and therefore we don't need a AwPermissionRequest anymore,
right?

Refcounting mostly makes sense when we don't know who will delete the object. I
don't think that's the case here. Yo me it seems to me like the memory ownership
can be expressed using scoped + weak pointers.

If the Java class own the C++ instance for the duration of the callback, won't
that be sufficient? You'd transfer ownership of the C++ class to the Java class
when you create it (and send it to the client) and get ownership back when the
request is sent back from the Java side (embedder calls grant/deny or class is
GC'd).
PermissionRequestHandler would hold raw pointers to AwPermissionRequests - it
doesn't look like it's possible for a AwPermissionRequest to 'go away' without
notifying the PermissionRequestHandler, right?

[michaelbai, 2014/04/24 00:56:43]

Sorry, I didn't explain it clearly in the first hand. yes, according current
implementation the refcount isn't necessary. Actually, I don't want to define
who will own the AwPermissionRequest, it gives other permission implementation
flexibility to hold the instance of AwPermissionRequest.

Also, as the AwPermissionRequest could be freed when
- Java side(embedder) processed request.
- or, request has been canceled from native side.
- or, web content goes away.
- or, specific permission implementation want to free it.

I think it is simple to make it refcounting.

On 2014/04/23 18:04:40, mkosiba wrote:

[mkosiba (inactive), 2014/04/24 09:24:58]

I don't quite understand how having a new permission type (that's what
you mean when you say implementation, right?) changes things.
New permission types should only require a new AwPermissionRequestDelegate
subclasses and those don't need to know about AwPermissionRequest.

As a rule of thumb I'd prefer simpler object lifetime over added flexibility
"just in case". It's not only me, from
http://www.chromium.org/developers/coding-style/important-abstractions-and-da...
:

"Reference counting is occasionally useful but is more often a sign that someone
isn't thinking carefully about ownership. Use it when ownership is truly shared
(for example, multiple tabs sharing the same renderer process), not for when
lifetime management is difficult to reason about."
sure, but in all of these cases you simply delete the instance. In which of
these cases do you need to destroy the Java object but keep around a
AwPermissionRequest?
Even the class description implies that the AwPermissionRequest C++ instance
is only useful when it's coupled to a Java object,
The code is simpler to write, yes. Reasoning about object lifespan and whether
there
are no memory leaks is much harder though. It took me a while to convince myself
that this code doesn't leak refs. The next person looking at this will have to
go
through the same exercise.

I don't know what changes you're planning, they may very well justify
refcounting,
but given the amount of info currently available to me it doesn't seem like the
right
choice.

[michaelbai, 2014/04/25 01:06:09]

The AwPermissionRequestDelegate is owned by AwPermissionRequest, the good things
for MediaAccessPermissionRequest implementation is all the data needed can be
copied, so it doesn't need to use delegate after NotifyRequestResult() called. I
don't know for other case and might be overwhelmed by the concern here. 

Well, according current implementation, it not necessary to have it RefCounted,
will add it back if it is really needed in the future.
It isn't such confusing, as long as you use scoped_refptr, there is nothing you
need worried about.

[mkosiba (inactive), 2014/04/28 15:35:36]

Thanks!

+ public:
+  // The definition must synced with Android's
+  // android.webkit.PermissionRequest.java.

[mkosiba (inactive), 2014/04/17 10:34:06]

nit: android.webkit.PermissionRequest (drop .java)

I'm not sold on the idea of syncing constants between Chrome and Android
repositories 'by hand'. I spoke about this with Torne and Primiano a bit and
there doesn't seem to be an obvious good solution.

[michaelbai, 2014/04/22 20:53:51]

Right, there is no good solution here.

On 2014/04/17 10:34:06, mkosiba wrote:

+  enum Resource {
+    GeoLocation = 1 << 0,

[benm (inactive), 2014/04/16 14:51:10]

nit s/L/l/

[michaelbai, 2014/04/22 20:53:51]

Done.

+    VideoCapture = 1 << 1,
+    AudioCapture = 1 << 2
+  };
+
+  // Take the ownership of |delegate|.
+  AwPermissionRequest(AwPermissionRequestDelegate* delegate);

[mkosiba (inactive), 2014/04/17 10:34:06]

if takes ownership then should take scoped_ptr<AwPermissionRequestDelegate>
instead of raw pointer.

[michaelbai, 2014/04/22 20:53:51]

Done.

+
+  // Create and return Java peer, |callback| is invoked after request result
+  // is returned from Java peer and notified to delegate.
+  base::android::ScopedJavaLocalRef<jobject> CreateJavaPeer(
+      base::Callback<void(scoped_refptr<AwPermissionRequest>)> callback);
+
+  // Return the Java peer.
+  base::android::ScopedJavaLocalRef<jobject> GetJavaObject();
+
+  // Invoked by Java peer when request is processed, |granted| indicates the
+  // request was granted or not.
+  void OnAccept(JNIEnv* env, jobject jcaller, jboolean granted);
+
+  // Called by Java peer to get the origin which initiated the request.
+  base::android::ScopedJavaLocalRef<jstring> GetOrigin(JNIEnv* env,
+                                                       jobject jcaller);
+
+  // Called by Java peer to get the resoures requested by origin.
+  jlong GetResources(JNIEnv* env, jobject jcaller);
+
+  // Return the origin which initiated the request.
+  const GURL& GetOrigin();
+
+  // Return the origin which initiated the request.

[benm (inactive), 2014/04/16 14:51:10]

wrong comment.

[michaelbai, 2014/04/22 20:53:51]

Done.

+  int64 GetResources();
+
+ private:
+  friend class base::RefCounted<AwPermissionRequest>;
+  virtual ~AwPermissionRequest();
+
+  scoped_ptr<AwPermissionRequestDelegate> delegate_;
+  base::Callback<void(scoped_refptr<AwPermissionRequest>)> callback_;
+  JavaObjectWeakGlobalRef java_ref_;
+
+  DISALLOW_COPY_AND_ASSIGN(AwPermissionRequest);
+};
+
+bool RegisterAwPermissionRequest(JNIEnv* env);
+
+}  // namespace android_webivew
+
+#endif  // ANDROID_WEBVIEW_NATIVE_PERMISSION_AW_PERMISSION_REQUEST_H