browser: implement multiple permission requesting

Implement browser side of multiple permissions request.

This CL implements navigator.permissions.request() with
multiple permissions on the browser side.

This involves changing permission_service and
permission_manager to store infomation about multiple
requests and dispatch callbacks as appropriate.

It also tries to reduce the amount of new code by
attempting to unify the single and multiple codepaths in
permission manager.

Finally, it updates the test classes to match the interface
updates.

BUG=516626

[Lalit Maganti, 2015-09-04 14:47:37]

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

[mlamouri (slow - plz ping), 2015-09-08 10:11:20]

Could you move out the CancelPermissionRequest() out of this CL? I think it
might be bigger than you think (you will have to change aw_permission_manager
for example). At a first glance, doing that change should be fine because we can
require request_id to be unique per RFH but I think it would make the reviews
way simpler if the change was separate from the RequestPermissions() one.

[Lalit Maganti, 2015-09-10 17:51:54]

Patchset #5 (id:80001) has been deleted

[Lalit Maganti, 2015-09-10 17:52:01]

Patchset #4 (id:60001) has been deleted

[mlamouri (slow - plz ping), 2015-09-16 15:08:10]

It seems that this CL is not fully rebase.

I had a look at everything but the Chrome's PermissionManager implementation.
Can you ping me after you applied these comments and rebased the CL?

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

https://codereview.chromium.org/1316863010/diff/140001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:42: void
PermissionRequestResponseCallbackWrapper(
Could you add a comment explaining why you need that?

https://codereview.chromium.org/1316863010/diff/140001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:114:
base::Bind(&PermissionRequestResponseCallbackWrapper,
Can you add a comment explaining why you are doing that?

https://codereview.chromium.org/1316863010/diff/140001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:133:
callback.Run(mojo::Array<PermissionStatus>());
Is that even possible?

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

https://codereview.chromium.org/1316863010/diff/140001/content/browser/permis...
content/browser/permissions/permission_service_impl.h:17: class
PermissionPendingRequest;
Hmm, I'm sure this is a stupid question but... what's that?

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

https://codereview.chromium.org/1316863010/diff/140001/content/public/browser...
content/public/browser/permission_manager.h:41: // CancelPermissionRequest).
nit: mention the NO_PENDING_REQUEST constant.

Also, mention that the vector<PermissionStatus> will be ordered the same way as
the vector<PermissionType>

https://codereview.chromium.org/1316863010/diff/140001/content/public/browser...
content/public/browser/permission_manager.h:48: const
std::vector<PermissionStatus>&)>& callback) = 0;
You are still missing the update to aw_permission_manager.{h,cc}

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

https://codereview.chromium.org/1316863010/diff/140001/content/shell/browser/...
content/shell/browser/shell_permission_manager.cc:44: return -1;
nit: you will need to update that and the ones above

[Lalit Maganti, 2015-09-16 17:21:19]

We need to discuss AwPermisisonManager and whether unifying the paths in
service_impl is the correct way to do things.

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

https://codereview.chromium.org/1316863010/diff/140001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:42: void
PermissionRequestResponseCallbackWrapper(
On 2015/09/16 15:08:09, Mounir Lamouri wrote:
> Could you add a comment explaining why you need that?

Done.

https://codereview.chromium.org/1316863010/diff/140001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:114:
base::Bind(&PermissionRequestResponseCallbackWrapper,
On 2015/09/16 15:08:09, Mounir Lamouri wrote:
> Can you add a comment explaining why you are doing that?

Done.

https://codereview.chromium.org/1316863010/diff/140001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:133:
callback.Run(mojo::Array<PermissionStatus>());
On 2015/09/16 15:08:09, Mounir Lamouri wrote:
> Is that even possible?

A rouge renderer I guess?

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

https://codereview.chromium.org/1316863010/diff/140001/content/browser/permis...
content/browser/permissions/permission_service_impl.h:17: class
PermissionPendingRequest;
On 2015/09/16 15:08:09, Mounir Lamouri wrote:
> Hmm, I'm sure this is a stupid question but... what's that?

Not stupid at all. It's a very old class which no longer exists. Forgot to
remove it.

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

https://codereview.chromium.org/1316863010/diff/140001/content/public/browser...
content/public/browser/permission_manager.h:41: // CancelPermissionRequest).
On 2015/09/16 15:08:09, Mounir Lamouri wrote:
> nit: mention the NO_PENDING_REQUEST constant.
> 
> Also, mention that the vector<PermissionStatus> will be ordered the same way
as
> the vector<PermissionType>

Done.

https://codereview.chromium.org/1316863010/diff/140001/content/public/browser...
content/public/browser/permission_manager.h:48: const
std::vector<PermissionStatus>&)>& callback) = 0;
On 2015/09/16 15:08:09, Mounir Lamouri wrote:
> You are still missing the update to aw_permission_manager.{h,cc}

