Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
239793002:
Handle media access permission request (Closed)
|
Created:
6 years, 8 months ago by michaelbai Modified:
6 years, 7 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionThis patch is also implements the general permission request.
- AwPermissionRequest wraps a permission request.
- AwPermissionRequestDelegate interface should be implemented by specific permission request like media access permission request.
- PermissionRequestHandler, send the request to AwContentsClient or Cancel a ongoing request.
NOTRY=true
BUG=363301
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267495
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267576
Patch Set 1 : #
Total comments: 59
Patch Set 2 : addressed comments, added unittests #
Total comments: 8
Patch Set 3 : addressed comments #
Total comments: 8
Patch Set 4 : Remove refcounted #Patch Set 5 : sync #
Total comments: 7
Patch Set 6 : #
Total comments: 10
Patch Set 7 : address comments #Patch Set 8 : fix trybot error #Patch Set 9 : fix error, landed it again #Messages
Total messages: 36 (0 generated)
thanks michael! https://codereview.chromium.org/239793002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:186: // Change the abstract once merged add a TODO and /*abstract*/ like on line 176/177 please https://codereview.chromium.org/239793002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:32: mOrigin = Uri.parse(nativeGetOrigin(mNativeAwPermissionRequest)); Is it better to pass these values from native into the factory function, to avoid hopping into and out of native here? https://codereview.chromium.org/239793002/diff/20001/android_webview/native/a... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/native/a... android_webview/native/aw_contents.cc:583: void AwContents::OnPermissionRequest(ScopedJavaLocalRef<jobject> obj) { can we pass the native type into this function, and convert to a java ref here? Same for OnPermissionRequestCanceled https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... File android_webview/native/permission/aw_permission_request.cc (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/aw_permission_request.cc:71: return RegisterNativesImpl(env) >= 0; please don't do the comparison, just return the function result (see https://codereview.chromium.org/238043003/) https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... File android_webview/native/permission/aw_permission_request.h (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/aw_permission_request.h:27: GeoLocation = 1 << 0, nit s/L/l/ https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/aw_permission_request.h:57: // Return the origin which initiated the request. wrong comment. https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... File android_webview/native/permission/aw_permission_request_delegate.h (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/aw_permission_request_delegate.h:29: virtual void OnRequestResult(bool allowed) = 0; I think this makes it sound like we are beginning the request, not ending it. Also "On" makes it sound like this is an IPC. Maybe NotifyPermissionResult(bool allowed) ? https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... File android_webview/native/permission/media_access_permission_request.cc (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/media_access_permission_request.cc:38: MediaCaptureDevices::GetInstance()->GetAudioCaptureDevices(); Should we store the result? And why not video too? https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... File android_webview/native/permission/media_access_permission_request.h (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/media_access_permission_request.h:27: const content::MediaStreamRequest request_; nit: did you mean for these to be const? https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... File android_webview/native/permission/permission_request_handler.cc (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/permission_request_handler.cc:26: base::Bind(&PermissionRequestHandler::OnRequestProcessed, do we need a callback here? Would it be better to just pass |this| and have the permssion request invoke onRequestProcessed directly? https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... File android_webview/native/permission/permission_request_handler.h (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/permission_request_handler.h:27: void SendRequest(scoped_refptr<AwPermissionRequest> request); const & https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/permission_request_handler.h:43: void CancelRequest(RequestIterator i); Are there any callers for this now? I think we'd want to cancel any pending requests if we start a new navigation, for example. Do we actually really only want a CancelAllRequests() method here, and then have that iterate over all the pending requests and call the java peer Cancel? https://codereview.chromium.org/239793002/diff/20001/android_webview/test/she... File android_webview/test/shell/src/org/chromium/android_webview/test/NullContentsClient.java (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/test/she... android_webview/test/shell/src/org/chromium/android_webview/test/NullContentsClient.java:202: public void onPermissionRequest(AwPermissionRequest awPermissionRequest) { i think we want to deny in this default implementation, right?
https://codereview.chromium.org/239793002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:57: private void destory() { probably a good idea to have a finalizer for this. It could just throw an exception or log an error to logcat and call deny. https://codereview.chromium.org/239793002/diff/20001/android_webview/native/a... File android_webview/native/aw_web_contents_delegate.cc (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/native/a... android_webview/native/aw_web_contents_delegate.cc:210: aw_contents->GetPermissionRequestHandler()->SendRequest(permission_request); could the PermissionRequestHandler be a member of AwWebContentsDelegate and not AwContents? https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... File android_webview/native/permission/aw_permission_request.h (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/aw_permission_request.h:22: class AwPermissionRequest : public base::RefCounted<AwPermissionRequest> { do these need to be refcounted? could PermissionRequestHandler store weak pointers instead? https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/aw_permission_request.h:25: // android.webkit.PermissionRequest.java. 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. https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/aw_permission_request.h:33: AwPermissionRequest(AwPermissionRequestDelegate* delegate); if takes ownership then should take scoped_ptr<AwPermissionRequestDelegate> instead of raw pointer. https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... File android_webview/native/permission/media_access_permission_request.cc (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/media_access_permission_request.cc:55: if (!audio_devices.empty()) { nit: early return would be cleaner https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/media_access_permission_request.cc:65: if (!audio_device_found) could you explain why this is desired? https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/media_access_permission_request.cc:75: if (!request_.requested_video_device_id.empty()) { maybe encapsulate this into a findByIdOrGetFirst function? seems like we're doing the same thing for video and audio devices. https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... File android_webview/native/permission/media_access_permission_request.h (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/media_access_permission_request.h:15: class MediaAccessPermissionRequest : public AwPermissionRequestDelegate { same here, you should be able to easily add a c++ unittest https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... File android_webview/native/permission/permission_request_handler.cc (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/permission_request_handler.cc:37: while (i != requests_.end()) { could you explain why we need this loop? https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/permission_request_handler.cc:53: PermissionRequestHandler::FindRequest(const GURL& origin, please explain why can't we look requests up by comparing pointers? https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... File android_webview/native/permission/permission_request_handler.h (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/permission_request_handler.h:21: class PermissionRequestHandler { this class could have a native (C++) unittest https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/permission_request_handler.h:30: void CancelRequest(const GURL& origin, int64 resources); doesn't look like anyone calls this.
- Added unit tests for PermissionRequestHandler and MediaAccessPermissionRequest. - Added PermissionRequestHandlerClient, so PermissionRequestHandler isolates with AwContents and easy to test. https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/j... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/j... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:186: // Change the abstract once merged On 2014/04/16 14:51:10, benm wrote: > add a TODO and /*abstract*/ like on line 176/177 please Done. https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/j... File android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/j... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:32: mOrigin = Uri.parse(nativeGetOrigin(mNativeAwPermissionRequest)); On 2014/04/16 14:51:10, benm wrote: > Is it better to pass these values from native into the factory function, to > avoid hopping into and out of native here? Thanks https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/j... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:57: private void destory() { Right, thanks On 2014/04/17 10:34:06, mkosiba wrote: > probably a good idea to have a finalizer for this. It could just throw an > exception or log an error to logcat and call deny. https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/aw_contents.cc (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/aw_contents.cc:583: void AwContents::OnPermissionRequest(ScopedJavaLocalRef<jobject> obj) { I were thinking passing in AwPermissionRequest requires AwContents know the detail of AwPermissionRequest, but it seemed that using java object here isn't good for testing. On 2014/04/16 14:51:10, benm wrote: > can we pass the native type into this function, and convert to a java ref here? > Same for OnPermissionRequestCanceled https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/aw_web_contents_delegate.cc (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/aw_web_contents_delegate.cc:210: aw_contents->GetPermissionRequestHandler()->SendRequest(permission_request); Considering about the geolocation implementation, not all all requests are coming from the delegate class, and PermissionRequestHandler should gotten from AwContents and have same lifecycle with AwContents On 2014/04/17 10:34:06, mkosiba wrote: > could the PermissionRequestHandler be a member of AwWebContentsDelegate and not > AwContents? https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/permission/aw_permission_request.cc (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/aw_permission_request.cc:71: return RegisterNativesImpl(env) >= 0; On 2014/04/16 14:51:10, benm wrote: > please don't do the comparison, just return the function result > > (see https://codereview.chromium.org/238043003/) Done. https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/permission/aw_permission_request.h (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/aw_permission_request.h:22: class AwPermissionRequest : public base::RefCounted<AwPermissionRequest> { 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: > do these need to be refcounted? could PermissionRequestHandler store weak > pointers instead? https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/aw_permission_request.h:25: // android.webkit.PermissionRequest.java. Right, there is no good solution here. On 2014/04/17 10:34:06, mkosiba wrote: > 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. https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/aw_permission_request.h:27: GeoLocation = 1 << 0, On 2014/04/16 14:51:10, benm wrote: > nit s/L/l/ Done. https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/aw_permission_request.h:33: AwPermissionRequest(AwPermissionRequestDelegate* delegate); On 2014/04/17 10:34:06, mkosiba wrote: > if takes ownership then should take scoped_ptr<AwPermissionRequestDelegate> > instead of raw pointer. Done. https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/aw_permission_request.h:57: // Return the origin which initiated the request. On 2014/04/16 14:51:10, benm wrote: > wrong comment. Done. https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/permission/aw_permission_request_delegate.h (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/aw_permission_request_delegate.h:29: virtual void OnRequestResult(bool allowed) = 0; On 2014/04/16 14:51:10, benm wrote: > I think this makes it sound like we are beginning the request, not ending it. > Also "On" makes it sound like this is an IPC. > > Maybe NotifyPermissionResult(bool allowed) ? Done. https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/permission/media_access_permission_request.cc (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/media_access_permission_request.cc:38: MediaCaptureDevices::GetInstance()->GetAudioCaptureDevices(); Sorry, we don't need this, removed. On 2014/04/16 14:51:10, benm wrote: > Should we store the result? And why not video too? https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/media_access_permission_request.cc:55: if (!audio_devices.empty()) { Can't early return here, as video_devices might not be NULL. On 2014/04/17 10:34:06, mkosiba wrote: > nit: early return would be cleaner https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/media_access_permission_request.cc:65: if (!audio_device_found) The device id in request is optional, clank choose the device in the order of device id if exist, default device, then first device. we don't have default device in WebView, then use first device. On 2014/04/17 10:34:06, mkosiba wrote: > could you explain why this is desired? https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/media_access_permission_request.cc:75: if (!request_.requested_video_device_id.empty()) { On 2014/04/17 10:34:06, mkosiba wrote: > maybe encapsulate this into a findByIdOrGetFirst function? seems like we're > doing the same thing for video and audio devices. Done. https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/permission/media_access_permission_request.h (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/media_access_permission_request.h:15: class MediaAccessPermissionRequest : public AwPermissionRequestDelegate { On 2014/04/17 10:34:06, mkosiba wrote: > same here, you should be able to easily add a c++ unittest Actually, it is not easy to write a unit test when you need some content features. We need have a class subclass from RenderViewHostTestHarness to take advantage the content testing features. https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/media_access_permission_request.h:27: const content::MediaStreamRequest request_; Yes, I want it to be const On 2014/04/16 14:51:10, benm wrote: > nit: did you mean for these to be const? https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/permission/permission_request_handler.cc (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/permission_request_handler.cc:26: base::Bind(&PermissionRequestHandler::OnRequestProcessed, If just passing this, PermissionRequestHandler and AwPermissionRequest will know each other. I'd prefer AwPermissionRequest doesn't know anything about PermissionRequestHandler. This callback is considering as a observer method, I could add a observer interface in AwPermissionRequest, but it has only one method. This might be over skill. On 2014/04/16 14:51:10, benm wrote: > do we need a callback here? Would it be better to just pass |this| and have the > permssion request invoke onRequestProcessed directly? https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/permission_request_handler.cc:37: while (i != requests_.end()) { See added comment On 2014/04/17 10:34:06, mkosiba wrote: > could you explain why we need this loop? https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/permission_request_handler.cc:53: PermissionRequestHandler::FindRequest(const GURL& origin, Comparing by pointers requires caller keep reference of AwPermissionRequest, but it is no necessary, just like current implementation. On 2014/04/17 10:34:06, mkosiba wrote: > please explain why can't we look requests up by comparing pointers? https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/permission/permission_request_handler.h (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/permission_request_handler.h:21: class PermissionRequestHandler { On 2014/04/17 10:34:06, mkosiba wrote: > this class could have a native (C++) unittest Done. https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/permission_request_handler.h:27: void SendRequest(scoped_refptr<AwPermissionRequest> request); On 2014/04/16 14:51:10, benm wrote: > const & Done. https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/permission_request_handler.h:30: void CancelRequest(const GURL& origin, int64 resources); Yes, this method is here for migrating geolocation. On 2014/04/17 10:34:06, mkosiba wrote: > doesn't look like anyone calls this. https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/permission_request_handler.h:43: void CancelRequest(RequestIterator i); The geolocation has cancel method, see GeolocationPermissionContext, it will call into public CancelRequest(), then here. On 2014/04/16 14:51:10, benm wrote: > Are there any callers for this now? > > I think we'd want to cancel any pending requests if we start a new navigation, > for example. Do we actually really only want a CancelAllRequests() method here, > and then have that iterate over all the pending requests and call the java peer > Cancel? https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/t... File android_webview/test/shell/src/org/chromium/android_webview/test/NullContentsClient.java (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/t... android_webview/test/shell/src/org/chromium/android_webview/test/NullContentsClient.java:202: public void onPermissionRequest(AwPermissionRequest awPermissionRequest) { On 2014/04/16 14:51:10, benm wrote: > i think we want to deny in this default implementation, right? Done.
https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/aw_web_contents_delegate.cc (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/aw_web_contents_delegate.cc:210: aw_contents->GetPermissionRequestHandler()->SendRequest(permission_request); On 2014/04/22 20:53:51, michaelbai wrote: > Considering about the geolocation implementation, not all all requests are > coming from the delegate class, and PermissionRequestHandler should gotten from > AwContents and have same lifecycle with AwContents ok. thanks for the explanation! > > On 2014/04/17 10:34:06, mkosiba wrote: > > could the PermissionRequestHandler be a member of AwWebContentsDelegate and > not > > AwContents? > https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/permission/aw_permission_request.h (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/aw_permission_request.h:22: class AwPermissionRequest : public base::RefCounted<AwPermissionRequest> { On 2014/04/22 20:53:51, michaelbai wrote: > 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. 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? > On 2014/04/17 10:34:06, mkosiba wrote: > > do these need to be refcounted? could PermissionRequestHandler store weak > > pointers instead? > https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/permission/permission_request_handler.h (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/permission_request_handler.h:21: class PermissionRequestHandler { On 2014/04/22 20:53:51, michaelbai wrote: > On 2014/04/17 10:34:06, mkosiba wrote: > > this class could have a native (C++) unittest > > Done. appreciated. thanks! https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/a... File android_webview/android_webview_tests.gypi (right): https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/a... android_webview/android_webview_tests.gypi:92: 'libwebviewchromium', I'm surprised it doesn't crash. - you've just introduced a 2nd entry point to the test binary. what error do you get without this? All the webview code the tests depend on should already be part of android_webview_common. https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/j... File android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java (right): https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/j... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:50: public void grant() { do we want to enforce the correct thread here or will that be done in the glue layer? https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/j... File android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java (right): https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/j... android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:81: Thread.sleep(5 * 1000); instead you could use CriteriaHelper to poll for the title == 'grant' or == 'deny' (for the second test).
https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/n... File android_webview/native/permission/permission_request_handler_unittest.cc (right): https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/n... android_webview/native/permission/permission_request_handler_unittest.cc:11: class TestAwPermissionRequestDelegate : public AwPermissionRequestDelegate { sweet! thanks for adding these btw.
Sent from my Android On 23 Apr 2014 19:04, <mkosiba@chromium.org> wrote: > > > https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... > File android_webview/native/aw_web_contents_delegate.cc (right): > > https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... > android_webview/native/aw_web_contents_delegate.cc:210: > aw_contents->GetPermissionRequestHandler()->SendRequest(permission_request); > On 2014/04/22 20:53:51, michaelbai wrote: >> >> Considering about the geolocation implementation, not all all requests > > are >> >> coming from the delegate class, and PermissionRequestHandler should > > gotten from >> >> AwContents and have same lifecycle with AwContents > > > ok. thanks for the explanation! > > > >> On 2014/04/17 10:34:06, mkosiba wrote: >> > could the PermissionRequestHandler be a member of > > AwWebContentsDelegate and >> >> not >> > AwContents? > > > > https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... > File android_webview/native/permission/aw_permission_request.h (right): > > https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... > android_webview/native/permission/aw_permission_request.h:22: class > AwPermissionRequest : public base::RefCounted<AwPermissionRequest> { > On 2014/04/22 20:53:51, michaelbai wrote: >> >> 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. > > > 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? > > > >> On 2014/04/17 10:34:06, mkosiba wrote: >> > do these need to be refcounted? could PermissionRequestHandler store > > weak >> >> > pointers instead? > > > > https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... > File android_webview/native/permission/permission_request_handler.h > (right): > > https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... > android_webview/native/permission/permission_request_handler.h:21: class > PermissionRequestHandler { > On 2014/04/22 20:53:51, michaelbai wrote: >> >> On 2014/04/17 10:34:06, mkosiba wrote: >> > this class could have a native (C++) unittest > > >> Done. > > > appreciated. thanks! > > https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/a... > File android_webview/android_webview_tests.gypi (right): > > https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/a... > android_webview/android_webview_tests.gypi:92: 'libwebviewchromium', > I'm surprised it doesn't crash. - you've just introduced a 2nd entry > point to the test binary. > > what error do you get without this? All the webview code the tests > depend on should already be part of android_webview_common. > > https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/j... > File > android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java > (right): > > https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/j... > android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:50: > public void grant() { > do we want to enforce the correct thread here or will that be done in > the glue layer? Let's keep thread checking/hopping logic upstream as much as possible. > > https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/j... > File > android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java > (right): > > https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/j... > android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:81: > Thread.sleep(5 * 1000); > instead you could use CriteriaHelper to poll for the title == 'grant' or > == 'deny' (for the second test). > > https://chromiumcodereview.appspot.com/239793002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PTAL https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/permission/aw_permission_request.h (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/aw_permission_request.h:22: class AwPermissionRequest : public base::RefCounted<AwPermissionRequest> { 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: > On 2014/04/22 20:53:51, michaelbai wrote: > > 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. > > 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? > > > > On 2014/04/17 10:34:06, mkosiba wrote: > > > do these need to be refcounted? could PermissionRequestHandler store weak > > > pointers instead? > > > https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/a... File android_webview/android_webview_tests.gypi (right): https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/a... android_webview/android_webview_tests.gypi:92: 'libwebviewchromium', oops, it should be removed, I were hoping it could avoid call RegisterJni. there is no crash, why you think it is add 2nd entry point, were you talking about Jni_onload? On 2014/04/23 18:04:40, mkosiba wrote: > I'm surprised it doesn't crash. - you've just introduced a 2nd entry point to > the test binary. > > what error do you get without this? All the webview code the tests depend on > should already be part of android_webview_common. https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/j... File android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java (right): https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/j... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:50: public void grant() { On 2014/04/23 18:04:40, mkosiba wrote: > do we want to enforce the correct thread here or will that be done in the glue > layer? Done. https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/j... File android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java (right): https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/j... android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:81: Thread.sleep(5 * 1000); On 2014/04/23 18:04:40, mkosiba wrote: > instead you could use CriteriaHelper to poll for the title == 'grant' or == > 'deny' (for the second test). Done.
https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/permission/aw_permission_request.h (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/aw_permission_request.h:22: class AwPermissionRequest : public base::RefCounted<AwPermissionRequest> { On 2014/04/24 00:56:43, michaelbai wrote: > 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. 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." > > 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. 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, > I think it is simple to make it refcounting. 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. > > On 2014/04/23 18:04:40, mkosiba wrote: > > On 2014/04/22 20:53:51, michaelbai wrote: > > > 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. > > > > 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? > > > > > > > On 2014/04/17 10:34:06, mkosiba wrote: > > > > do these need to be refcounted? could PermissionRequestHandler store weak > > > > pointers instead? > > > > > > https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/a... File android_webview/android_webview_tests.gypi (right): https://chromiumcodereview.appspot.com/239793002/diff/30001/android_webview/a... android_webview/android_webview_tests.gypi:92: 'libwebviewchromium', On 2014/04/24 00:56:43, michaelbai wrote: > oops, it should be removed, I were hoping it could avoid call RegisterJni. there > is no crash, why you think it is add 2nd entry point, were you talking about > Jni_onload? yes, testing/android/native_test_launcher.cc defines JNI_OnLoad which I thought would be shadowed by the one from android_webview/lib/main/webview_entry_point.cc > > On 2014/04/23 18:04:40, mkosiba wrote: > > I'm surprised it doesn't crash. - you've just introduced a 2nd entry point to > > the test binary. > > > > what error do you get without this? All the webview code the tests depend on > > should already be part of android_webview_common. > https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/j... File android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java (right): https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/j... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:57: destory(); I think it would be clearer if you kept around an extra boolean to keep track of whether grant or deny was already called. That way we could throw an exception if one of these methods are called more than once (which I think we should). https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/j... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:74: private void destory() { nit: I'd call this onNativeDestroyed or detachNativeInstance. Destroy implies this method deletes the native instance too, which it doesn't. https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/j... File android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java (right): https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/j... android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:116: return CriteriaHelper.pollForCriteria(new Criteria () { sorry, my bad, totally forgot about this. AwTestBase has a poll method that wraps around criteriahelper with the right timeouts. Please re-write this as: poll(new Callable<Boolean>() { @Override public Boolean call() throws Exception { return title.equals(getTitleOnUiThread(awContents)); } }); the poll method will throw an exception when polling for criteria fails so it has no return value. https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/l... File android_webview/lib/main/webview_tests.cc (right): https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/l... android_webview/lib/main/webview_tests.cc:10: android_webview::RegisterJni(base::android::AttachCurrentThread()); some of the unittests call register methods for classes that would be registered from here too. Not sure if that will cause problems or not but if you see failures in other unittests (like InputStreamTest) you'll have to remove the individual calls from those unittests.
PTAL https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/permission/aw_permission_request.h (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/aw_permission_request.h:22: class AwPermissionRequest : public base::RefCounted<AwPermissionRequest> { On 2014/04/24 09:24:58, mkosiba wrote: > On 2014/04/24 00:56:43, michaelbai wrote: > > 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. > > 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. > 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. > 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." > > > > > 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. > > 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, > > > I think it is simple to make it refcounting. > > 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. > It isn't such confusing, as long as you use scoped_refptr, there is nothing you need worried about. > 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. > > > > > On 2014/04/23 18:04:40, mkosiba wrote: > > > On 2014/04/22 20:53:51, michaelbai wrote: > > > > 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. > > > > > > 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? > > > > > > > > > > On 2014/04/17 10:34:06, mkosiba wrote: > > > > > do these need to be refcounted? could PermissionRequestHandler store > weak > > > > > pointers instead? > > > > > > > > > > https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/j... File android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java (right): https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/j... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:57: destory(); This helps the developer debug, like finalizer(), I think outputting a error message is sufficient. On 2014/04/24 09:24:58, mkosiba wrote: > I think it would be clearer if you kept around an extra boolean to keep track of > whether grant or deny was already called. That way we could throw an exception > if one of these methods are called more than once (which I think we should). https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/j... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:74: private void destory() { On 2014/04/24 09:24:58, mkosiba wrote: > nit: I'd call this onNativeDestroyed or detachNativeInstance. Destroy implies > this method deletes the native instance too, which it doesn't. Done. https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/j... File android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java (right): https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/j... android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:116: return CriteriaHelper.pollForCriteria(new Criteria () { On 2014/04/24 09:24:58, mkosiba wrote: > sorry, my bad, totally forgot about this. AwTestBase has a poll method that > wraps around criteriahelper with the right timeouts. Please re-write this as: > > poll(new Callable<Boolean>() { > @Override > public Boolean call() throws Exception { > return title.equals(getTitleOnUiThread(awContents)); > } > }); > > the poll method will throw an exception when polling for criteria fails so it > has no return value. Done.
PTAL https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... File android_webview/native/permission/aw_permission_request.h (right): https://chromiumcodereview.appspot.com/239793002/diff/20001/android_webview/n... android_webview/native/permission/aw_permission_request.h:22: class AwPermissionRequest : public base::RefCounted<AwPermissionRequest> { On 2014/04/24 09:24:58, mkosiba wrote: > On 2014/04/24 00:56:43, michaelbai wrote: > > 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. > > 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. > 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. > 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." > > > > > 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. > > 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, > > > I think it is simple to make it refcounting. > > 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. > It isn't such confusing, as long as you use scoped_refptr, there is nothing you need worried about. > 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. > > > > > On 2014/04/23 18:04:40, mkosiba wrote: > > > On 2014/04/22 20:53:51, michaelbai wrote: > > > > 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. > > > > > > 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? > > > > > > > > > > On 2014/04/17 10:34:06, mkosiba wrote: > > > > > do these need to be refcounted? could PermissionRequestHandler store > weak > > > > > pointers instead? > > > > > > > > > > https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/j... File android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java (right): https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/j... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:57: destory(); This helps the developer debug, like finalizer(), I think outputting a error message is sufficient. On 2014/04/24 09:24:58, mkosiba wrote: > I think it would be clearer if you kept around an extra boolean to keep track of > whether grant or deny was already called. That way we could throw an exception > if one of these methods are called more than once (which I think we should). https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/j... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:74: private void destory() { On 2014/04/24 09:24:58, mkosiba wrote: > nit: I'd call this onNativeDestroyed or detachNativeInstance. Destroy implies > this method deletes the native instance too, which it doesn't. Done. https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/j... File android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java (right): https://chromiumcodereview.appspot.com/239793002/diff/50001/android_webview/j... android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:116: return CriteriaHelper.pollForCriteria(new Criteria () { On 2014/04/24 09:24:58, mkosiba wrote: > sorry, my bad, totally forgot about this. AwTestBase has a poll method that > wraps around criteriahelper with the right timeouts. Please re-write this as: > > poll(new Callable<Boolean>() { > @Override > public Boolean call() throws Exception { > return title.equals(getTitleOnUiThread(awContents)); > } > }); > > the poll method will throw an exception when polling for criteria fails so it > has no return value. Done.
https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... File android_webview/native/permission/aw_permission_request.h (right): https://codereview.chromium.org/239793002/diff/20001/android_webview/native/p... android_webview/native/permission/aw_permission_request.h:22: class AwPermissionRequest : public base::RefCounted<AwPermissionRequest> { On 2014/04/25 01:06:09, michaelbai wrote: > 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. Thanks! https://codereview.chromium.org/239793002/diff/50001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java (right): https://codereview.chromium.org/239793002/diff/50001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:57: destory(); On 2014/04/25 01:06:09, michaelbai wrote: > This helps the developer debug, like finalizer(), I think outputting a error > message is sufficient. A good API helps you spot logical mistakes in your code. I'm quite confident that calling grant and then deny is a logical mistake. I don't think many developers look at logcat when running their code. Throwing an IllegalStateException in this case will alert them to the problem. I asked Ben for his opinion on this and he also thinks we should throw. > > On 2014/04/24 09:24:58, mkosiba wrote: > > I think it would be clearer if you kept around an extra boolean to keep track > of > > whether grant or deny was already called. That way we could throw an exception > > if one of these methods are called more than once (which I think we should). > https://codereview.chromium.org/239793002/diff/90001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java (right): https://codereview.chromium.org/239793002/diff/90001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:53: ThreadUtils.runOnUiThread(new Runnable() { I think we throw exceptions when you call WebView APIs from a thread different than the UI thread. Unless you think there is a realistic use case for processing the PermissionRequest on a different thread we should just throw if we're on the wrong thread. https://codereview.chromium.org/239793002/diff/90001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:59: detachNativeInstance(); You don't need this call anymore. nativeOnAccept will call detachNativeInstance. https://codereview.chromium.org/239793002/diff/90001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java (right): https://codereview.chromium.org/239793002/diff/90001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:60: // The test isn't passed due to WebRTC requests security origin. Is this something you're addressing with a separate CL? What's the plan for enabling the test?
PTAL https://codereview.chromium.org/239793002/diff/90001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java (right): https://codereview.chromium.org/239793002/diff/90001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:53: ThreadUtils.runOnUiThread(new Runnable() { On 2014/04/28 15:35:36, mkosiba wrote: > I think we throw exceptions when you call WebView APIs from a thread different > than the UI thread. Unless you think there is a realistic use case for > processing the PermissionRequest on a different thread we should just throw if > we're on the wrong thread. Done. https://codereview.chromium.org/239793002/diff/90001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:59: detachNativeInstance(); On 2014/04/28 15:35:36, mkosiba wrote: > You don't need this call anymore. nativeOnAccept will call detachNativeInstance. Done. https://codereview.chromium.org/239793002/diff/90001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:88: deny(); Do we need to throw exception here? https://codereview.chromium.org/239793002/diff/90001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java (right): https://codereview.chromium.org/239793002/diff/90001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:60: // The test isn't passed due to WebRTC requests security origin. Currently, webrtc can't run with data scheme, I am finding out whether there is security issue of it. On 2014/04/28 15:35:36, mkosiba wrote: > Is this something you're addressing with a separate CL? What's the plan for > enabling the test?
https://codereview.chromium.org/239793002/diff/110001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java (right): https://codereview.chromium.org/239793002/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:85: throw new IllegalStateException("Either grant() or deny() has been called."); nit: has been _already_ called. https://codereview.chromium.org/239793002/diff/110001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java (right): https://codereview.chromium.org/239793002/diff/110001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:60: // The test isn't passed due to WebRTC requests security origin. if the only problem is the data: URL could we host the data on the TestWebServer instead? https://codereview.chromium.org/239793002/diff/110001/android_webview/native/... File android_webview/native/permission/aw_permission_request.h (right): https://codereview.chromium.org/239793002/diff/110001/android_webview/native/... android_webview/native/permission/aw_permission_request.h:40: base::android::ScopedJavaLocalRef<jobject> CreateJavaPeer(); The 2 separate methods for creating and getting a java object are a bit confusing. Especially that you don't actually use the result of one of them... https://codereview.chromium.org/239793002/diff/110001/android_webview/native/... File android_webview/native/permission/permission_request_handler.cc (right): https://codereview.chromium.org/239793002/diff/110001/android_webview/native/... android_webview/native/permission/permission_request_handler.cc:30: ScopedJavaLocalRef<jobject> java_ref = aw_request->CreateJavaPeer(); this is a bit confusing - you're creating the java peer but not using it? https://codereview.chromium.org/239793002/diff/110001/android_webview/native/... android_webview/native/permission/permission_request_handler.cc:36: PruneRequests(); If you register the PermissionRequestHandler as an observer to the AwPermissionRequest (and add a ondestroyed notification to the observers) you should be able to know exactly when to remove requests from the list.
PTAL https://codereview.chromium.org/239793002/diff/110001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java (right): https://codereview.chromium.org/239793002/diff/110001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/permission/AwPermissionRequest.java:85: throw new IllegalStateException("Either grant() or deny() has been called."); On 2014/04/29 17:29:53, mkosiba wrote: > nit: has been _already_ called. Done. https://codereview.chromium.org/239793002/diff/110001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java (right): https://codereview.chromium.org/239793002/diff/110001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:60: // The test isn't passed due to WebRTC requests security origin. It also need to be https:// if use TestWebServer. On 2014/04/29 17:29:53, mkosiba wrote: > if the only problem is the data: URL could we host the data on the TestWebServer > instead? https://codereview.chromium.org/239793002/diff/110001/android_webview/native/... File android_webview/native/permission/aw_permission_request.h (right): https://codereview.chromium.org/239793002/diff/110001/android_webview/native/... android_webview/native/permission/aw_permission_request.h:40: base::android::ScopedJavaLocalRef<jobject> CreateJavaPeer(); Thanks, this left from previous design, and just found the Java object could be GC-ed between CreateJavaPeer() and GetJavaObject() On 2014/04/29 17:29:53, mkosiba wrote: > The 2 separate methods for creating and getting a java object are a bit > confusing. Especially that you don't actually use the result of one of them... https://codereview.chromium.org/239793002/diff/110001/android_webview/native/... File android_webview/native/permission/permission_request_handler.cc (right): https://codereview.chromium.org/239793002/diff/110001/android_webview/native/... android_webview/native/permission/permission_request_handler.cc:30: ScopedJavaLocalRef<jobject> java_ref = aw_request->CreateJavaPeer(); On 2014/04/29 17:29:53, mkosiba wrote: > this is a bit confusing - you're creating the java peer but not using it? Done. https://codereview.chromium.org/239793002/diff/110001/android_webview/native/... android_webview/native/permission/permission_request_handler.cc:36: PruneRequests(); There was a callback like observer, Using weakref here is to remove that interface and make design simpler. On 2014/04/29 17:29:53, mkosiba wrote: > If you register the PermissionRequestHandler as an observer to the > AwPermissionRequest (and add a ondestroyed notification to the observers) you > should be able to know exactly when to remove requests from the list.
lgtm
The CQ bit was checked by michaelbai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/239793002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
The CQ bit was checked by michaelbai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/239793002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium
The CQ bit was checked by michaelbai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/239793002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium
The CQ bit was checked by michaelbai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/239793002/140001
Message was sent while issue was closed.
Change committed as 267495
Message was sent while issue was closed.
On 2014/05/01 08:52:54, I haz the power (commit-bot) wrote: > Change committed as 267495 It looks like this broke Android builder: http://build.chromium.org/p/chromium.linux/builders/Android%20Builder%20%28db... In file included from ../../android_webview/native/permission/aw_permission_request_delegate.cc:5:0: ../../android_webview/native/permission/aw_permission_request_delegate.h:8:37: fatal error: base/android/jni_helper.h: No such file or directory compilation terminated. etc
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/268543005/ by falken@chromium.org. The reason for reverting is: It looks like this broke Android builder: http://build.chromium.org/p/chromium.linux/builders/Android%20Builder%20%28db... In file included from ../../android_webview/native/permission/aw_permission_request_delegate.cc:5:0: ../../android_webview/native/permission/aw_permission_request_delegate.h:8:37: fatal error: base/android/jni_helper.h: No such file or directory compilation terminated. etc.
The CQ bit was checked by michaelbai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/239793002/150001
Message was sent while issue was closed.
Change committed as 267576 |
