Add initial support for runtime permissions in android M.

Add initial support for runtime permissions in android M.

After clicking the "Allow" button in the infobars, Chrome will request
the permission if required from Android.

BUG=496447
Committed: https://crrev.com/7e231f0a071edb7181990ada5a115defc4477278
Cr-Commit-Position: refs/heads/master@{#333663}

[Ted C, 2015-06-04 01:04:43]

tedchoc@chromium.org changed reviewers:
+ dfalcantara@chromium.org, pkasting@chromium.org

[Ted C, 2015-06-04 01:04:44]

PTAL

dfalcantara - *
pkasting - components/infobars

First pass before I rope in the remainder of the OWNERS.  This is relatively
isolated to android files which will make cherrypicking to 44 easier.

[Peter Kasting, 2015-06-04 07:08:13]

I don't love the changes to confirm_infobar.cc and wonder if it would be any
cleaner to instead subclass that and override CreateRenderInfoBar for the
different cases different delegates need... but I haven't thought through that
deeply so I don't know if it's sane or even possible.

LGTM

https://codereview.chromium.org/1164973003/diff/1/chrome/browser/content_sett...
File chrome/browser/content_settings/permission_infobar_delegate.h (right):

https://codereview.chromium.org/1164973003/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/permission_infobar_delegate.h:24:
ContentSettingsType content_setting() { return type_; }
Nit: const

https://codereview.chromium.org/1164973003/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/permission_infobar_delegate.h:40:
PermissionInfobarDelegate* AsPermissionInfobarDelegate() override;
Nit: This belongs right after InfoBarDismissed() to match its order in the base
class header files.

https://codereview.chromium.org/1164973003/diff/1/chrome/browser/media/media_...
File chrome/browser/media/media_stream_infobar_delegate.h (right):

https://codereview.chromium.org/1164973003/diff/1/chrome/browser/media/media_...
chrome/browser/media/media_stream_infobar_delegate.h:33: bool
IsRequestingVideoAccess();
Nit: Can these be const?

[gone, 2015-06-04 18:42:04]

lgtm as the first pass.  Can't wait until we can use the real SDK and get rid of
that reflection bit, though.

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

https://codereview.chromium.org/1164973003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:67:
return context.checkPermission(permission, Process.myPid(), Process.myUid())
Does it make sense to use checkCallingPermission or checkCallingOrSelfPermission
here, instead?

https://codereview.chromium.org/1164973003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:112:
new PermissionCallback() {
nit: may be cleaner to define the permission callback right before requesting
the permissions.  it'd save some indenting, at least.

https://codereview.chromium.org/1164973003/diff/1/ui/android/java/src/org/chr...
File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
(right):

https://codereview.chromium.org/1164973003/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:163: public
void requestPermissions(String[] permissions, PermissionCallback callback) {
Maybe TODO to fix this when the M SDK comes out?

https://codereview.chromium.org/1164973003/diff/1/ui/android/java/src/org/chr...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1164973003/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:222: Log.d(TAG, "Can
not request permissions as the context is not an Activity");
*cannot

[Ted C, 2015-06-05 00:57:57]

@pkasting - I agree that the conflation of ConfirmInfobar is a bit unfortunate
but the amount of boiler plate code to make subclasses looked much worse to me.

I think with the work we still need to do for these in M45 and beyond might
justify it in the future though.

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

https://codereview.chromium.org/1164973003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:67:
return context.checkPermission(permission, Process.myPid(), Process.myUid())
On 2015/06/04 18:42:04, dfalcantara wrote:
> Does it make sense to use checkCallingPermission or
checkCallingOrSelfPermission
> here, instead?

This recommends checkSelfPermission, which is not yet available, so I think this
is the same but in a more verbose way.
http://developer.android.com/preview/features/runtime-permissions.html

https://codereview.chromium.org/1164973003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:112:
new PermissionCallback() {
On 2015/06/04 18:42:04, dfalcantara wrote:
> nit: may be cleaner to define the permission callback right before requesting
> the permissions.  it'd save some indenting, at least.

Done.

https://codereview.chromium.org/1164973003/diff/1/chrome/browser/content_sett...
File chrome/browser/content_settings/permission_infobar_delegate.h (right):

https://codereview.chromium.org/1164973003/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/permission_infobar_delegate.h:24:
ContentSettingsType content_setting() { return type_; }
On 2015/06/04 07:08:13, Peter Kasting wrote:
> Nit: const

Done.

https://codereview.chromium.org/1164973003/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/permission_infobar_delegate.h:40:
PermissionInfobarDelegate* AsPermissionInfobarDelegate() override;
On 2015/06/04 07:08:13, Peter Kasting wrote:
> Nit: This belongs right after InfoBarDismissed() to match its order in the
base
> class header files.

Done.  Re-ordered cc to match

https://codereview.chromium.org/1164973003/diff/1/chrome/browser/media/media_...
File chrome/browser/media/media_stream_infobar_delegate.h (right):

https://codereview.chromium.org/1164973003/diff/1/chrome/browser/media/media_...
chrome/browser/media/media_stream_infobar_delegate.h:33: bool
IsRequestingVideoAccess();
On 2015/06/04 07:08:13, Peter Kasting wrote:
> Nit: Can these be const?

Done.

https://codereview.chromium.org/1164973003/diff/1/ui/android/java/src/org/chr...
File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
(right):

https://codereview.chromium.org/1164973003/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:163: public
void requestPermissions(String[] permissions, PermissionCallback callback) {
On 2015/06/04 18:42:04, dfalcantara wrote:
> Maybe TODO to fix this when the M SDK comes out?

Done.

https://codereview.chromium.org/1164973003/diff/1/ui/android/java/src/org/chr...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1164973003/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:222: Log.d(TAG, "Can
not request permissions as the context is not an Activity");
On 2015/06/04 18:42:04, dfalcantara wrote:
> *cannot

Done.

[Ted C, 2015-06-05 01:01:36]

tedchoc@chromium.org changed reviewers:
+ bauerb@chromium.org, tommi@chromium.org

[Ted C, 2015-06-05 01:01:37]

Adding the additional owners:

tommi - chrome/browser/media
bauerb - chrome/browser/content_settings/

[Bernhard Bauer, 2015-06-05 08:34:05]

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

https://codereview.chromium.org/1164973003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:39:
private final int[] mContentSettings;
Add a Javadoc comment what this field means?

https://codereview.chromium.org/1164973003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:54:
assert false
Assert the condition directly?

https://codereview.chromium.org/1164973003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:99:
|| mContentSettings.length == 0 || !isPrimaryButton || getContext() == null) {
The `.length == 0` check isn't really necessary; the code will deal with an
empty list just fine.

https://codereview.chromium.org/1164973003/diff/40001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/omaha/UpdateInfoBarHelper.java
(right):

https://codereview.chromium.org/1164973003/diff/40001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/omaha/UpdateInfoBarHelper.java:79:
OmahaUpdateInfobar updateBar = new OmahaUpdateInfobar(
Nit: This change seems to be purely cosmetic?

https://codereview.chromium.org/1164973003/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/confirm_infobar.cc (right):

https://codereview.chromium.org/1164973003/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/confirm_infobar.cc:83: DCHECK(web_contents);
These DCHECKs aren't necessary: if |web_contents| or |cvc| is null, you will
crash below anyway.

https://codereview.chromium.org/1164973003/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1164973003/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:222: Log.d(TAG,
"Cannot request permissions as the context is not an Activity");
Should this be .d or something else? Maybe even assert?

[Peter Kasting, 2015-06-05 10:33:53]

https://codereview.chromium.org/1164973003/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/confirm_infobar.cc (right):

https://codereview.chromium.org/1164973003/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/confirm_infobar.cc:83: DCHECK(web_contents);
On 2015/06/05 08:34:04, Bernhard Bauer wrote:
> These DCHECKs aren't necessary: if |web_contents| or |cvc| is null, you will
> crash below anyway.

True, but the DCHECKs do serve as documentation to the reader that these must be
non-null and thus, if there _is_ a crash, the fix is not to add a null-check but
rather to fix the reason they were null.

Or at least that's the way I use and read DCHECKs... perhaps not everyone does.

(It's possible it doesn't matter; maybe in this case the same semantic content
is already obvious by inspection.)

[Bernhard Bauer, 2015-06-05 14:26:38]

https://codereview.chromium.org/1164973003/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/confirm_infobar.cc (right):

https://codereview.chromium.org/1164973003/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/confirm_infobar.cc:83: DCHECK(web_contents);
On 2015/06/05 10:33:53, Peter Kasting wrote:
> On 2015/06/05 08:34:04, Bernhard Bauer wrote:
> > These DCHECKs aren't necessary: if |web_contents| or |cvc| is null, you will
> > crash below anyway.
> 
> True, but the DCHECKs do serve as documentation to the reader that these must
be
> non-null and thus, if there _is_ a crash, the fix is not to add a null-check
but
> rather to fix the reason they were null.
> 
> Or at least that's the way I use and read DCHECKs... perhaps not everyone
does.
> 
> (It's possible it doesn't matter; maybe in this case the same semantic content
> is already obvious by inspection.)

In practice what I've seen more often is that the assumption that the web
contents cannot be null turns out to be false, so an early return is added.

I do see your point though -- Ted, if you want to keep the DCHECKs for the
reasons Peter outlined, I won't object.

[Ted C, 2015-06-05 16:35:30]

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

https://codereview.chromium.org/1164973003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:39:
private final int[] mContentSettings;
On 2015/06/05 08:34:04, Bernhard Bauer wrote:
> Add a Javadoc comment what this field means?

Done.

https://codereview.chromium.org/1164973003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:54:
assert false
On 2015/06/05 08:34:04, Bernhard Bauer wrote:
> Assert the condition directly?

Done.

https://codereview.chromium.org/1164973003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:99:
|| mContentSettings.length == 0 || !isPrimaryButton || getContext() == null) {
On 2015/06/05 08:34:04, Bernhard Bauer wrote:
> The `.length == 0` check isn't really necessary; the code will deal with an
> empty list just fine.

Done.

https://codereview.chromium.org/1164973003/diff/40001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/omaha/UpdateInfoBarHelper.java
(right):

https://codereview.chromium.org/1164973003/diff/40001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/omaha/UpdateInfoBarHelper.java:79:
OmahaUpdateInfobar updateBar = new OmahaUpdateInfobar(
On 2015/06/05 08:34:04, Bernhard Bauer wrote:
> Nit: This change seems to be purely cosmetic?

Was cruft left behind from a previous version where I tried passing the window. 
Removed.

https://codereview.chromium.org/1164973003/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/confirm_infobar.cc (right):

https://codereview.chromium.org/1164973003/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/confirm_infobar.cc:83: DCHECK(web_contents);
On 2015/06/05 14:26:38, Bernhard Bauer wrote:
> On 2015/06/05 10:33:53, Peter Kasting wrote:
> > On 2015/06/05 08:34:04, Bernhard Bauer wrote:
> > > These DCHECKs aren't necessary: if |web_contents| or |cvc| is null, you
will
> > > crash below anyway.
> > 
> > True, but the DCHECKs do serve as documentation to the reader that these
must
> be
> > non-null and thus, if there _is_ a crash, the fix is not to add a null-check
> but
> > rather to fix the reason they were null.
> > 
> > Or at least that's the way I use and read DCHECKs... perhaps not everyone
> does.
> > 
> > (It's possible it doesn't matter; maybe in this case the same semantic
content
> > is already obvious by inspection.)
> 
> In practice what I've seen more often is that the assumption that the web
> contents cannot be null turns out to be false, so an early return is added.
> 
> I do see your point though -- Ted, if you want to keep the DCHECKs for the
> reasons Peter outlined, I won't object.

I personally prefer to keep them.  In addition to what Peter said, I find they
make diagnosing developer
fails much clearer (without having to see what version of code they belonged to,
etc...).  Granted CHECKs
are actually better for that in the wild and these are justifiably checkable
conditions since they'll fail anyway
the line below as pointed out.

https://codereview.chromium.org/1164973003/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1164973003/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:222: Log.d(TAG,
"Cannot request permissions as the context is not an Activity");
On 2015/06/05 08:34:04, Bernhard Bauer wrote:
> Should this be .d or something else? Maybe even assert?

I was keeping this in line with the show*Intent methods above, but I think
having an assert here is also good.  I think changing it to warning is also
justified.

[Bernhard Bauer, 2015-06-05 16:45:37]

LGTM, thanks!

[Ted C, 2015-06-08 20:39:06]

tedchoc@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[Ted C, 2015-06-08 20:39:07]

+dalecurtis for media OWNERS

Tommi was OOO and looking for other owners that can review this.

[DaleCurtis, 2015-06-09 19:43:05]

media/ lgtm

[Ted C, 2015-06-09 20:44:33]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2015-06-09 20:44:33]

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

[commit-bot: I haz the power, 2015-06-09 20:46:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164973003/100001

[commit-bot: I haz the power, 2015-06-09 21:57:21]

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

[commit-bot: I haz the power, 2015-06-09 21:57:23]

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...)

[Ted C, 2015-06-09 22:14:45]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2015-06-09 22:14:45]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
pkasting@chromium.org, dfalcantara@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1164973003/#ps100002
(title: "Appease findbugs")

[commit-bot: I haz the power, 2015-06-09 22:16:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164973003/100002

[commit-bot: I haz the power, 2015-06-10 00:31:26]

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

[commit-bot: I haz the power, 2015-06-10 00:31:27]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Ted C, 2015-06-10 00:32:42]

The CQ bit was checked by tedchoc@chromium.org

[commit-bot: I haz the power, 2015-06-10 00:36:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164973003/100002

[commit-bot: I haz the power, 2015-06-10 02:42:25]

Committed patchset #8 (id:100002)

[commit-bot: I haz the power, 2015-06-10 02:43:12]

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