Use vector of PermissionRequest instead of ContentSettingsTypes for GroupedPermissionInfoBarDelegate

Use vector of PermissionRequest instead of ContentSettingsTypes for GroupedPermissionInfoBarDelegate

Plumb in permission requests in grouped permission infobar delegate, instead of
just content setting types.

BUG=606138
Committed: https://crrev.com/ada00c1966ad0607f0fb61c970d9d92713679fb7
Cr-Commit-Position: refs/heads/master@{#425616}

[lshang, 2016-09-14 06:32:30]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 06:32:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 06:53:15]

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

[commit-bot: I haz the power, 2016-09-14 06:53:16]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[lshang, 2016-09-14 07:01:17]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 07:01:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-09-14 07:13:54]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 07:14:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-09-14 07:46:20]

Description was changed from

==========
Use list of PermissionRequest instead of ContentSettingsTypes for
GroupedPermissionInfoBarDelegate


use array of permission requests

BUG=
==========

to

==========
Use vector of PermissionRequest instead of ContentSettingsTypes for
GroupedPermissionInfoBarDelegate

This will allow GroupedPermissionInfoBarDelegate(GPIBD) to manager other 
permission types, and allow MediaStreamInfoBarDelegateAndroid to be merged into 
GPIBD.

BUG=606138
==========

[lshang, 2016-09-14 07:46:35]

Patchset #2 (id:20001) has been deleted

[lshang, 2016-09-14 07:47:45]

lshang@chromium.org changed reviewers:
+ tsergeant@chromium.org

[lshang, 2016-09-14 07:47:46]

PTAL thanks!

[commit-bot: I haz the power, 2016-09-14 08:00:16]

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

[commit-bot: I haz the power, 2016-09-14 08:00:17]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-09-15 01:26:12]

lshang@chromium.org changed reviewers:
+ raymes@chromium.org

[lshang, 2016-09-15 12:57:56]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 12:58:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 13:32:10]

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

[commit-bot: I haz the power, 2016-09-15 13:32:11]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-09-20 06:27:02]

Description was changed from

==========
Use vector of PermissionRequest instead of ContentSettingsTypes for
GroupedPermissionInfoBarDelegate

This will allow GroupedPermissionInfoBarDelegate(GPIBD) to manager other 
permission types, and allow MediaStreamInfoBarDelegateAndroid to be merged into 
GPIBD.

BUG=606138
==========

to

==========
Use vector of PermissionRequest instead of ContentSettingsTypes for
GroupedPermissionInfoBarDelegate

Plumb in permission requests in grouped permission infobar delegate, instead of
just content setting types.

BUG=606138
==========

[lshang, 2016-09-20 06:33:22]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 06:33:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 07:23:41]

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

[commit-bot: I haz the power, 2016-09-20 07:23:42]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2016-09-22 06:53:56]

My comments expand the scope of this CL a bit, sorry!

They were things that probably needed to happen eventually anyway, so hopefully
it won't be too much trouble to do them now.

https://codereview.chromium.org/2339863002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/grouped_permission_infobar_delegate.cc (right):

https://codereview.chromium.org/2339863002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/grouped_permission_infobar_delegate.cc:5: #include
"chrome/browser/media/webrtc/media_stream_devices_controller.h"
Is this include necessary?