Good point. Should I implement it properly? If so I'll have to understand how
their whole system works.

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

https://codereview.chromium.org/1316863010/diff/140001/content/shell/browser/...
content/shell/browser/shell_permission_manager.cc:44: return -1;
On 2015/09/16 15:08:10, Mounir Lamouri wrote:
> nit: you will need to update that and the ones above

Done.

[mlamouri (slow - plz ping), 2015-09-23 16:37:37]

I have a few comments about the Chrome's PermissionManager implementation:
- can you remove the Push/Notification workarounds, we could add that later if
needed but at the moment, let's not make this code more complex than it needs to
be;
- can you remove all the side effects from PendingRequest::SetPermission(), it
is calling the callback and returning whether the object should be removed, it
should only set the permission;
- can you use a multimap for permissions so you have the list of permissions as
key and you can iterate trough the indexes in case of duplicate. I think that
would make some code more readable and reduce the complexity (which is currently
n^2).

Otherwise, the CL looks great! :)

(Oh, and, could you add tests? Maybe for bubble manager?)

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

https://codereview.chromium.org/1316863010/diff/360001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:244: return
kNoPendingRequestOrSubscription;
Could you add NOTIMPLEMENTED()?

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

https://codereview.chromium.org/1316863010/diff/360001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:99: DCHECK(vector.size() == 1);
nit: DCHECK_EQ()

https://codereview.chromium.org/1316863010/diff/360001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:159: pending -= 1;
nit: --pending;

https://codereview.chromium.org/1316863010/diff/360001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:170: size_t pending;
nit: pending_results?

https://codereview.chromium.org/1316863010/diff/360001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_id.h (right):

https://codereview.chromium.org/1316863010/diff/360001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_id.h:14: } // namespace content
style: two spaces between '}' and '// namespace content'.

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

https://codereview.chromium.org/1316863010/diff/360001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:47: DCHECK(vector.size()
== 1);
nit: use DCHECK_EQ()

https://codereview.chromium.org/1316863010/diff/360001/content/browser/permis...
File content/browser/permissions/permission_service_impl.h (left):

https://codereview.chromium.org/1316863010/diff/360001/content/browser/permis...
content/browser/permissions/permission_service_impl.h:29: 
nit: do not remove this line.

[michaelbai, 2015-09-23 17:51:51]

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

[michaelbai, 2015-09-23 17:51:52]

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

https://codereview.chromium.org/1316863010/diff/360001/content/public/browser...
content/public/browser/permission_manager.h:55: const
std::vector<PermissionStatus>&)>& callback) = 0;
Are you going to combine RequestPermission into RequestPermissions? they are
same if vector<> only has one permission.

[Lalit Maganti, 2015-09-23 17:57:25]

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

https://codereview.chromium.org/1316863010/diff/360001/content/public/browser...
content/public/browser/permission_manager.h:55: const
std::vector<PermissionStatus>&)>& callback) = 0;
On 2015/09/23 17:51:52, michaelbai wrote:
> Are you going to combine RequestPermission into RequestPermissions? they are
> same if vector<> only has one permission.

