permissions: implement coalesced permissions on Android

permissions: implement coalesced permissions on Android
    
Implement the coalesced permission UI on Android. This nicely
displays a multi permission request in a single infobar rather
than being spread over several space-taking infobars.

This CL is also a part of a group of CLs:
(1) https://codereview.chromium.org/1332293002
(2) https://codereview.chromium.org/1337903002
(3) https://codereview.chromium.org/1332063003 (this)

BUG=516626

[Lalit Maganti, 2015-09-10 20:38:38]

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

[Lalit Maganti, 2015-09-10 20:38:38]

Just FYI. I'm thinking of how best to split it up.

[mlamouri (slow - plz ping), 2015-09-15 13:47:06]

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

[mlamouri (slow - plz ping), 2015-09-15 13:47:07]

Dan, can you PTAL?

[Lalit Maganti, 2015-09-15 16:27:52]

This is still an WIP so just a quick look over to see if I'm going along the
right lines would be good Dan :)

[gone, 2015-09-18 21:02:43]

https://chromiumcodereview.appspot.com/1332063003/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java
(right):

https://chromiumcodereview.appspot.com/1332063003/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:138:
public void setConfirmListener(InfoBarListeners.Confirm confirmListener) {
nit: public -> Javadoc

https://chromiumcodereview.appspot.com/1332063003/diff/80001/chrome/android/j...
File chrome/android/java/res/layout/permission_group_infobar_item.xml (right):

https://chromiumcodereview.appspot.com/1332063003/diff/80001/chrome/android/j...
chrome/android/java/res/layout/permission_group_infobar_item.xml:8:
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
After talking with Rebecca about this, we'll want to generalize this, probably
through the InfoBarLayout.  There's a checkbox in the (currently implemented)
translate infobar that UX is requesting to become a toggle like the permissions
stuff, instead, with the same spacings and such.

https://chromiumcodereview.appspot.com/1332063003/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java
(right):

https://chromiumcodereview.appspot.com/1332063003/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:138:
public void setConfirmListener(InfoBarListeners.Confirm confirmListener) {
why do you need this?  Can't you just pass the PermissionInfobar itself in the
constructor?  is it one of those weird java things where "this" doesn't exist
yet?  can't this protected instead?

https://chromiumcodereview.appspot.com/1332063003/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java
(right):

https://chromiumcodereview.appspot.com/1332063003/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:41:
mNativePointer = 0;
Doesn't make sense to call this destroy() if it's just clearing out a native
pointer.  onNativeDestroyed(), maybe?

[Lalit Maganti, 2015-09-21 11:20:08]

https://codereview.chromium.org/1332063003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java
(right):

https://codereview.chromium.org/1332063003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:138:
public void setConfirmListener(InfoBarListeners.Confirm confirmListener) {
On 2015/09/18 21:02:42, dfalcantara wrote:
> nit: public -> Javadoc

Changed to protected now.

https://codereview.chromium.org/1332063003/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/permission_group_infobar_item.xml (right):

https://codereview.chromium.org/1332063003/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/permission_group_infobar_item.xml:8:
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
On 2015/09/18 21:02:42, dfalcantara wrote:
> After talking with Rebecca about this, we'll want to generalize this, probably
> through the InfoBarLayout.  There's a checkbox in the (currently implemented)
> translate infobar that UX is requesting to become a toggle like the
permissions
> stuff, instead, with the same spacings and such.

Acknowledged.

https://codereview.chromium.org/1332063003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java
(right):

https://codereview.chromium.org/1332063003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:138:
public void setConfirmListener(InfoBarListeners.Confirm confirmListener) {
On 2015/09/18 21:02:42, dfalcantara wrote:
> why do you need this?  Can't you just pass the PermissionInfobar itself in the
> constructor?  is it one of those weird java things where "this" doesn't exist
> yet?  can't this protected instead?

Changed as discussed.

https://codereview.chromium.org/1332063003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java
(right):

https://codereview.chromium.org/1332063003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:41:
mNativePointer = 0;
On 2015/09/18 21:02:43, dfalcantara wrote:
> Doesn't make sense to call this destroy() if it's just clearing out a native
> pointer.  onNativeDestroyed(), maybe?

Done.

[mlamouri (slow - plz ping), 2015-09-23 14:25:48]

From the discussion we had with UX, this should not land, right?

[Lalit Maganti, 2015-09-23 14:26:57]

On 2015/09/23 14:25:48, Mounir Lamouri wrote:
> From the discussion we had with UX, this should not land, right?

Yes. Consider this a WIP which Dan can base his work on.

[gone, 2015-09-24 17:06:25]

+Newton for a question.

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/android/...
File chrome/android/java/res/layout/permission_group_infobar_item.xml (right):

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/android/...
chrome/android/java/res/layout/permission_group_infobar_item.xml:8:
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
Newton: from a generic perspective, is it possible to use a FrameLayout here,
given that the right control might not always be a a Switch?

[newt (away), 2015-09-24 17:30:11]

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

[newt (away), 2015-09-24 17:30:11]

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/android/...
File chrome/android/java/res/layout/permission_group_infobar_item.xml (right):

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/android/...
chrome/android/java/res/layout/permission_group_infobar_item.xml:8:
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
On 2015/09/24 17:06:25, dfalcantara wrote:
> Newton: from a generic perspective, is it possible to use a FrameLayout here,
> given that the right control might not always be a a Switch?

FrameLayout would only work if we knew the exact dimensions of both the icon and
the switch / other view at the end. Otherwise, the FrameLayout can't keep the
TextView from overlapping the switch / other view.

[gone, 2015-09-24 17:32:16]

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/android/...
File chrome/android/java/res/layout/permission_group_infobar_item.xml (right):

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/android/...
chrome/android/java/res/layout/permission_group_infobar_item.xml:8:
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
On 2015/09/24 17:30:11, newt wrote:
> On 2015/09/24 17:06:25, dfalcantara wrote:
> > Newton: from a generic perspective, is it possible to use a FrameLayout
here,
> > given that the right control might not always be a a Switch?
> 
> FrameLayout would only work if we knew the exact dimensions of both the icon
and
> the switch / other view at the end. Otherwise, the FrameLayout can't keep the
> TextView from overlapping the switch / other view.

Yeah, that's what I was worried about.  Thanks!

[gone, 2015-09-25 10:34:43]

It looks like you copy-pasted another class again.  Can you take a pass and
clean up the headers and things that you don't use in the native classes?  It'll
be faster for you to do it with your local build than me eyeballing it.

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/android/...
File chrome/android/java/res/layout/permission_group_infobar_item.xml (right):

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/android/...
chrome/android/java/res/layout/permission_group_infobar_item.xml:12:
android:padding="8dp" >
where'd this 8 come from?

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/browser/...
File chrome/browser/ui/android/infobars/permission_group_infobar_android.cc
(right):

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/browser/...
chrome/browser/ui/android/infobars/permission_group_infobar_android.cc:16:
#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
get rid of the ones you don't use

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/browser/...
File chrome/browser/ui/android/infobars/permission_group_infobar_android.h
(right):

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/browser/...
chrome/browser/ui/android/infobars/permission_group_infobar_android.h:11:
#include "components/content_settings/core/common/content_settings_types.h"
remove necessary headers

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/browser/...
chrome/browser/ui/android/infobars/permission_group_infobar_android.h:18: //
Constructs an AppBannerInfoBarAndroid promoting a native app.
no it doesn't.

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/browser/...
chrome/browser/ui/android/infobars/permission_group_infobar_android.h:34: GURL
requesting_frame;
unused.

[gone, 2015-09-25 11:02:58]

guess another patch set is incoming; will send off whatever comments I've got
and will wait for it.

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/android/...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/android/...
chrome/android/java/strings/android_chrome_strings.grd:2268: <message
name="IDS_COALESCED_PERMISSION_INFOBAR_GEOLOCATION" desc="Description used on
the info bar when the user's location is one of several requested permissions.">
why can't you use the ones that are already in generated_resources.grd, or even
the ones that already exist in this file?

e.g.
chrome/app/generated_resources.grd:        Know your location
chrome/app/generated_resources.grd:          Receive push messages
chrome/app/generated_resources.grd:        MIDI devices full control
chrome/android/java/strings/android_chrome_strings.grd:        Allow full
control of MIDI devices
etc etc

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/app/gene...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/1332063003/diff/140001/chrome/app/gene...
chrome/app/generated_resources.grd:15362: +      <ph name="URL">
one line for everything between <message> and </message>.

[Lalit Maganti, 2015-09-25 13:09:24]

Still not complete (documentation is missing and so are tests) but in a better
state now.

https://codereview.chromium.org/1332063003/diff/140001/chrome/android/java/re...
File chrome/android/java/res/layout/permission_group_infobar_item.xml (right):

https://codereview.chromium.org/1332063003/diff/140001/chrome/android/java/re...
chrome/android/java/res/layout/permission_group_infobar_item.xml:12:
android:padding="8dp" >
On 2015/09/25 10:34:43, dfalcantara wrote:
> where'd this 8 come from?

Arbitrary number I came up with so it would match the mocks.

https://codereview.chromium.org/1332063003/diff/140001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1332063003/diff/140001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2268: <message
name="IDS_COALESCED_PERMISSION_INFOBAR_GEOLOCATION" desc="Description used on
the info bar when the user's location is one of several requested permissions.">
On 2015/09/25 11:02:57, dfalcantara wrote:
> why can't you use the ones that are already in generated_resources.grd, or
even
> the ones that already exist in this file?
> 
> e.g.
> chrome/app/generated_resources.grd:        Know your location
> chrome/app/generated_resources.grd:          Receive push messages
> chrome/app/generated_resources.grd:        MIDI devices full control
> chrome/android/java/strings/android_chrome_strings.grd:        Allow full
> control of MIDI devices
> etc etc

Done.

https://codereview.chromium.org/1332063003/diff/140001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1332063003/diff/140001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15362: +      <ph name="URL">
On 2015/09/25 11:02:58, dfalcantara wrote:
> one line for everything between <message> and </message>.

Done.

https://codereview.chromium.org/1332063003/diff/140001/chrome/browser/ui/andr...
File chrome/browser/ui/android/infobars/permission_group_infobar_android.cc
(right):

https://codereview.chromium.org/1332063003/diff/140001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/permission_group_infobar_android.cc:16:
#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
On 2015/09/25 10:34:43, dfalcantara wrote:
> get rid of the ones you don't use

Done.

https://codereview.chromium.org/1332063003/diff/140001/chrome/browser/ui/andr...
File chrome/browser/ui/android/infobars/permission_group_infobar_android.h
(right):

https://codereview.chromium.org/1332063003/diff/140001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/permission_group_infobar_android.h:11:
#include "components/content_settings/core/common/content_settings_types.h"
On 2015/09/25 10:34:43, dfalcantara wrote:
> remove necessary headers

Done.

https://codereview.chromium.org/1332063003/diff/140001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/permission_group_infobar_android.h:18: //
Constructs an AppBannerInfoBarAndroid promoting a native app.
On 2015/09/25 10:34:43, dfalcantara wrote:
> no it doesn't.

Done.

https://codereview.chromium.org/1332063003/diff/140001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/permission_group_infobar_android.h:34: GURL
requesting_frame;
On 2015/09/25 10:34:43, dfalcantara wrote:
> unused.

Done.

[gone, 2015-09-25 14:41:14]

Removed the [WIP] [DO NOT MERGE] thing, given that we're actually trying to land
this now.

https://chromiumcodereview.appspot.com/1332063003/diff/160001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java
(right):

https://chromiumcodereview.appspot.com/1332063003/diff/160001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:72:
public void onConfirmInfoBarButtonClicked(ConfirmInfoBar infoBar, boolean
confirm) {
I don't think you actually have to do this if you override onButtonClicked()
like TranslateInfoBar does.  Also, maybe assert that confirm is true -- the spec
doesn't have a cancel button.

https://chromiumcodereview.appspot.com/1332063003/diff/160001/chrome/browser/...
File chrome/browser/ui/android/infobars/permission_group_infobar_android.h
(right):

https://chromiumcodereview.appspot.com/1332063003/diff/160001/chrome/browser/...
chrome/browser/ui/android/infobars/permission_group_infobar_android.h:24:
private:
nit: newline before

[gone, 2015-09-25 16:43:52]

https://chromiumcodereview.appspot.com/1332063003/diff/180001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java
(right):

https://chromiumcodereview.appspot.com/1332063003/diff/180001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:21:
*/
nit: fill out this todo at some point

https://chromiumcodereview.appspot.com/1332063003/diff/180001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:40:

nit: remove newline

https://chromiumcodereview.appspot.com/1332063003/diff/180001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:73:
boolean[] checkedState = new boolean[mContentSettings.length];
I think you can override this instead, then get rid of the listener:

@Override
public void onButtonClicked(int action, String actionType) {
  (your code)
  
  super.onButtonClicked(action, actionType);
}

https://chromiumcodereview.appspot.com/1332063003/diff/180001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:103:
return R.drawable.permission_camera;
I think you're supposed to use

default:
    assert false;

https://chromiumcodereview.appspot.com/1332063003/diff/180001/chrome/browser/...
File
chrome/browser/android/permissions/permission_group_infobar_delegate_android.h
(right):

https://chromiumcodereview.appspot.com/1332063003/diff/180001/chrome/browser/...
chrome/browser/android/permissions/permission_group_infobar_delegate_android.h:28:
// TODO(lalitm)
also fill this out

https://chromiumcodereview.appspot.com/1332063003/diff/180001/chrome/browser/...
chrome/browser/android/permissions/permission_group_infobar_delegate_android.h:38:
// For use by PermissionGroupInfoBar.
If you think this comment is necessary, maybe use:

// Used by PermissionGroupInfoBar to do <blah>

https://chromiumcodereview.appspot.com/1332063003/diff/180001/chrome/browser/...
chrome/browser/android/permissions/permission_group_infobar_delegate_android.h:59:
};  // AppBannerInfoBarDelegateAndroid
s / AppBannerInfoBar / PermissionGroupInfoBar / g

https://chromiumcodereview.appspot.com/1332063003/diff/180001/chrome/browser/...
File chrome/browser/ui/android/infobars/permission_group_infobar_android.cc
(right):

https://chromiumcodereview.appspot.com/1332063003/diff/180001/chrome/browser/...
chrome/browser/ui/android/infobars/permission_group_infobar_android.cc:35:
NOTREACHED();
are all of these NOTREACHED() still necessary?

https://chromiumcodereview.appspot.com/1332063003/diff/180001/chrome/browser/...
File chrome/browser/ui/android/infobars/permission_group_infobar_android.h
(right):

https://chromiumcodereview.appspot.com/1332063003/diff/180001/chrome/browser/...
chrome/browser/ui/android/infobars/permission_group_infobar_android.h:14: class
PermissionGroupInfoBarAndroid : public ConfirmInfoBar {
class comment

[Lalit Maganti, 2015-09-28 09:24:02]

https://codereview.chromium.org/1332063003/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java
(right):

https://codereview.chromium.org/1332063003/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:21:
*/
On 2015/09/25 16:43:52, dfalcantara wrote:
> nit: fill out this todo at some point

Done.

https://codereview.chromium.org/1332063003/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:40:

On 2015/09/25 16:43:52, dfalcantara wrote:
> nit: remove newline

Done.

https://codereview.chromium.org/1332063003/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:73:
boolean[] checkedState = new boolean[mContentSettings.length];
On 2015/09/25 16:43:52, dfalcantara wrote:
> I think you can override this instead, then get rid of the listener:
> 
> @Override
> public void onButtonClicked(int action, String actionType) {
>   (your code)
>   
>   super.onButtonClicked(action, actionType);
> }

I think I actually need to override the single parameter version which I've
done.

https://codereview.chromium.org/1332063003/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:103:
return R.drawable.permission_camera;
On 2015/09/25 16:43:52, dfalcantara wrote:
> I think you're supposed to use
> 
> default:
>     assert false;

Done.

https://codereview.chromium.org/1332063003/diff/180001/chrome/browser/android...
File
chrome/browser/android/permissions/permission_group_infobar_delegate_android.h
(right):

https://codereview.chromium.org/1332063003/diff/180001/chrome/browser/android...
chrome/browser/android/permissions/permission_group_infobar_delegate_android.h:28:
// TODO(lalitm)
On 2015/09/25 16:43:52, dfalcantara wrote:
> also fill this out

Done.

https://codereview.chromium.org/1332063003/diff/180001/chrome/browser/android...
chrome/browser/android/permissions/permission_group_infobar_delegate_android.h:38:
// For use by PermissionGroupInfoBar.
On 2015/09/25 16:43:52, dfalcantara wrote:
> If you think this comment is necessary, maybe use:
> 
> // Used by PermissionGroupInfoBar to do <blah>

Removed altogether.

https://codereview.chromium.org/1332063003/diff/180001/chrome/browser/android...
chrome/browser/android/permissions/permission_group_infobar_delegate_android.h:59:
};  // AppBannerInfoBarDelegateAndroid
On 2015/09/25 16:43:52, dfalcantara wrote:
> s / AppBannerInfoBar / PermissionGroupInfoBar / g

Done.

https://codereview.chromium.org/1332063003/diff/180001/chrome/browser/ui/andr...
File chrome/browser/ui/android/infobars/permission_group_infobar_android.cc
(right):

https://codereview.chromium.org/1332063003/diff/180001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/permission_group_infobar_android.cc:35:
NOTREACHED();
On 2015/09/25 16:43:52, dfalcantara wrote:
> are all of these NOTREACHED() still necessary?

Not really. I'll just removed them and collapse into default.

https://codereview.chromium.org/1332063003/diff/180001/chrome/browser/ui/andr...
File chrome/browser/ui/android/infobars/permission_group_infobar_android.h
(right):

https://codereview.chromium.org/1332063003/diff/180001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/permission_group_infobar_android.h:14: class
PermissionGroupInfoBarAndroid : public ConfirmInfoBar {
On 2015/09/25 16:43:52, dfalcantara wrote:
> class comment

Done.

[Lalit Maganti, 2015-09-28 09:28:14]

https://codereview.chromium.org/1332063003/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java
(right):

https://codereview.chromium.org/1332063003/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:72:
public void onConfirmInfoBarButtonClicked(ConfirmInfoBar infoBar, boolean
confirm) {
On 2015/09/25 14:41:14, dfalcantara wrote:
> I don't think you actually have to do this if you override onButtonClicked()
> like TranslateInfoBar does.  Also, maybe assert that confirm is true -- the
spec
> doesn't have a cancel button.

Done as discussed.

https://codereview.chromium.org/1332063003/diff/160001/chrome/browser/ui/andr...
File chrome/browser/ui/android/infobars/permission_group_infobar_android.h
(right):

https://codereview.chromium.org/1332063003/diff/160001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/permission_group_infobar_android.h:24:
private:
On 2015/09/25 14:41:14, dfalcantara wrote:
> nit: newline before

Done.

[gone, 2015-09-28 10:48:43]

lgtming on the basis that this will be landing behind a flag and will be
adjusted for UX's latest mocks before launching officially.

https://chromiumcodereview.appspot.com/1332063003/diff/180001/chrome/browser/...
File chrome/browser/ui/android/infobars/permission_group_infobar_android.cc
(right):

https://chromiumcodereview.appspot.com/1332063003/diff/180001/chrome/browser/...
chrome/browser/ui/android/infobars/permission_group_infobar_android.cc:35:
NOTREACHED();
On 2015/09/28 09:24:02, Lalit Maganti wrote:
> On 2015/09/25 16:43:52, dfalcantara wrote:
> > are all of these NOTREACHED() still necessary?
> 
> Not really. I'll just removed them and collapse into default.

Should be explicit when listing these out.  I think findbugs will complain if
you don't.  You can still collapse them into a bunch of explicitly stated cases
that are NOTREACHED, though.

https://chromiumcodereview.appspot.com/1332063003/diff/260001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java
(right):

https://chromiumcodereview.appspot.com/1332063003/diff/260001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:20:
* Infobar which are shown when multiple permissions are requested at the same
time.
nit: are -> is

https://chromiumcodereview.appspot.com/1332063003/diff/260001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:91:
default:
Checked with Lalit -- apparently these other ones aren't triggerable yet.  I'd
still collapse all of the defined ones into a

case ContentSettingsType.CONTENT_SETTINGS_TYPE_PUSH_MESSAGING:
case ContentSettingsType.CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER:
case ContentSettingsType.CONTENT_SETTINGS_TYPE_DURABLE_STORAGE:
case ContentSettingsType.CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC:
case ContentSettingsType.CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA:
    assert false;
    // return the warning image maybe?
default:
    assert false;

https://chromiumcodereview.appspot.com/1332063003/diff/260001/chrome/browser/...
File
chrome/browser/android/permissions/permission_group_infobar_delegate_android.h
(right):

https://chromiumcodereview.appspot.com/1332063003/diff/260001/chrome/browser/...
chrome/browser/android/permissions/permission_group_infobar_delegate_android.h:28:
// Delegate for infobar which is displayed when multiple permissions are
"requested." pushes you over 80 chars?

[gone, 2015-09-28 10:49:05]

er lgtm-ing?  lgtm?

[Lalit Maganti, 2015-09-28 12:41:25]

https://codereview.chromium.org/1332063003/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java
(right):

https://codereview.chromium.org/1332063003/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:20:
* Infobar which are shown when multiple permissions are requested at the same
time.
On 2015/09/28 10:48:43, dfalcantara wrote:
> nit: are -> is

Hmmmm I would still use are TBH. Anyway, I've reworded to be much clearer.

https://codereview.chromium.org/1332063003/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:91:
default:
On 2015/09/28 10:48:43, dfalcantara wrote:
> Checked with Lalit -- apparently these other ones aren't triggerable yet.  I'd
> still collapse all of the defined ones into a
> 
> case ContentSettingsType.CONTENT_SETTINGS_TYPE_PUSH_MESSAGING:
> case ContentSettingsType.CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER:
> case ContentSettingsType.CONTENT_SETTINGS_TYPE_DURABLE_STORAGE:
> case ContentSettingsType.CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC:
> case ContentSettingsType.CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA:
>     assert false;
>     // return the warning image maybe?
> default:
>     assert false;

Done.

https://codereview.chromium.org/1332063003/diff/260001/chrome/browser/android...
File
chrome/browser/android/permissions/permission_group_infobar_delegate_android.h
(right):

https://codereview.chromium.org/1332063003/diff/260001/chrome/browser/android...
chrome/browser/android/permissions/permission_group_infobar_delegate_android.h:28:
// Delegate for infobar which is displayed when multiple permissions are
On 2015/09/28 10:48:43, dfalcantara wrote:
> "requested." pushes you over 80 chars?

Unfortunately yes :(

https://codereview.chromium.org/1332063003/diff/260001/chrome/browser/ui/andr...
File chrome/browser/ui/android/infobars/permission_group_infobar_android.cc
(right):

https://codereview.chromium.org/1332063003/diff/260001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/permission_group_infobar_android.cc:34:
default:
Added the cases back in as discussed.

[gone, 2015-09-28 12:43:21]

https://codereview.chromium.org/1332063003/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java
(right):

https://codereview.chromium.org/1332063003/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:20:
* Infobar which are shown when multiple permissions are requested at the same
time.
On 2015/09/28 12:41:25, Lalit Maganti wrote:
> On 2015/09/28 10:48:43, dfalcantara wrote:
> > nit: are -> is
> 
> Hmmmm I would still use are TBH. Anyway, I've reworded to be much clearer.

Split personalities then; from your permission_group_infobar_android.h:
 14 // Infobar which is shown when multiple permissions are requested at the
same
 15 // time.

;)

[Lalit Maganti, 2015-09-28 12:59:01]

On 2015/09/28 12:43:21, dfalcantara wrote:
>
https://codereview.chromium.org/1332063003/diff/260001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java
> (right):
> 
>
https://codereview.chromium.org/1332063003/diff/260001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionGroupInfoBarAndroid.java:20:
> * Infobar which are shown when multiple permissions are requested at the same
> time.
> On 2015/09/28 12:41:25, Lalit Maganti wrote:
> > On 2015/09/28 10:48:43, dfalcantara wrote:
> > > nit: are -> is
> > 
> > Hmmmm I would still use are TBH. Anyway, I've reworded to be much clearer.
> 
> Split personalities then; from your permission_group_infobar_android.h:
>  14 // Infobar which is shown when multiple permissions are requested at the
> same
>  15 // time.
> 
> ;)

Now I understand why you don't like extraneous comments :P

Fixed.

[gone, 2016-04-25 17:30:00]

dfalcantara@chromium.org changed reviewers:
- dfalcantara@chromium.org

[mlamouri (slow - plz ping), 2017-05-15 14:33:03]

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

[mlamouri (slow - plz ping), 2017-05-15 14:33:04]

Closing this as it popped back into my review queue.