Permissions: Add new Infobar to support grouped permission requests on Android

Permissions: Add new InfoBar to support grouped permission requests on Android

The infobar, GroupedPermissionInfoBar, is intended to eventually replace
PermissionInfoBarDelegate. As an initial step, this CL adds the InfoBar
and a simple InfoBarDelegate, and hooks them up so they can be used
within MediaStream permission requests.

BUG=458606
Committed: https://crrev.com/cb9fb087006240580dd818a810f36a55a5c42079
Cr-Commit-Position: refs/heads/master@{#395282}

[tsergeant, 2016-04-14 06:10:18]

Description was changed from

==========
Permissions: Add new Infobar to support grouped permission requests on Android

The Infobar, GroupedPermissionInfoBar, is intended to eventually replace
PermissionInfoBarDelegate. As an initial step, this CL adds the InfoBar
and a simple InfoBarDelegate, and hooks them up so they can be used
within MediaStream permission requests.

WIP, Do Not Submit

BUG=458606
==========

to

==========
Permissions: Add new InfoBar to support grouped permission requests on Android

The infobar, GroupedPermissionInfoBar, is intended to eventually replace
PermissionInfoBarDelegate. As an initial step, this CL adds the InfoBar
and a simple InfoBarDelegate, and hooks them up so they can be used
within MediaStream permission requests.

WIP, Do Not Submit

BUG=458606
==========

[tsergeant, 2016-04-14 06:31:51]

Description was changed from

==========
Permissions: Add new InfoBar to support grouped permission requests on Android

The infobar, GroupedPermissionInfoBar, is intended to eventually replace
PermissionInfoBarDelegate. As an initial step, this CL adds the InfoBar
and a simple InfoBarDelegate, and hooks them up so they can be used
within MediaStream permission requests.

WIP, Do Not Submit

BUG=458606
==========

to

==========
Permissions: Add new InfoBar to support grouped permission requests on Android

The infobar, GroupedPermissionInfoBar, is intended to eventually replace
PermissionInfoBarDelegate. As an initial step, this CL adds the InfoBar
and a simple InfoBarDelegate, and hooks them up so they can be used
within MediaStream permission requests.

BUG=458606
==========

[tsergeant, 2016-04-14 06:33:34]

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

[tsergeant, 2016-04-14 06:33:35]

miguelg@, can you please take a look?

[tsergeant, 2016-04-14 07:09:06]

Patchset #2 (id:20001) has been deleted

[tsergeant, 2016-04-14 07:09:17]

Patchset #3 (id:60001) has been deleted

[tsergeant, 2016-04-14 07:09:26]

Patchset #2 (id:40001) has been deleted

[Miguel Garcia, 2016-04-14 11:50:49]

miguelg@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[Miguel Garcia, 2016-04-14 11:50:49]

Dan Alcantara should probably be the one reviewing this since he was working in
this area. 

Mounir is probably interested too so CCing him as well.

[gone, 2016-04-19 19:54:32]

Mainly questions mixed with nits.

https://codereview.chromium.org/1889643003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java
(right):

https://codereview.chromium.org/1889643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:31:
for (int i = 0; i < mPermissionIcons.length; i++) {
If I'm reading this right, this is only adding the icon for multiple permissions
instead of toggles.  Is there a follow up CL to make this match the screenshots
you posted?

https://codereview.chromium.org/1889643003/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/grouped_permission_infobar_delegate.cc (right):

https://codereview.chromium.org/1889643003/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/grouped_permission_infobar_delegate.cc:65: } else {
Is this something that should be DCHECKED?  If an infobar appears asking for a
permission but there's no text to display, it seems like something that
shouldn't be allowed to go through.

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

https://codereview.chromium.org/1889643003/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/grouped_permission_infobar_delegate.h:52: GURL
requesting_origin_;
Also const?

https://codereview.chromium.org/1889643003/diff/100001/chrome/browser/ui/andr...
File chrome/browser/ui/android/infobars/grouped_permission_infobar.cc (right):

https://codereview.chromium.org/1889643003/diff/100001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/grouped_permission_infobar.cc:62: //
GroupedPermissionInfoBarDelegate:
Don't think this comment is necessary.

[tsergeant, 2016-04-20 07:26:17]

https://codereview.chromium.org/1889643003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java
(right):

https://codereview.chromium.org/1889643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:31:
for (int i = 0; i < mPermissionIcons.length; i++) {
On 2016/04/19 19:54:32, dfalcantara wrote:
> If I'm reading this right, this is only adding the icon for multiple
permissions
> instead of toggles.  Is there a follow up CL to make this match the
screenshots
> you posted?

Yup, this CL only displays a list of permissions. I'm mostly done on a follow-up
with the toggles.

https://codereview.chromium.org/1889643003/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/grouped_permission_infobar_delegate.cc (right):

https://codereview.chromium.org/1889643003/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/grouped_permission_infobar_delegate.cc:65: } else {
On 2016/04/19 19:54:32, dfalcantara wrote:
> Is this something that should be DCHECKED?  If an infobar appears asking for a
> permission but there's no text to display, it seems like something that
> shouldn't be allowed to go through.

Good point, done.

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

https://codereview.chromium.org/1889643003/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/grouped_permission_infobar_delegate.h:52: GURL
requesting_origin_;
On 2016/04/19 19:54:32, dfalcantara wrote:
> Also const?

Done.

https://codereview.chromium.org/1889643003/diff/100001/chrome/browser/ui/andr...
File chrome/browser/ui/android/infobars/grouped_permission_infobar.cc (right):

https://codereview.chromium.org/1889643003/diff/100001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/grouped_permission_infobar.cc:62: //
GroupedPermissionInfoBarDelegate:
On 2016/04/19 19:54:32, dfalcantara wrote:
> Don't think this comment is necessary.

Done.

[gone, 2016-04-20 17:11:21]

lgtm

https://codereview.chromium.org/1889643003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java
(right):

https://codereview.chromium.org/1889643003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:31:
for (int i = 0; i < mPermissionIcons.length; i++) {
On 2016/04/20 07:26:17, tsergeant wrote:
> On 2016/04/19 19:54:32, dfalcantara wrote:
> > If I'm reading this right, this is only adding the icon for multiple
> permissions
> > instead of toggles.  Is there a follow up CL to make this match the
> screenshots
> > you posted?
> 
> Yup, this CL only displays a list of permissions. I'm mostly done on a
follow-up
> with the toggles.

Acknowledged.

[tsergeant, 2016-04-21 06:31:47]

Patchset #5 (id:140001) has been deleted

[tsergeant, 2016-04-21 07:24:43]

miguelg@, do you mind taking a look at
chrome/browser/permissions/grouped_permission_infobar_delegate.h/cc

[tsergeant, 2016-04-27 21:36:46]

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

[tsergeant, 2016-04-27 21:36:47]

+mlamouri@, can you please review c/b/permissions?

[mlamouri (slow - plz ping), 2016-04-28 13:06:49]

lgtm

https://codereview.chromium.org/1889643003/diff/160001/chrome/browser/ui/andr...
File chrome/browser/ui/android/infobars/grouped_permission_infobar.cc (right):

https://codereview.chromium.org/1889643003/diff/160001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/grouped_permission_infobar.cc:18:
GroupedPermissionInfoBar::~GroupedPermissionInfoBar() {
= default;

You might even be able to move that to the header.

[tsergeant, 2016-05-17 04:23:16]

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

[tsergeant, 2016-05-17 04:23:17]

This got shelved while I was travelling, current plan is not to land it until
after branch point.

+tommi@ for media owners in the meantime.

[tommi (sloooow) - chröme, 2016-05-17 14:14:30]

lgtm

[tsergeant, 2016-05-23 01:23:31]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2016-05-23 01:23:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1889643003/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-23 01:23:39]

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

[commit-bot: I haz the power, 2016-05-23 03:10:26]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-05-23 03:11:26]

Description was changed from

==========
Permissions: Add new InfoBar to support grouped permission requests on Android

The infobar, GroupedPermissionInfoBar, is intended to eventually replace
PermissionInfoBarDelegate. As an initial step, this CL adds the InfoBar
and a simple InfoBarDelegate, and hooks them up so they can be used
within MediaStream permission requests.

BUG=458606
==========

to

==========
Permissions: Add new InfoBar to support grouped permission requests on Android

The infobar, GroupedPermissionInfoBar, is intended to eventually replace
PermissionInfoBarDelegate. As an initial step, this CL adds the InfoBar
and a simple InfoBarDelegate, and hooks them up so they can be used
within MediaStream permission requests.

BUG=458606

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

[commit-bot: I haz the power, 2016-05-23 03:11:27]

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