Add an infobar to resolve android runtime permission mismatches.

Add an infobar to resolve android runtime permission mismatches.

With runtime permissions on android M, Chrome can get into a
mismatched state where a user had previously granted a site
a particular content setting (i.e. location), but then the user
goes into the app info and disable the required permission.

When a user is in this state, we want to prompt an infobar that
notifies the user of this mismatch and allows them to rectify it.

This patch only adds this infobar to geolocation, but it should
also be added to the media permission requests.

BUG=499393
Committed: https://crrev.com/76bf8120ce6c788584212096886036d023d5c178
Cr-Commit-Position: refs/heads/master@{#339939}

[Ted C, 2015-07-16 21:30:18]

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

[Ted C, 2015-07-16 21:30:19]

PTAL -- I will look into adding tests for this, but at least want to get started
on the logic thus far.

[Ted C, 2015-07-17 16:07:57]

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

[commit-bot: I haz the power, 2015-07-17 16:08:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1240923002/1

[commit-bot: I haz the power, 2015-07-17 16:40:32]

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

[commit-bot: I haz the power, 2015-07-17 16:40:32]

Dry run: 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...)

[gone, 2015-07-17 17:41:40]

questions, mainly.

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_permission_context_android.h
(right):

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_permission_context_android.h:65: //
destroyed first, which will invalidate weak pointers
nit: add period.  wrap at 80.

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_update_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:29: <<
"Caller shoudl check ShouldShowPermissionInfobar before creating the "
you shoudl fix this typo

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:40: //
corresponding android permission.
what happens if the infobar is already on screen when the user goes into
settings and removes some more?  do those permissions just not get granted?

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:57:
NOTREACHED();
does a NOTREACHED() crash, or is that only on debug builds?

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:113: 
nit: intentional newline?

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_update_infobar_delegate_android.h
(right):

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_update_infobar_delegate_android.h:37: //
Chrome and the previously granted ContentSettingTypes to a particular site.
// Return whether the runtime permissions currently granted to Chrome by Android
// are compatible with ContentSettingTypes previously granted to a site by the
user.

[Ted C, 2015-07-17 20:37:19]

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_permission_context_android.h
(right):

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_permission_context_android.h:65: //
destroyed first, which will invalidate weak pointers
On 2015/07/17 17:41:39, dfalcantara wrote:
> nit: add period.  wrap at 80.

Done.  Silly me for copying blindly from other infobars.

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_update_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:29: <<
"Caller shoudl check ShouldShowPermissionInfobar before creating the "
On 2015/07/17 17:41:40, dfalcantara wrote:
> you shoudl fix this typo

:-) Dnoe

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:40: //
corresponding android permission.
On 2015/07/17 17:41:39, dfalcantara wrote:
> what happens if the infobar is already on screen when the user goes into
> settings and removes some more?  do those permissions just not get granted?

Fair point.  I removed the trimming here and propagate the full list to
the infobar.  When processing the result in the java delegate it ensures it
has all permissions required.

That way it will return false to the callback if the previously granted
permissions are revoked while the infobar is shown.

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:57:
NOTREACHED();
On 2015/07/17 17:41:40, dfalcantara wrote:
> does a NOTREACHED() crash, or is that only on debug builds?

It is a DCHECK(false), so only debug builds.

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:113: 
On 2015/07/17 17:41:40, dfalcantara wrote:
> nit: intentional newline?

Nope...removed

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_update_infobar_delegate_android.h
(right):

https://codereview.chromium.org/1240923002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_update_infobar_delegate_android.h:37: //
Chrome and the previously granted ContentSettingTypes to a particular site.
On 2015/07/17 17:41:40, dfalcantara wrote:
> // Return whether the runtime permissions currently granted to Chrome by
Android
> // are compatible with ContentSettingTypes previously granted to a site by the
> user.

Done.

[gone, 2015-07-17 20:47:45]

lgtm

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

https://codereview.chromium.org/1240923002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionUpdateInfoBarDelegate.java:109:
notifyPermissionResult();
Guess it's easier to read with single codepath instead of checking grantResults
here?

