permissions: handle request ids for permissions in permission manager

permissions: handle request ids for permissions in permission manager

Until this point permission service was generating ids and passing these to
permission manager to use. However, since each web_contents has its own
service, these ids are not unique.

If data needs to be stored in manager a string representation of the request
needs to be used which is ugly to say the least.

Switch to permission manager generating request ids so they are unique to the
manager and return this when request occurs for use by service to cancel.

BUG=516626
Committed: https://crrev.com/27583e9b340401a935ee45c5110506cc5a858c59
Cr-Commit-Position: refs/heads/master@{#352002}

[Lalit Maganti, 2015-09-14 16:12:32]

No documentation or aw/layout/shell but just to let you have a look to see if
this is OK.

[Lalit Maganti, 2015-09-14 16:15:53]

lalitm@google.com changed reviewers:
+ mlamouri@chromium.org

[Lalit Maganti, 2015-09-14 16:15:54]

lalitm@google.com changed reviewers:
+ mlamouri@chromium.org

[Lalit Maganti, 2015-09-14 16:15:54]

Actually add you this time :P

See above comment.

[Lalit Maganti, 2015-09-14 16:15:54]

Actually add you this time :P

See above comment.

[mlamouri (slow - plz ping), 2015-09-15 12:36:14]

I don't think that change is quite right. You don't actually use RenderFrameHost
as a key to cancel the permission, only request_id is being used. Maybe the
change you ought to make is to have CancelPermissionRequest() only take a
request_id?

https://codereview.chromium.org/1342833002/diff/60001/content/public/browser/...
File content/public/browser/permission_manager.h (right):

https://codereview.chromium.org/1342833002/diff/60001/content/public/browser/...
content/public/browser/permission_manager.h:36: // Cancels a previous permission
request specified by |request_id|.
What happens if trying to cancel a permission that was already canceled?

[Lalit Maganti, 2015-09-16 13:22:56]

Mounir: I think this is ready for a look now.

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

https://codereview.chromium.org/1342833002/diff/180001/android_webview/browse...
File android_webview/browser/aw_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/180001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:231: const GURL&
embedding_origin,
Why do you pass |request_id| with all the other values that are inside the
PendingRequest object? Seems like you could make that function a bit leaner.

https://codereview.chromium.org/1342833002/diff/180001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:253: return
pending_requests_.Add(request);
Is having that helper method really better than doing:
int request_id = pending_requests_.Add(new PendingRequest(render_process_id,
render_frame_id, permission, request_origin));

Note that you could even pass RenderFrameHost in there instead of the two ids.

https://codereview.chromium.org/1342833002/diff/180001/android_webview/browse...
File android_webview/browser/aw_permission_manager.h (right):

https://codereview.chromium.org/1342833002/diff/180001/android_webview/browse...
android_webview/browser/aw_permission_manager.h:54: const
base::WeakPtr<AwPermissionManager>& manager,
Don't make the method static, having a WeakPtr will do the right thing: the
callback will not be run if the WeakPtr is null.

https://codereview.chromium.org/1342833002/diff/180001/chrome/browser/permiss...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:178: const PermissionRequestID
request(render_process_id,
Will we be able to remove that at some point?

https://codereview.chromium.org/1342833002/diff/180001/chromecast/browser/cas...
File chromecast/browser/cast_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/180001/chromecast/browser/cas...
chromecast/browser/cast_permission_manager.cc:29: return -1;
use the constant

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
File content/browser/permissions/permission_service_impl.cc (left):

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:122: 
nit: keep that line

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:120: int request_id =
pending_requests_.Add(
nit: rename to pending_request_id

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:122: int manager_id =
browser_context->GetPermissionManager()->RequestPermission(
s/manager_id/id/

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:133: PendingRequest*
pending_request = pending_requests_.Lookup(request_id);
This is a bit unfortunate. Though, unless we call all the callbacks
asynchronously I don't think I have a better plan.

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:136:
pending_request->manager_id = manager_id;
pending_request->id = id;

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
File content/browser/permissions/permission_service_impl.h (right):

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
content/browser/permissions/permission_service_impl.h:51: int manager_id;
nit: rename to id to stay consistent with the subscription code

https://codereview.chromium.org/1342833002/diff/180001/content/public/browser...
File content/public/browser/permission_manager.h (right):

https://codereview.chromium.org/1342833002/diff/180001/content/public/browser...
content/public/browser/permission_manager.h:29: // no further need to cancel the
permission but |callback| MUST have been
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."

https://codereview.chromium.org/1342833002/diff/180001/content/public/browser...
content/public/browser/permission_manager.h:82: static const int
kCancelUnsubscribeNoOp = -1;
style: kFooBar isn't right per coding style.
nit: I think the name should be NO_PENDING_REQUEST.

https://codereview.chromium.org/1342833002/diff/180001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_permission_manager.cc
(right):

https://codereview.chromium.org/1342833002/diff/180001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_permission_manager.cc:76: return
-1;
return PermissionManager::CONSTANT

https://codereview.chromium.org/1342833002/diff/180001/content/shell/browser/...
File content/shell/browser/shell_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/180001/content/shell/browser/...
content/shell/browser/shell_permission_manager.cc:27: return -1;
ditto

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

https://codereview.chromium.org/1342833002/diff/180001/android_webview/browse...
File android_webview/browser/aw_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/180001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:231: const GURL&
embedding_origin,
On 2015/09/16 14:42:56, Mounir Lamouri wrote:
> Why do you pass |request_id| with all the other values that are inside the
> PendingRequest object? Seems like you could make that function a bit leaner.

Good point. Fixed.

https://codereview.chromium.org/1342833002/diff/180001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:253: return
pending_requests_.Add(request);
On 2015/09/16 14:42:56, Mounir Lamouri wrote:
> Is having that helper method really better than doing:
> int request_id = pending_requests_.Add(new PendingRequest(render_process_id,
> render_frame_id, permission, request_origin));
> 
> Note that you could even pass RenderFrameHost in there instead of the two ids.

Done.

https://codereview.chromium.org/1342833002/diff/180001/android_webview/browse...
File android_webview/browser/aw_permission_manager.h (right):

https://codereview.chromium.org/1342833002/diff/180001/android_webview/browse...
android_webview/browser/aw_permission_manager.h:54: const
base::WeakPtr<AwPermissionManager>& manager,
On 2015/09/16 14:42:56, Mounir Lamouri wrote:
> Don't make the method static, having a WeakPtr will do the right thing: the
> callback will not be run if the WeakPtr is null.

As discussed offline.

https://codereview.chromium.org/1342833002/diff/180001/chrome/browser/permiss...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:178: const PermissionRequestID
request(render_process_id,
On 2015/09/16 14:42:56, Mounir Lamouri wrote:
> Will we be able to remove that at some point?

Hoping so yes. But not unless the infobar manager lands since queue controller
makes heavy use of PermissionRequestID.

https://codereview.chromium.org/1342833002/diff/180001/chromecast/browser/cas...
File chromecast/browser/cast_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/180001/chromecast/browser/cas...
chromecast/browser/cast_permission_manager.cc:29: return -1;
On 2015/09/16 14:42:56, Mounir Lamouri wrote:
> use the constant

Done.

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
File content/browser/permissions/permission_service_impl.cc (left):

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:122: 
On 2015/09/16 14:42:57, Mounir Lamouri wrote:
> nit: keep that line

Done.

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:120: int request_id =
pending_requests_.Add(
On 2015/09/16 14:42:56, Mounir Lamouri wrote:
> nit: rename to pending_request_id

Done.

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:122: int manager_id =
browser_context->GetPermissionManager()->RequestPermission(
On 2015/09/16 14:42:56, Mounir Lamouri wrote:
> s/manager_id/id/

Done.

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:133: PendingRequest*
pending_request = pending_requests_.Lookup(request_id);
On 2015/09/16 14:42:57, Mounir Lamouri wrote:
> This is a bit unfortunate. Though, unless we call all the callbacks
> asynchronously I don't think I have a better plan.

Agreed.

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:136:
pending_request->manager_id = manager_id;
On 2015/09/16 14:42:56, Mounir Lamouri wrote:
> pending_request->id = id;

Done.

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
File content/browser/permissions/permission_service_impl.h (right):

https://codereview.chromium.org/1342833002/diff/180001/content/browser/permis...
content/browser/permissions/permission_service_impl.h:51: int manager_id;
On 2015/09/16 14:42:57, Mounir Lamouri wrote:
> nit: rename to id to stay consistent with the subscription code

Done.

https://codereview.chromium.org/1342833002/diff/180001/content/public/browser...
File content/public/browser/permission_manager.h (right):

https://codereview.chromium.org/1342833002/diff/180001/content/public/browser...
content/public/browser/permission_manager.h:29: // no further need to cancel the
permission but |callback| MUST have been
On 2015/09/16 14:42:57, Mounir Lamouri wrote:
> 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."

Done.

https://codereview.chromium.org/1342833002/diff/180001/content/public/browser...
content/public/browser/permission_manager.h:82: static const int
kCancelUnsubscribeNoOp = -1;
On 2015/09/16 14:42:57, Mounir Lamouri wrote:
> style: kFooBar isn't right per coding style.
> nit: I think the name should be NO_PENDING_REQUEST.

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?

https://codereview.chromium.org/1342833002/diff/180001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_permission_manager.cc
(right):

https://codereview.chromium.org/1342833002/diff/180001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_permission_manager.cc:76: return
-1;
On 2015/09/16 14:42:57, Mounir Lamouri wrote:
> return PermissionManager::CONSTANT

Done.

https://codereview.chromium.org/1342833002/diff/180001/content/shell/browser/...
File content/shell/browser/shell_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/180001/content/shell/browser/...
content/shell/browser/shell_permission_manager.cc:27: return -1;
On 2015/09/16 14:42:57, Mounir Lamouri wrote:
> ditto

Done.

[mlamouri (slow - plz ping), 2015-09-23 14:46:00]

sgtm! Please apply the comments and add the required OWNERS to review.

https://codereview.chromium.org/1342833002/diff/240001/android_webview/browse...
File android_webview/browser/aw_permission_manager.h (right):

https://codereview.chromium.org/1342833002/diff/240001/android_webview/browse...
android_webview/browser/aw_permission_manager.h:53: static void
OnRequestResponse(
Could you add a comment explaining why you pass |this| in here?

https://codereview.chromium.org/1342833002/diff/240001/chrome/browser/permiss...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/240001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:175:
pending_request->permission = permission;
Could you add a ctor to PendingRequest?

https://codereview.chromium.org/1342833002/diff/240001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:177: int request_id =
pending_requests_.Add(pending_request);
... you could even call new PendingRequest() here using the ctor.

https://codereview.chromium.org/1342833002/diff/240001/content/browser/permis...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/1342833002/diff/240001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:48: : id(-1),
I think you could use the constant here.

https://codereview.chromium.org/1342833002/diff/240001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:134: // the response
callback.
nit: could you rephrase to explain that the callback might have been run
synchronously.

https://codereview.chromium.org/1342833002/diff/240001/content/public/browser...
File content/public/browser/permission_manager.h (right):

https://codereview.chromium.org/1342833002/diff/240001/content/public/browser...
content/public/browser/permission_manager.h:83: static const int
kNoPendingRequestOrSubscription = -1;
Good point, the exception was for enums. Though, I would prefer a shorter name.
What about kNoPendingOperation?

[Lalit Maganti, 2015-09-23 15:24:30]

lalitm@google.com changed reviewers:
+ jochen@chromium.org, lcwu@chromium.org, mnaganov@chromium.org

[Lalit Maganti, 2015-09-23 15:24:31]

jochen@: can you take a look at content/permission_manager and rubberstamp
everything else in content/chrome?
lcwu@: can you take a look at the chromecast/ changes?
mnaganov@: can you take a look at the android_webview/ changes?

https://codereview.chromium.org/1342833002/diff/240001/android_webview/browse...
File android_webview/browser/aw_permission_manager.h (right):

https://codereview.chromium.org/1342833002/diff/240001/android_webview/browse...
android_webview/browser/aw_permission_manager.h:53: static void
OnRequestResponse(
On 2015/09/23 14:46:00, Mounir Lamouri wrote:
> Could you add a comment explaining why you pass |this| in here?

Done.

https://codereview.chromium.org/1342833002/diff/240001/chrome/browser/permiss...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/240001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:175:
pending_request->permission = permission;
On 2015/09/23 14:46:00, Mounir Lamouri wrote:
> Could you add a ctor to PendingRequest?

Done.

https://codereview.chromium.org/1342833002/diff/240001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:177: int request_id =
pending_requests_.Add(pending_request);
On 2015/09/23 14:46:00, Mounir Lamouri wrote:
> ... you could even call new PendingRequest() here using the ctor.

The way I did the constructor means that I need the pointer.

https://codereview.chromium.org/1342833002/diff/240001/content/browser/permis...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/1342833002/diff/240001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:48: : id(-1),
On 2015/09/23 14:46:00, Mounir Lamouri wrote:
> I think you could use the constant here.

Done.

https://codereview.chromium.org/1342833002/diff/240001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:134: // the response
callback.
On 2015/09/23 14:46:00, Mounir Lamouri wrote:
> nit: could you rephrase to explain that the callback might have been run
> synchronously.

Done.

https://codereview.chromium.org/1342833002/diff/240001/content/public/browser...
File content/public/browser/permission_manager.h (right):

https://codereview.chromium.org/1342833002/diff/240001/content/public/browser...
content/public/browser/permission_manager.h:83: static const int
kNoPendingRequestOrSubscription = -1;
On 2015/09/23 14:46:00, Mounir Lamouri wrote:
> Good point, the exception was for enums. Though, I would prefer a shorter
name.
> What about kNoPendingOperation?

Done.

[mnaganov (inactive), 2015-09-23 16:09:35]

mnaganov@chromium.org changed reviewers:
+ michaelbai@google.com
- mnaganov@chromium.org

[mnaganov (inactive), 2015-09-23 16:10:21]

mnaganov@chromium.org changed reviewers:
+ mnaganov@chromium.org

[mnaganov (inactive), 2015-09-23 16:10:22]

I would suggest Tao to look at this, as he is the author of these files.

[michaelbai, 2015-09-23 16:57:25]

michaelbai@chromium.org changed reviewers:
+ michaelbai@chromium.org

[michaelbai, 2015-09-23 16:57:26]

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
File android_webview/browser/aw_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:151: int render_frame_id;
It seemed that all those data members can not be used to identify the permission
request, i.e. different request id could map to same pending request (have same
data members)

If so, using PendingRequest in CancelPermissionRequest() seems not right.

Did I miss something?

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
File android_webview/browser/aw_permission_manager.h (right):

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
android_webview/browser/aw_permission_manager.h:61: bool allowed);
This hasn't to be class's static method, move it to cc file.

[Lalit Maganti, 2015-09-23 17:02:07]

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
File android_webview/browser/aw_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:151: int render_frame_id;
On 2015/09/23 16:57:26, michaelbai wrote:
> It seemed that all those data members can not be used to identify the
permission
> request, i.e. different request id could map to same pending request (have
same
> data members)
> 
> If so, using PendingRequest in CancelPermissionRequest() seems not right.
> 
> Did I miss something?
> 

This has now changed. Since PermissionManager implentations now return an int,
the request_id is unique with respect to the PermissionManager and
PermissionService will use the request_id returned when cancelling.

For context, most of this work is being done so that multiple permissions can be
requested at the same time. See https://codereview.chromium.org/1316863010/ for
more info on that.

[michaelbai, 2015-09-23 17:56:26]

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
File android_webview/browser/aw_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:151: int render_frame_id;
On 2015/09/23 17:02:07, Lalit Maganti wrote:
> On 2015/09/23 16:57:26, michaelbai wrote:
> > It seemed that all those data members can not be used to identify the
> permission
> > request, i.e. different request id could map to same pending request (have
> same
> > data members)
> > 
> > If so, using PendingRequest in CancelPermissionRequest() seems not right.
> > 
> > Did I miss something?
> > 
> 
> This has now changed. Since PermissionManager implentations now return an int,
> the request_id is unique with respect to the PermissionManager and
> PermissionService will use the request_id returned when cancelling.
> 
> For context, most of this work is being done so that multiple permissions can
be
> requested at the same time. See https://codereview.chromium.org/1316863010/
for
> more info on that.

My question is
Can the data members of PendingRequest be used to identify a permission request?
(in multiple permission case, using vector<PermissionType>)

If so, why request id needed?

If not, the implementation of AwPermissionManager::CancelPermissionRequest(int
request_id) is wrong.

[Lalit Maganti, 2015-09-23 18:02:37]

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
File android_webview/browser/aw_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:151: int render_frame_id;
On 2015/09/23 17:56:26, michaelbai wrote:
> On 2015/09/23 17:02:07, Lalit Maganti wrote:
> > On 2015/09/23 16:57:26, michaelbai wrote:
> > > It seemed that all those data members can not be used to identify the
> > permission
> > > request, i.e. different request id could map to same pending request (have
> > same
> > > data members)
> > > 
> > > If so, using PendingRequest in CancelPermissionRequest() seems not right.
> > > 
> > > Did I miss something?
> > > 
> > 
> > This has now changed. Since PermissionManager implentations now return an
int,
> > the request_id is unique with respect to the PermissionManager and
> > PermissionService will use the request_id returned when cancelling.
> > 
> > For context, most of this work is being done so that multiple permissions
can
> be
> > requested at the same time. See https://codereview.chromium.org/1316863010/
> for
> > more info on that.
> 
> My question is
> Can the data members of PendingRequest be used to identify a permission
request?
> (in multiple permission case, using vector<PermissionType>)
> 
> If so, why request id needed?
> 
> If not, the implementation of AwPermissionManager::CancelPermissionRequest(int
> request_id) is wrong.

No these members alone cannot identify a request in the multiple request case.
Theoretically they cannot in the single request case either because there could
be subsequent requests which request the same permission from the same frame.
request_id alone uniquely identifies a request.

Would appreciate advice on how to go about fixing CancelPermissionRequest. I
have not looked into how details of the WebView permission system works so I do
not understand the role of AwBrowserPermissionRequestDelegate etc but pointers
on why it is broken and how I could investigate the fix are much appreciated :)

[michaelbai, 2015-09-23 18:08:05]

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
File android_webview/browser/aw_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:151: int render_frame_id;
On 2015/09/23 18:02:37, Lalit Maganti wrote:
> On 2015/09/23 17:56:26, michaelbai wrote:
> > On 2015/09/23 17:02:07, Lalit Maganti wrote:
> > > On 2015/09/23 16:57:26, michaelbai wrote:
> > > > It seemed that all those data members can not be used to identify the
> > > permission
> > > > request, i.e. different request id could map to same pending request
(have
> > > same
> > > > data members)
> > > > 
> > > > If so, using PendingRequest in CancelPermissionRequest() seems not
right.
> > > > 
> > > > Did I miss something?
> > > > 
> > > 
> > > This has now changed. Since PermissionManager implentations now return an
> int,
> > > the request_id is unique with respect to the PermissionManager and
> > > PermissionService will use the request_id returned when cancelling.
> > > 
> > > For context, most of this work is being done so that multiple permissions
> can
> > be
> > > requested at the same time. See
https://codereview.chromium.org/1316863010/
> > for
> > > more info on that.
> > 
> > My question is
> > Can the data members of PendingRequest be used to identify a permission
> request?
> > (in multiple permission case, using vector<PermissionType>)
> > 
> > If so, why request id needed?
> > 
> > If not, the implementation of
AwPermissionManager::CancelPermissionRequest(int
> > request_id) is wrong.
> 
> No these members alone cannot identify a request in the multiple request case.
> Theoretically they cannot in the single request case either because there
could
> be subsequent requests which request the same permission from the same frame.
> request_id alone uniquely identifies a request.
> 

In the cases, WebView ignore the second one, in which use case, this could
happen?

> Would appreciate advice on how to go about fixing CancelPermissionRequest. I
> have not looked into how details of the WebView permission system works so I
do
> not understand the role of AwBrowserPermissionRequestDelegate etc but pointers
> on why it is broken and how I could investigate the fix are much appreciated
:)

[Lalit Maganti, 2015-09-23 18:20:05]

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
File android_webview/browser/aw_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:151: int render_frame_id;
On 2015/09/23 18:08:05, michaelbai wrote:
> On 2015/09/23 18:02:37, Lalit Maganti wrote:
> > On 2015/09/23 17:56:26, michaelbai wrote:
> > > On 2015/09/23 17:02:07, Lalit Maganti wrote:
> > > > On 2015/09/23 16:57:26, michaelbai wrote:
> > > > > It seemed that all those data members can not be used to identify the
> > > > permission
> > > > > request, i.e. different request id could map to same pending request
> (have
> > > > same
> > > > > data members)
> > > > > 
> > > > > If so, using PendingRequest in CancelPermissionRequest() seems not
> right.
> > > > > 
> > > > > Did I miss something?
> > > > > 
> > > > 
> > > > This has now changed. Since PermissionManager implentations now return
an
> > int,
> > > > the request_id is unique with respect to the PermissionManager and
> > > > PermissionService will use the request_id returned when cancelling.
> > > > 
> > > > For context, most of this work is being done so that multiple
permissions
> > can
> > > be
> > > > requested at the same time. See
> https://codereview.chromium.org/1316863010/
> > > for
> > > > more info on that.
> > > 
> > > My question is
> > > Can the data members of PendingRequest be used to identify a permission
> > request?
> > > (in multiple permission case, using vector<PermissionType>)
> > > 
> > > If so, why request id needed?
> > > 
> > > If not, the implementation of
> AwPermissionManager::CancelPermissionRequest(int
> > > request_id) is wrong.
> > 
> > No these members alone cannot identify a request in the multiple request
case.
> > Theoretically they cannot in the single request case either because there
> could
> > be subsequent requests which request the same permission from the same
frame.
> > request_id alone uniquely identifies a request.
> > 
> 
> In the cases, WebView ignore the second one, in which use case, this could
> happen?
> 
> > Would appreciate advice on how to go about fixing CancelPermissionRequest. I
> > have not looked into how details of the WebView permission system works so I
> do
> > not understand the role of AwBrowserPermissionRequestDelegate etc but
pointers
> > on why it is broken and how I could investigate the fix are much appreciated
> :)
> 
> 
> 

With the permissions api you could have a request for a permission and then
immediately after have another request for the same permission in JavaScript.

[michaelbai, 2015-09-23 18:29:54]

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
File android_webview/browser/aw_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:151: int render_frame_id;
On 2015/09/23 18:20:05, Lalit Maganti wrote:
> On 2015/09/23 18:08:05, michaelbai wrote:
> > On 2015/09/23 18:02:37, Lalit Maganti wrote:
> > > On 2015/09/23 17:56:26, michaelbai wrote:
> > > > On 2015/09/23 17:02:07, Lalit Maganti wrote:
> > > > > On 2015/09/23 16:57:26, michaelbai wrote:
> > > > > > It seemed that all those data members can not be used to identify
the
> > > > > permission
> > > > > > request, i.e. different request id could map to same pending request
> > (have
> > > > > same
> > > > > > data members)
> > > > > > 
> > > > > > If so, using PendingRequest in CancelPermissionRequest() seems not
> > right.
> > > > > > 
> > > > > > Did I miss something?
> > > > > > 
> > > > > 
> > > > > This has now changed. Since PermissionManager implentations now return
> an
> > > int,
> > > > > the request_id is unique with respect to the PermissionManager and
> > > > > PermissionService will use the request_id returned when cancelling.
> > > > > 
> > > > > For context, most of this work is being done so that multiple
> permissions
> > > can
> > > > be
> > > > > requested at the same time. See
> > https://codereview.chromium.org/1316863010/
> > > > for
> > > > > more info on that.
> > > > 
> > > > My question is
> > > > Can the data members of PendingRequest be used to identify a permission
> > > request?
> > > > (in multiple permission case, using vector<PermissionType>)
> > > > 
> > > > If so, why request id needed?
> > > > 
> > > > If not, the implementation of
> > AwPermissionManager::CancelPermissionRequest(int
> > > > request_id) is wrong.
> > > 
> > > No these members alone cannot identify a request in the multiple request
> case.
> > > Theoretically they cannot in the single request case either because there
> > could
> > > be subsequent requests which request the same permission from the same
> frame.
> > > request_id alone uniquely identifies a request.
> > > 
> > 
> > In the cases, WebView ignore the second one, in which use case, this could
> > happen?
> > 
> > > Would appreciate advice on how to go about fixing CancelPermissionRequest.
I
> > > have not looked into how details of the WebView permission system works so
I
> > do
> > > not understand the role of AwBrowserPermissionRequestDelegate etc but
> pointers
> > > on why it is broken and how I could investigate the fix are much
appreciated
> > :)
> > 
> > 
> > 
> 
> With the permissions api you could have a request for a permission and then
> immediately after have another request for the same permission in JavaScript.


It seemed that the requests are actually duplicated, it make no sense to prompt
duplicate request to user in both browser and WebView, I suggest to merge the
duplicate requests in content layer, then, PermissionManager
implementations(browser and webview) don't need to handle the duplicate requests
at all?

[Lalit Maganti, 2015-09-24 09:06:15]

On 2015/09/23 18:29:54, michaelbai wrote:
>
https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
> File android_webview/browser/aw_permission_manager.cc (right):
> 
>
https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
> android_webview/browser/aw_permission_manager.cc:151: int render_frame_id;
> On 2015/09/23 18:20:05, Lalit Maganti wrote:
> > On 2015/09/23 18:08:05, michaelbai wrote:
> > > On 2015/09/23 18:02:37, Lalit Maganti wrote:
> > > > On 2015/09/23 17:56:26, michaelbai wrote:
> > > > > On 2015/09/23 17:02:07, Lalit Maganti wrote:
> > > > > > On 2015/09/23 16:57:26, michaelbai wrote:
> > > > > > > It seemed that all those data members can not be used to identify
> the
> > > > > > permission
> > > > > > > request, i.e. different request id could map to same pending
request
> > > (have
> > > > > > same
> > > > > > > data members)
> > > > > > > 
> > > > > > > If so, using PendingRequest in CancelPermissionRequest() seems not
> > > right.
> > > > > > > 
> > > > > > > Did I miss something?
> > > > > > > 
> > > > > > 
> > > > > > This has now changed. Since PermissionManager implentations now
return
> > an
> > > > int,
> > > > > > the request_id is unique with respect to the PermissionManager and
> > > > > > PermissionService will use the request_id returned when cancelling.
> > > > > > 
> > > > > > For context, most of this work is being done so that multiple
> > permissions
> > > > can
> > > > > be
> > > > > > requested at the same time. See
> > > https://codereview.chromium.org/1316863010/
> > > > > for
> > > > > > more info on that.
> > > > > 
> > > > > My question is
> > > > > Can the data members of PendingRequest be used to identify a
permission
> > > > request?
> > > > > (in multiple permission case, using vector<PermissionType>)
> > > > > 
> > > > > If so, why request id needed?
> > > > > 
> > > > > If not, the implementation of
> > > AwPermissionManager::CancelPermissionRequest(int
> > > > > request_id) is wrong.
> > > > 
> > > > No these members alone cannot identify a request in the multiple request
> > case.
> > > > Theoretically they cannot in the single request case either because
there
> > > could
> > > > be subsequent requests which request the same permission from the same
> > frame.
> > > > request_id alone uniquely identifies a request.
> > > > 
> > > 
> > > In the cases, WebView ignore the second one, in which use case, this could
> > > happen?
> > > 
> > > > Would appreciate advice on how to go about fixing
CancelPermissionRequest.
> I
> > > > have not looked into how details of the WebView permission system works
so
> I
> > > do
> > > > not understand the role of AwBrowserPermissionRequestDelegate etc but
> > pointers
> > > > on why it is broken and how I could investigate the fix are much
> appreciated
> > > :)
> > > 
> > > 
> > > 
> > 
> > With the permissions api you could have a request for a permission and then
> > immediately after have another request for the same permission in
JavaScript.
> 
> 
> It seemed that the requests are actually duplicated, it make no sense to
prompt
> duplicate request to user in both browser and WebView, I suggest to merge the
> duplicate requests in content layer, then, PermissionManager
> implementations(browser and webview) don't need to handle the duplicate
requests
> at all?

It does not feel like it is content's job to deal with this issue. Different
implementations may deal with this in different ways. Opera for example have a
completely different way of dealing with permissions to our system.

Also this problem I describe is not something this CL introduces - it already
exists in the code ATM.

[michaelbai, 2015-09-24 17:55:33]

On 2015/09/24 09:06:15, Lalit Maganti wrote:
> On 2015/09/23 18:29:54, michaelbai wrote:
> >
>
https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
> > File android_webview/browser/aw_permission_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/1342833002/diff/260001/android_webview/browse...
> > android_webview/browser/aw_permission_manager.cc:151: int render_frame_id;
> > On 2015/09/23 18:20:05, Lalit Maganti wrote:
> > > On 2015/09/23 18:08:05, michaelbai wrote:
> > > > On 2015/09/23 18:02:37, Lalit Maganti wrote:
> > > > > On 2015/09/23 17:56:26, michaelbai wrote:
> > > > > > On 2015/09/23 17:02:07, Lalit Maganti wrote:
> > > > > > > On 2015/09/23 16:57:26, michaelbai wrote:
> > > > > > > > It seemed that all those data members can not be used to
identify
> > the
> > > > > > > permission
> > > > > > > > request, i.e. different request id could map to same pending
> request
> > > > (have
> > > > > > > same
> > > > > > > > data members)
> > > > > > > > 
> > > > > > > > If so, using PendingRequest in CancelPermissionRequest() seems
not
> > > > right.
> > > > > > > > 
> > > > > > > > Did I miss something?
> > > > > > > > 
> > > > > > > 
> > > > > > > This has now changed. Since PermissionManager implentations now
> return
> > > an
> > > > > int,
> > > > > > > the request_id is unique with respect to the PermissionManager and
> > > > > > > PermissionService will use the request_id returned when
cancelling.
> > > > > > > 
> > > > > > > For context, most of this work is being done so that multiple
> > > permissions
> > > > > can
> > > > > > be
> > > > > > > requested at the same time. See
> > > > https://codereview.chromium.org/1316863010/
> > > > > > for
> > > > > > > more info on that.
> > > > > > 
> > > > > > My question is
> > > > > > Can the data members of PendingRequest be used to identify a
> permission
> > > > > request?
> > > > > > (in multiple permission case, using vector<PermissionType>)
> > > > > > 
> > > > > > If so, why request id needed?
> > > > > > 
> > > > > > If not, the implementation of
> > > > AwPermissionManager::CancelPermissionRequest(int
> > > > > > request_id) is wrong.
> > > > > 
> > > > > No these members alone cannot identify a request in the multiple
request
> > > case.
> > > > > Theoretically they cannot in the single request case either because
> there
> > > > could
> > > > > be subsequent requests which request the same permission from the same
> > > frame.
> > > > > request_id alone uniquely identifies a request.
> > > > > 
> > > > 
> > > > In the cases, WebView ignore the second one, in which use case, this
could
> > > > happen?
> > > > 
> > > > > Would appreciate advice on how to go about fixing
> CancelPermissionRequest.
> > I
> > > > > have not looked into how details of the WebView permission system
works
> so
> > I
> > > > do
> > > > > not understand the role of AwBrowserPermissionRequestDelegate etc but
> > > pointers
> > > > > on why it is broken and how I could investigate the fix are much
> > appreciated
> > > > :)
> > > > 
> > > > 
> > > > 
> > > 
> > > With the permissions api you could have a request for a permission and
then
> > > immediately after have another request for the same permission in
> JavaScript.
> > 
> > 
> > It seemed that the requests are actually duplicated, it make no sense to
> prompt
> > duplicate request to user in both browser and WebView, I suggest to merge
the
> > duplicate requests in content layer, then, PermissionManager
> > implementations(browser and webview) don't need to handle the duplicate
> requests
> > at all?
> 
> It does not feel like it is content's job to deal with this issue. Different
> implementations may deal with this in different ways. Opera for example have a
> completely different way of dealing with permissions to our system.
> 

I disagree, no browser will like duplicate permission request which is the
misbehavior of website.

> Also this problem I describe is not something this CL introduces - it already
> exists in the code ATM.

[jochen (gone - plz use gerrit), 2015-09-25 11:09:50]

looking at the CL description, what about doing one CL per change you describe?

[Lalit Maganti, 2015-09-25 11:52:38]

On 2015/09/25 11:09:50, jochen wrote:
> looking at the CL description, what about doing one CL per change you
describe?

I could break it up like that, but considering this is just a cleanup CL for
further work it doesn't seem like there is much point. Moreover, splitting the
CLs would mean I would store all the information in request first and then
utilize it in cancel in the next CL. This would cause all context of the second
change to be lost in the first as well as having a very redundant piece of code.

Mounir is speaking with michaelbai@ to resolve his concerns over IM.

[michaelbai, 2015-09-25 16:48:29]

If you don't want to resolve duplicate permission request issue, I am ok, as to
this patch, the issues are

- PendingRequest class, the mapping between request id and PendingRequest object
are in both chrome and webview, they are duplicated,

- WebView does't support the duplicate permission request, the duplicated
request is ignored, this patch didn't cover this case, also please added unit
test for this.

[jochen (gone - plz use gerrit), 2015-09-28 11:19:51]

On 2015/09/25 at 11:52:38, lalitm wrote:
> On 2015/09/25 11:09:50, jochen wrote:
> > looking at the CL description, what about doing one CL per change you
describe?
> 
> I could break it up like that, but considering this is just a cleanup CL for
further work it doesn't seem like there is much point. Moreover, splitting the
CLs would mean I would store all the information in request first and then
utilize it in cancel in the next CL. This would cause all context of the second
change to be lost in the first as well as having a very redundant piece of code.
> 

As is, the CL is rather difficult to review.

[Lalit Maganti, 2015-09-28 13:55:43]

On 2015/09/28 11:19:51, jochen wrote:
> On 2015/09/25 at 11:52:38, lalitm wrote:
> > On 2015/09/25 11:09:50, jochen wrote:
> > > looking at the CL description, what about doing one CL per change you
> describe?
> > 
> > I could break it up like that, but considering this is just a cleanup CL for
> further work it doesn't seem like there is much point. Moreover, splitting the
> CLs would mean I would store all the information in request first and then
> utilize it in cancel in the next CL. This would cause all context of the
second
> change to be lost in the first as well as having a very redundant piece of
code.
> > 
> 
> As is, the CL is rather difficult to review.

jochen@: PTAL now. This CL focuses purely on request adding parameters
michaelbai@: Mounir and I still disagree with your stance. There is no nice
location in content where this code can be extracted. Moreover, when request
multiple arrives, webview and chrome will deal with it in different ways so
should have different PendingRequests as appropriate.

[mlamouri (slow - plz ping), 2015-09-28 14:15:05]

lgtm

https://codereview.chromium.org/1342833002/diff/280001/android_webview/browse...
File android_webview/browser/aw_permission_manager.h (right):

https://codereview.chromium.org/1342833002/diff/280001/android_webview/browse...
android_webview/browser/aw_permission_manager.h:60: static void
OnRequestResponse(
Move that to the .cc file?

https://codereview.chromium.org/1342833002/diff/280001/content/browser/permis...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/1342833002/diff/280001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:133: // Check if the
request still exists. It may have been removed by the
nit: s/may/might/ ?

[Lalit Maganti, 2015-09-28 16:07:02]

https://codereview.chromium.org/1342833002/diff/280001/android_webview/browse...
File android_webview/browser/aw_permission_manager.h (right):

https://codereview.chromium.org/1342833002/diff/280001/android_webview/browse...
android_webview/browser/aw_permission_manager.h:60: static void
OnRequestResponse(
On 2015/09/28 14:15:04, Mounir Lamouri wrote:
> Move that to the .cc file?

Done.

Apologies for missing your earlier comment michaelbai@

https://codereview.chromium.org/1342833002/diff/280001/content/browser/permis...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/1342833002/diff/280001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:133: // Check if the
request still exists. It may have been removed by the
On 2015/09/28 14:15:05, Mounir Lamouri wrote:
> nit: s/may/might/ ?

Done.

[jochen (gone - plz use gerrit), 2015-09-28 17:27:06]

lgtm with nit

https://codereview.chromium.org/1342833002/diff/300001/android_webview/browse...
File android_webview/browser/aw_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/300001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:160: class
AwPermissionManager::PendingRequest {
should be a struct

https://codereview.chromium.org/1342833002/diff/300001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:162:
PendingRequest(PermissionType permission,
please also add a dtor

https://codereview.chromium.org/1342833002/diff/300001/content/public/browser...
File content/public/browser/permission_manager.h (right):

https://codereview.chromium.org/1342833002/diff/300001/content/public/browser...
content/public/browser/permission_manager.h:85: static const int
kNoPendingOperation = -1;
static should come before after typedefs/enums and before everything else

[Lalit Maganti, 2015-09-28 17:59:29]

https://codereview.chromium.org/1342833002/diff/300001/android_webview/browse...
File android_webview/browser/aw_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/300001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:160: class
AwPermissionManager::PendingRequest {
On 2015/09/28 17:27:06, jochen wrote:
> should be a struct

Done.

https://codereview.chromium.org/1342833002/diff/300001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:162:
PendingRequest(PermissionType permission,
On 2015/09/28 17:27:06, jochen wrote:
> please also add a dtor

Done.

https://codereview.chromium.org/1342833002/diff/300001/content/public/browser...
File content/public/browser/permission_manager.h (right):

https://codereview.chromium.org/1342833002/diff/300001/content/public/browser...
content/public/browser/permission_manager.h:85: static const int
kNoPendingOperation = -1;
On 2015/09/28 17:27:06, jochen wrote:
> static should come before after typedefs/enums and before everything else

Done.

[michaelbai, 2015-09-28 22:51:44]

On 2015/09/28 13:55:43, Lalit Maganti wrote:
> On 2015/09/28 11:19:51, jochen wrote:
> > On 2015/09/25 at 11:52:38, lalitm wrote:
> > > On 2015/09/25 11:09:50, jochen wrote:
> > > > looking at the CL description, what about doing one CL per change you
> > describe?
> > > 
> > > I could break it up like that, but considering this is just a cleanup CL
for
> > further work it doesn't seem like there is much point. Moreover, splitting
the
> > CLs would mean I would store all the information in request first and then
> > utilize it in cancel in the next CL. This would cause all context of the
> second
> > change to be lost in the first as well as having a very redundant piece of
> code.
> > > 
> > 
> > As is, the CL is rather difficult to review.
> 
> jochen@: PTAL now. This CL focuses purely on request adding parameters
> michaelbai@: Mounir and I still disagree with your stance. There is no nice
> location in content where this code can be extracted. Moreover, when request
> multiple arrives, webview and chrome will deal with it in different ways so
> should have different PendingRequests as appropriate.

If you think there is no location for this code, we can find one, but if you
expect code is different after multiple request change, then it is fine to have
temporarily duplicate here.

In any case, you still need to handle the duplicate request in your patch for
Android webview and have a unit test for it.

[Lalit Maganti, 2015-09-29 09:08:00]

michaelbai@: I understand your concern about duplicate requests but this is not
a problem I have introduced with this CL. This is an already existing issue
which you even have a comment saying that this is a problem.

I can fix this in another (dependent on this CL) CL but I do not feel that this
CL should be blocked because of that issue.

https://codereview.chromium.org/1342833002/diff/280001/android_webview/browse...
File android_webview/browser/aw_permission_manager.h (right):

https://codereview.chromium.org/1342833002/diff/280001/android_webview/browse...
android_webview/browser/aw_permission_manager.h:60: static void
OnRequestResponse(
On 2015/09/28 16:07:01, Lalit Maganti wrote:
> On 2015/09/28 14:15:04, Mounir Lamouri wrote:
> > Move that to the .cc file?
> 
> Done.
> 
> Apologies for missing your earlier comment michaelbai@

I've had to switch this back because I utilize some private members of
AwPermissionManager so I need it to be a method of the class.

[Lalit Maganti, 2015-09-29 10:36:14]

The CQ bit was checked by lalitm@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-29 10:36:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1342833002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1342833002/360001

[commit-bot: I haz the power, 2015-09-29 10:59:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-29 10:59:21]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[mlamouri (slow - plz ping), 2015-09-29 13:28:16]

michaelbai@, do you think we could unblock this CL so Lalit could land it before
the end of his internship then we could work together on any follow-up?

[michaelbai, 2015-09-29 19:09:37]

Lalit, I am not asking you to fix the existing issue, WebView handles the
duplicate request by ignore it, but the code you added didn't handle this case,
see my comment for the possible solution.

I understand you want to land all your work before the internship ends, but you
don't want to leave issues for others to fix, right? if you can't complete it
right, you might consider to narrow the scope.

Last of all, in the Google, 'the problem already exists' is not the excuse to
not fix it.

Mounir, I don't have time to work on permission stuff in near future, need to
make sure patch not have potential issue, I gave a suggestion, hope it can let
this patch landed soon.

https://codereview.chromium.org/1342833002/diff/360001/android_webview/browse...
File android_webview/browser/aw_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/360001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:190: request_id =
pending_requests_.Add(new PendingRequest(
Could you find out if there is duplicated request in pending_requests_? If so,
then drop this request?

[Lalit Maganti, 2015-10-01 11:12:37]

michaelbai@: done as you have requested so PTAL.

[Lalit Maganti, 2015-10-01 11:15:02]

lalitm@google.com changed reviewers:
- lcwu@chromium.org

[Lalit Maganti, 2015-10-01 11:15:04]

-lcwu1
+halliwell: PTAL at chromecast files

[Lalit Maganti, 2015-10-01 11:15:19]

lalitm@google.com changed reviewers:
+ halliwell@chromium.org

[Lalit Maganti, 2015-10-01 11:15:20]

Actually +halliwell

[halliwell, 2015-10-01 12:31:22]

On 2015/10/01 11:15:20, Lalit Maganti wrote:
> Actually +halliwell

chromecast/ lgtm

[michaelbai, 2015-10-01 17:17:59]

https://codereview.chromium.org/1342833002/diff/380001/android_webview/browse...
File android_webview/browser/aw_permission_manager.cc (right):

https://codereview.chromium.org/1342833002/diff/380001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:177:
callback.Run(content::PERMISSION_STATUS_DENIED);
Deny the request is not good, it will break the current WebView apps, could you 

- still add the request to pendingRequestMap
- not call the delegate
- When the same request responded on OnRequestResponse(), find and respond this
request.

[Lalit Maganti, 2015-10-01 18:16:06]

micahelbai@: have done as suggested and changed the test. PTAL.

[michaelbai, 2015-10-01 18:24:34]

lgtm, thanks

https://codereview.chromium.org/1342833002/diff/400001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwPermissionManagerTest.java
(right):

https://codereview.chromium.org/1342833002/diff/400001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/AwPermissionManagerTest.java:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
Copyright  2015

[Lalit Maganti, 2015-10-02 10:19:54]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-10-02 10:19:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, jochen@chromium.org, halliwell@chromium.org,
michaelbai@chromium.org
Link to the patchset: https://codereview.chromium.org/1342833002/#ps420001
(title: "Address review comment")

[commit-bot: I haz the power, 2015-10-02 10:20:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1342833002/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1342833002/420001

[Lalit Maganti, 2015-10-02 10:20:52]

https://codereview.chromium.org/1342833002/diff/400001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwPermissionManagerTest.java
(right):

https://codereview.chromium.org/1342833002/diff/400001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/AwPermissionManagerTest.java:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/10/01 18:24:34, michaelbai wrote:
> Copyright  2015

Done.

[commit-bot: I haz the power, 2015-10-02 11:34:32]

Committed patchset #22 (id:420001)

[commit-bot: I haz the power, 2015-10-02 11:35:28]

Patchset 22 (id:??) landed as
https://crrev.com/27583e9b340401a935ee45c5110506cc5a858c59
Cr-Commit-Position: refs/heads/master@{#352002}