https://codereview.chromium.org/2339863002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/grouped_permission_infobar_delegate.cc:16:
ContentSettingsType PermissionRequestTypeToContentType(
I think it would be good to avoid adding this conversion method if possible. It
won't work for media, and PermissionRequestType is currently only used for UMA.

The easiest way would be to add a new method to PermissionRequest like

PermissionRequest::GetContentSettingsTypes()

that returns a vector of the content settings types for by that request.

You can then write an implementation for PermissionRequestImpl and
MediaStreamDevicesController, which are the only interesting cases.

Annoyingly, PermissionRequestImpl currently only has PermissionTypes, not
ContentSettingsTypes. I think it would be fine to write a conversion method in
there temporarily. Longer term, we might be able to convert the Android OS
permissions system to use PermissionTypes instead of ContentSettingsTypes.

https://codereview.chromium.org/2339863002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/grouped_permission_infobar_delegate.cc:92: return
PermissionRequestTypeToContentType(
The way that this method was written doesn't necessarily work once we have
PermissionRequests, since a single PermissionRequest can have multiple content
settings types (media will do this).

PermissionInfoBarDelegate now has a content_settings() method which returns the
full vector of content settings requested by an infobar.

Can we override that method and use it instead? This method is only called from
one place,
https://cs.chromium.org/chromium/src/chrome/browser/ui/android/infobars/group...

[lshang, 2016-09-27 07:19:34]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 07:19:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-09-27 07:21:52]

https://codereview.chromium.org/2339863002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/grouped_permission_infobar_delegate.cc (right):

https://codereview.chromium.org/2339863002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/grouped_permission_infobar_delegate.cc:5: #include
"chrome/browser/media/webrtc/media_stream_devices_controller.h"
On 2016/09/22 06:53:56, tsergeant wrote:
> Is this include necessary?

Removed.

https://codereview.chromium.org/2339863002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/grouped_permission_infobar_delegate.cc:16:
ContentSettingsType PermissionRequestTypeToContentType(
On 2016/09/22 06:53:56, tsergeant wrote:
> I think it would be good to avoid adding this conversion method if possible.
It
> won't work for media, and PermissionRequestType is currently only used for
UMA.
> 
> The easiest way would be to add a new method to PermissionRequest like
> 
> PermissionRequest::GetContentSettingsTypes()
> 
> that returns a vector of the content settings types for by that request.
> 
> You can then write an implementation for PermissionRequestImpl and
> MediaStreamDevicesController, which are the only interesting cases.
> 
> Annoyingly, PermissionRequestImpl currently only has PermissionTypes, not
> ContentSettingsTypes. I think it would be fine to write a conversion method in
> there temporarily. Longer term, we might be able to convert the Android OS
> permissions system to use PermissionTypes instead of ContentSettingsTypes.

I moved this conversion to PermissionRequest, but haven't override it in
media_stream_devices_controller, since I think now is not a proper time to add
support of media in grouped permission infobar delegate. See as below.

https://codereview.chromium.org/2339863002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/grouped_permission_infobar_delegate.cc:92: return
PermissionRequestTypeToContentType(
On 2016/09/22 06:53:56, tsergeant wrote:
> The way that this method was written doesn't necessarily work once we have
> PermissionRequests, since a single PermissionRequest can have multiple content
> settings types (media will do this).
> 
> PermissionInfoBarDelegate now has a content_settings() method which returns
the
> full vector of content settings requested by an infobar.
> 
> Can we override that method and use it instead? This method is only called
from
> one place,
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/android/infobars/group...
> 

I think this CL is not ready for adding support for medias in grouped permission
infobar delegate, as we discussed, since if a request can have multiple content
settings types, we need to figure out what |index/position| means in getters and
ToggleAccept(), which might not be consistent.

[commit-bot: I haz the power, 2016-09-27 08:04:55]

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

[commit-bot: I haz the power, 2016-09-27 08:04:56]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2016-09-28 00:01:07]

lgtm with a comment comment.

https://codereview.chromium.org/2339863002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/grouped_permission_infobar_delegate.cc (right):

https://codereview.chromium.org/2339863002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/grouped_permission_infobar_delegate.cc:92: return
PermissionRequestTypeToContentType(
On 2016/09/27 07:21:52, lshang wrote:
> On 2016/09/22 06:53:56, tsergeant wrote:
> > The way that this method was written doesn't necessarily work once we have
> > PermissionRequests, since a single PermissionRequest can have multiple
content
> > settings types (media will do this).
> > 
> > PermissionInfoBarDelegate now has a content_settings() method which returns
> the
> > full vector of content settings requested by an infobar.
> > 
> > Can we override that method and use it instead? This method is only called
> from
> > one place,
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/android/infobars/group...
> > 
> 
> I think this CL is not ready for adding support for medias in grouped
permission
> infobar delegate, as we discussed, since if a request can have multiple
content
> settings types, we need to figure out what |index/position| means in getters
and
> ToggleAccept(), which might not be consistent. 

Yeah, you're right -- there are complications in the Android layer to do with
using multiple content settings types per request. This seems fine for now.

https://codereview.chromium.org/2339863002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request.h (right):

https://codereview.chromium.org/2339863002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request.h:112: // Used for grouped
permission infobar.
Can you be more specific in this comment?

"Used on Android to determine what Android OS permissions are needed for this
permission request"

[lshang, 2016-09-28 00:45:53]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 00:46:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-09-28 00:47:07]

Thanks! raymes@, could you PTAL?

https://codereview.chromium.org/2339863002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request.h (right):

https://codereview.chromium.org/2339863002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request.h:112: // Used for grouped
permission infobar.
On 2016/09/28 00:01:07, tsergeant wrote:
> Can you be more specific in this comment?
> 
> "Used on Android to determine what Android OS permissions are needed for this
> permission request"

Done with this comment comment XD

[commit-bot: I haz the power, 2016-09-28 01:31:11]

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

[commit-bot: I haz the power, 2016-09-28 01:31:11]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-10-15 06:23:28]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-15 06:23:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-10-15 07:02:09]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-15 07:02:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-10-15 07:02:24]