Yes you're right but no we are not going to do this. We did consider it but we
decided against forcing implementations from supporting a multiple permission
request if they did not want to.

Mounir: anything to add from your end here?

[Lalit Maganti, 2015-09-24 09:24:11]

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

https://codereview.chromium.org/1316863010/diff/360001/android_webview/browse...
android_webview/browser/aw_permission_manager.cc:244: return
kNoPendingRequestOrSubscription;
On 2015/09/23 16:37:37, Mounir Lamouri wrote:
> Could you add NOTIMPLEMENTED()?

Done.

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

https://codereview.chromium.org/1316863010/diff/360001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:99: DCHECK(vector.size() == 1);
On 2015/09/23 16:37:37, Mounir Lamouri wrote:
> nit: DCHECK_EQ()

Done.

https://codereview.chromium.org/1316863010/diff/360001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:159: pending -= 1;
On 2015/09/23 16:37:37, Mounir Lamouri wrote:
> nit: --pending;

All changed.

https://codereview.chromium.org/1316863010/diff/360001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:170: size_t pending;
On 2015/09/23 16:37:37, Mounir Lamouri wrote:
> nit: pending_results?

Gone.

https://codereview.chromium.org/1316863010/diff/360001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_id.h (right):

https://codereview.chromium.org/1316863010/diff/360001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_id.h:14: } // namespace content
On 2015/09/23 16:37:37, Mounir Lamouri wrote:
> style: two spaces between '}' and '// namespace content'.

Done.

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

https://codereview.chromium.org/1316863010/diff/360001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:47: DCHECK(vector.size()
== 1);
On 2015/09/23 16:37:37, Mounir Lamouri wrote:
> nit: use DCHECK_EQ()

Done.

https://codereview.chromium.org/1316863010/diff/360001/content/browser/permis...
File content/browser/permissions/permission_service_impl.h (left):

https://codereview.chromium.org/1316863010/diff/360001/content/browser/permis...
content/browser/permissions/permission_service_impl.h:29: 
On 2015/09/23 16:37:37, Mounir Lamouri wrote:
> nit: do not remove this line.

Done.

[mlamouri (slow - plz ping), 2015-09-28 14:06:22]

This CL is blocked by https://codereview.chromium.org/1342833002 so I will keep
it aside.

[mlamouri (slow - plz ping), 2015-10-05 16:02:00]

I think this CL needs to be rebased on top of the CancelPermissionRequest()
changes.

[mlamouri (slow - plz ping), 2015-10-23 13:58:13]

On 2015/10/05 at 16:02:00, Mounir Lamouri wrote:
> I think this CL needs to be rebased on top of the CancelPermissionRequest()
changes.

I sent a rebased and modified version of this CL here:
https://codereview.chromium.org/1419083002

[mlamouri (slow - plz ping), 2015-10-24 06:30:25]

On 2015/10/23 at 13:58:13, Mounir Lamouri wrote:
> On 2015/10/05 at 16:02:00, Mounir Lamouri wrote:
> > I think this CL needs to be rebased on top of the CancelPermissionRequest()
changes.
> 
> I sent a rebased and modified version of this CL here:
https://codereview.chromium.org/1419083002

My CL above has landed. Closing this one.

[Lalit Maganti (personal), 2015-10-24 07:18:42]

On 2015/10/24 06:30:25, Mounir Lamouri wrote:
> On 2015/10/23 at 13:58:13, Mounir Lamouri wrote:
> > On 2015/10/05 at 16:02:00, Mounir Lamouri wrote:
> > > I think this CL needs to be rebased on top of the
CancelPermissionRequest()
> changes.
> > 
> > I sent a rebased and modified version of this CL here:
> https://codereview.chromium.org/1419083002
> 
> My CL above has landed. Closing this one.

Glad to see it in :)

I'm going to rebase my Infobar Manager one too so that some progress can be made
on Android as well.