[Ted C, 2015-07-17 20:55:28]

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

https://codereview.chromium.org/1240923002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionUpdateInfoBarDelegate.java:109:
notifyPermissionResult();
On 2015/07/17 20:47:45, dfalcantara wrote:
> Guess it's easier to read with single codepath instead of checking
grantResults
> here?

Yeah, it also covers the case where you had 3 required permissions, but 1 was
previously granted.  Then we ask android for the other two and in the meantime
the user goes and disables the third.

I don't think it's actually possible, but this covers that case if it ever is.

[gone, 2015-07-17 20:57:03]

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

https://codereview.chromium.org/1240923002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionUpdateInfoBarDelegate.java:109:
notifyPermissionResult();
On 2015/07/17 20:55:28, Ted C wrote:
> On 2015/07/17 20:47:45, dfalcantara wrote:
> > Guess it's easier to read with single codepath instead of checking
> grantResults
> > here?
> 
> Yeah, it also covers the case where you had 3 required permissions, but 1 was
> previously granted.  Then we ask android for the other two and in the meantime
> the user goes and disables the third.
> 
> I don't think it's actually possible, but this covers that case if it ever is.

Acknowledged.

[Ted C, 2015-07-17 21:38:23]

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

[commit-bot: I haz the power, 2015-07-17 21:39:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1240923002/60001

[Ted C, 2015-07-17 21:40:35]

tedchoc@chromium.org changed reviewers:
+ mlamouri@chromium.org, mvanouwerkerk@chromium.org

[Ted C, 2015-07-17 21:40:35]

mlamouri - PTAL permissions/
mvanouwerkerk - PTAL geolocation/

As for testing, it will be much easier to do with a stable M SDK, so I think for
now I will need to land it as is.

[commit-bot: I haz the power, 2015-07-17 22:47:11]

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

[commit-bot: I haz the power, 2015-07-17 22:47:12]

Dry run: This issue passed the CQ dry run.

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

lgtm.

I wish this code was using Permission types instead of ContentSettings types. If
you feel like refactoring a bit, I would appreciate it :)

[Ted C, 2015-07-21 17:21:45]

On 2015/07/20 13:13:09, Mounir Lamouri wrote:
> lgtm.
> 
> I wish this code was using Permission types instead of ContentSettings types.
If
> you feel like refactoring a bit, I would appreciate it :)

Right now the mapping between android settings and chrome anything is through
content settings.  If we want to switch to permissions, I'd rather do that
across
the board instead of just in this one place.

For example, the permission_infobar_delegate.h returns the content setting
instead
of the permission (added by me, so blame me):
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/per...

And the confirm infobar in android uses that to determine permisisons:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

As of now, PermissionType does not understand MICROPHONE or CAMERA.  I don't
know the permission code well enough to know if it should either (or if that
has implications on whether it should be using the permission manager instead
of it's home grown handling).

While I think the refactoring can happen, I think it is out of the scope of this
CL :-)

[Ted C, 2015-07-21 18:39:47]

mvanouwerkerk ping

[Michael van Ouwerkerk, 2015-07-22 09:41:26]

chrome/browser/geolocation/ lgtm

https://codereview.chromium.org/1240923002/diff/60001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/1240923002/diff/60001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context_android.cc:54:
::HandleUpdateAndroidPermissions,
nit: ws++++? not sure what the official way to wrap this is

[Ted C, 2015-07-22 19:04:55]

https://codereview.chromium.org/1240923002/diff/60001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/1240923002/diff/60001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context_android.cc:54:
::HandleUpdateAndroidPermissions,
On 2015/07/22 09:41:26, Michael van Ouwerkerk wrote:
> nit: ws++++? not sure what the official way to wrap this is

Done.  Found a couple examples of this, so I'm convinced.

[Ted C, 2015-07-22 19:05:07]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2015-07-22 19:05:07]

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

[commit-bot: I haz the power, 2015-07-22 19:06:10]

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

[commit-bot: I haz the power, 2015-07-22 19:52:13]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-22 19:52:45]

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