Patchset #8 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-10-15 08:20:55]

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

[commit-bot: I haz the power, 2016-10-15 08:20:56]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-10-16 23:11:09]

raymes@: depending on https://codereview.chromium.org/2415863002/, could you
take a look at this CL again? thanks!

[raymes, 2016-10-17 02:10:52]

As discussed patchset 6 lgtm. It's unfortunate to expose the ContentSettingsType
but we really need to unify on a single permission type in the code.

[lshang, 2016-10-17 02:52:43]

Patchset #7 (id:140001) has been deleted

[lshang, 2016-10-17 02:52:54]

Patchset #7 (id:180001) has been deleted

[lshang, 2016-10-17 02:53:08]

Patchset #7 (id:200001) has been deleted

[lshang, 2016-10-17 02:55:48]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 02:56:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 02:57:50]

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

[commit-bot: I haz the power, 2016-10-17 02:57:51]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[lshang, 2016-10-17 03:11:12]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 03:11:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 03:57:05]

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

[commit-bot: I haz the power, 2016-10-17 03:57:06]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-10-17 04:46:00]

The CQ bit was checked by lshang@chromium.org

[lshang, 2016-10-17 04:46:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2339863002/#ps240001
(title: "fix patch failure")

[commit-bot: I haz the power, 2016-10-17 04:46:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 04:51:51]

Description was changed from

==========
Use vector of PermissionRequest instead of ContentSettingsTypes for
GroupedPermissionInfoBarDelegate

Plumb in permission requests in grouped permission infobar delegate, instead of
just content setting types.

BUG=606138
==========

to

==========
Use vector of PermissionRequest instead of ContentSettingsTypes for
GroupedPermissionInfoBarDelegate

Plumb in permission requests in grouped permission infobar delegate, instead of
just content setting types.

BUG=606138
==========

[commit-bot: I haz the power, 2016-10-17 04:51:52]

Committed patchset #8 (id:240001)

[commit-bot: I haz the power, 2016-10-17 04:54:28]

Description was changed from

==========
Use vector of PermissionRequest instead of ContentSettingsTypes for
GroupedPermissionInfoBarDelegate

Plumb in permission requests in grouped permission infobar delegate, instead of
just content setting types.

BUG=606138
==========

to

==========
Use vector of PermissionRequest instead of ContentSettingsTypes for
GroupedPermissionInfoBarDelegate

Plumb in permission requests in grouped permission infobar delegate, instead of
just content setting types.

BUG=606138

Committed: https://crrev.com/ada00c1966ad0607f0fb61c970d9d92713679fb7
Cr-Commit-Position: refs/heads/master@{#425616}
==========

[commit-bot: I haz the power, 2016-10-17 04:54:31]

Patchset 8 (id:??) landed as
https://crrev.com/ada00c1966ad0607f0fb61c970d9d92713679fb7
Cr-Commit-Position: refs/heads/master@{#425616}