[Android] Explicitly request all needed runtime permissions.

[Android] Explicitly request all needed runtime permissions.

Previously, we only requested a single permission in
a particular permission group to get access to everything
in the group.  Going forward, it is a best practice to
request them all and rely on the framework to handle
granting.

This changes to requesting the groups to requesting all
needed runtime permissions.

BUG=666603
Review-Url: https://codereview.chromium.org/2682863002
Cr-Commit-Position: refs/heads/master@{#451823}
Committed: https://chromium.googlesource.com/chromium/src/+/653256d7f577693757276034d109ac77de359705

[Ted C, 2017-02-08 00:14:41]

Description was changed from

==========
[Android] Update to requested runtime permissions.

Previously, we only requested a single permission in
a particular permission group to get access to everything
in the group.  Going forward, it is a best practice to
request them all and rely on the framework to handle
granting.

This changes to requesting the groups to requesting all
needed runtime permissions.

BUG=666603
==========

to

==========
[Android] Explicitly request all needed runtime permissions.

Previously, we only requested a single permission in
a particular permission group to get access to everything
in the group.  Going forward, it is a best practice to
request them all and rely on the framework to handle
granting.

This changes to requesting the groups to requesting all
needed runtime permissions.

BUG=666603
==========

[Ted C, 2017-02-08 00:23:24]

tedchoc@chromium.org changed reviewers:
+ benwells@chromium.org, cco3@chromium.org, juncai@chromium.org,
mlamouri@chromium.org, qinmin@chromium.org, raymes@chromium.org

[Ted C, 2017-02-08 00:23:25]

Adding a bunch of folk that have dealt with permissions.  Please take a look and
verify that the explicit permissions requested match the expectation you have.

Take a look at the table here to get an idea of the grouping:
https://developer.android.com/guide/topics/permissions/requesting.html#normal...

In general, the two groups that I see where we were relying on the group
granting logic is location and file access.  Now we need to request COARSE/FINE
explicitly as well as READ/WRITE.

For geolocation, I've updated to request FINE/COARSE.

WebRTC/Media should be unchanged for camera and mic.

When downloading files, it is kept to only request WRITE_EXTERNAL_STORAGE

Reading file:// URLs is changed to READ_EXTERNAL_STORAGE

@juncai - Question regarding bluetooth whether it should be requesting just
COARSE location as it is now, or whether it would use fine as well.

@cco3 - For Physical Web, we are currently relying only on FINE_LOCATION. 
Should that be changed to include FINE and COARSE?

[juncai, 2017-02-08 01:00:03]

Bluetooth should be requesting just COARSE location as it is now.

[cco3, 2017-02-08 01:02:28]

Assuming I've been added for ExternalNavigationDelegateImpl.java, it lgtm, but I
may not be the best to tell.

[raymes, 2017-02-08 02:42:38]

Thanks tedchoc@. Are you able to describe more concretely the impact of this
change for each permission that may be requested?

[Ted C, 2017-02-08 02:58:31]

On 2017/02/08 01:02:28, cco3 wrote:
> Assuming I've been added for ExternalNavigationDelegateImpl.java, it lgtm, but
I
> may not be the best to tell.

@cco3, you were there to clarify that physical web only needs FINE_LOCATION as
requested in:

https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

FINE_LOCATION will no longer imply COARSE_LOCATION, so we need to be
sure we are requesting what is needed.

[Ted C, 2017-02-08 03:01:28]

On 2017/02/08 02:42:38, raymes wrote:
> Thanks tedchoc@. Are you able to describe more concretely the impact of this
> change for each permission that may be requested?

Not much more than beyond what the commit message and first message states.

After runtime permissions were enabled in Android M, they were enabled at the
group level.  That no longer holds going forward.  So if you relied on implicit
permission granting, then that needs to be addressed.

Website geolocation for example can request fine or coarse location at the
api level, but we were only requesting fine before.  So if we started using
coarse granted APIs, they would crash.  Same with the read/write external
storage permission change.

Nothing really more than that.

[qinmin, 2017-02-08 21:59:05]

lgtm

[raymes, 2017-02-09 03:54:21]

Unfortunately I don't know android permissions very well so I don't have much to
add :( The code changes seem to make sense though.

[benwells, 2017-02-09 05:45:46]

On 2017/02/09 03:54:21, raymes wrote:
> Unfortunately I don't know android permissions very well so I don't have much
to
> add :( The code changes seem to make sense though.

The mapping of web permissions to android permission looks good to me. The code
changes also look good but I haven't reviewed in any depth.

[mlamouri (slow - plz ping), 2017-02-10 14:09:04]

https://codereview.chromium.org/2682863002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/2682863002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:784:
if (mWindowAndroid.canRequestPermission(permissionType[i])) {
for readability, what about an early `continue`? Indentation is going crazy :)

https://codereview.chromium.org/2682863002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:790:
&& grantResults[0] == PackageManager.PERMISSION_GRANTED) {
What if there were multiple permissions? Does it still make sense to only look
at the first one?

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

https://codereview.chromium.org/2682863002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:49: if
(!window_android->HasPermission(*it)) {
style/nit:
```
if (window_android->HasPermission(*it))
  continue;
```

https://codereview.chromium.org/2682863002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:61: }
This is a bit confusing: we write a `message_id` by iterating trough the
`permissions` but then, there is: `if (permissions.size() > 1) message_id =
...`. Should we do that check first and only check the permissions otherwise?

[Ted C, 2017-02-10 17:24:46]

https://codereview.chromium.org/2682863002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/2682863002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:784:
if (mWindowAndroid.canRequestPermission(permissionType[i])) {
On 2017/02/10 14:09:04, mlamouri wrote:
> for readability, what about an early `continue`? Indentation is going crazy :)

Done.

https://codereview.chromium.org/2682863002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:790:
&& grantResults[0] == PackageManager.PERMISSION_GRANTED) {
On 2017/02/10 14:09:04, mlamouri wrote:
> What if there were multiple permissions? Does it still make sense to only look
> at the first one?

Nice catch.  Since each row can be tied to multiple permissions, it only makes
sense to update the UI if all of them are granted.  Now it loops and checks
if they're all granted before updating the UI.

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

https://codereview.chromium.org/2682863002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:49: if
(!window_android->HasPermission(*it)) {
On 2017/02/10 14:09:04, mlamouri wrote:
> style/nit:
> ```
> if (window_android->HasPermission(*it))
>   continue;
> ```

Done.

https://codereview.chromium.org/2682863002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:61: }
On 2017/02/10 14:09:04, mlamouri wrote:
> This is a bit confusing: we write a `message_id` by iterating trough the
> `permissions` but then, there is: `if (permissions.size() > 1) message_id =
> ...`. Should we do that check first and only check the permissions otherwise?

Definitely right.  Moved it around a bit to exit early if the missing
permissions
are greater than 1.  Still need to do the outer loop to gather all permissions,
but the inner loop can be skipped in that case.

[mlamouri (slow - plz ping), 2017-02-13 11:58:01]

lgtm

https://codereview.chromium.org/2682863002/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_update_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2682863002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:59:
break;
nit: you could integrate this with the other conditions by doing an `else if`.

https://codereview.chromium.org/2682863002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:60: }
Here, if I understand correctly, you want to double-break, right? Because you
will go back at the beginning of the for loop and `continue` up to the end. I'm
not sure if I understand this correctly but in that case, would it make sense to
`break` after the inner-for to you leave the outer-for and return properly? I've
heard `goto` can help sometimes too :)

[Ted C, 2017-02-14 00:53:33]

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

[Ted C, 2017-02-14 00:53:34]

+tommi for chrome/browser/media/webrtc/media_stream_devices_controller.cc OWNERS

https://codereview.chromium.org/2682863002/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_update_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2682863002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:59:
break;
On 2017/02/13 11:58:01, mlamouri wrote:
> nit: you could integrate this with the other conditions by doing an `else if`.

Done.

https://codereview.chromium.org/2682863002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:60: }
On 2017/02/13 11:58:01, mlamouri wrote:
> Here, if I understand correctly, you want to double-break, right? Because you
> will go back at the beginning of the for loop and `continue` up to the end.
I'm
> not sure if I understand this correctly but in that case, would it make sense
to
> `break` after the inner-for to you leave the outer-for and return properly?
I've
> heard `goto` can help sometimes too :)

Yeah, this is a bit confusing, but it is actually doing what I want here.

The GetAndroidPermissionsForContentSetting above is collecting all the
permissions to request from Android, but the inner loop is just figuring out the
message id to show to the user.

Once there are multiple missing permissions, the inner loop is unnecessary but
the out loop will still collect any and all permissions until all content
settings are visited.

[Ted C, 2017-02-16 21:18:16]

On 2017/02/14 00:53:34, Ted C (OOO till 2.17) wrote:
> +tommi for chrome/browser/media/webrtc/media_stream_devices_controller.cc
OWNERS
> 
>
https://codereview.chromium.org/2682863002/diff/20001/chrome/browser/permissi...
> File chrome/browser/permissions/permission_update_infobar_delegate_android.cc
> (right):
> 
>
https://codereview.chromium.org/2682863002/diff/20001/chrome/browser/permissi...
> chrome/browser/permissions/permission_update_infobar_delegate_android.cc:59:
> break;
> On 2017/02/13 11:58:01, mlamouri wrote:
> > nit: you could integrate this with the other conditions by doing an `else
if`.
> 
> Done.
> 
>
https://codereview.chromium.org/2682863002/diff/20001/chrome/browser/permissi...
> chrome/browser/permissions/permission_update_infobar_delegate_android.cc:60: }
> On 2017/02/13 11:58:01, mlamouri wrote:
> > Here, if I understand correctly, you want to double-break, right? Because
you
> > will go back at the beginning of the for loop and `continue` up to the end.
> I'm
> > not sure if I understand this correctly but in that case, would it make
sense
> to
> > `break` after the inner-for to you leave the outer-for and return properly?
> I've
> > heard `goto` can help sometimes too :)
> 
> Yeah, this is a bit confusing, but it is actually doing what I want here.
> 
> The GetAndroidPermissionsForContentSetting above is collecting all the
> permissions to request from Android, but the inner loop is just figuring out
the
> message id to show to the user.
> 
> Once there are multiple missing permissions, the inner loop is unnecessary but
> the out loop will still collect any and all permissions until all content
> settings are visited.

@tommi - ping

[tommi (sloooow) - chröme, 2017-02-16 21:42:05]

lgtm % fix code to avoid unwanted string copies.

https://codereview.chromium.org/2682863002/diff/60001/chrome/browser/android/...
File chrome/browser/android/preferences/pref_service_bridge.h (right):

https://codereview.chromium.org/2682863002/diff/60001/chrome/browser/android/...
chrome/browser/android/preferences/pref_service_bridge.h:30:
std::vector<std::string>* out);
just return the vector instead?

https://codereview.chromium.org/2682863002/diff/60001/chrome/browser/media/we...
File chrome/browser/media/webrtc/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/2682863002/diff/60001/chrome/browser/media/we...
chrome/browser/media/webrtc/media_stream_devices_controller.cc:617: for
(std::string android_permission : android_permissions) {
please change to |const std::string& android_permission|
(otherwise you create one more copy of the strings)

alternatively you can use auto:

for (const auto& permission : android_permissions) {
...

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

https://codereview.chromium.org/2682863002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:114:
for (auto android_permission : android_permissions) {
|const auto&| to avoid making copies
(please check throughout in case there are more occurrences)

[Ted C, 2017-02-17 21:36:52]

https://codereview.chromium.org/2682863002/diff/60001/chrome/browser/android/...
File chrome/browser/android/preferences/pref_service_bridge.h (right):

https://codereview.chromium.org/2682863002/diff/60001/chrome/browser/android/...
chrome/browser/android/preferences/pref_service_bridge.h:30:
std::vector<std::string>* out);
On 2017/02/16 21:42:04, tommi (krómíum) wrote:
> just return the vector instead?

The permission update infobar accumulates the permissions for multiple content
settings, so it is easier to just build on an existing vector than creating a
new one and adding those back in (at least IMO).

https://codereview.chromium.org/2682863002/diff/60001/chrome/browser/media/we...
File chrome/browser/media/webrtc/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/2682863002/diff/60001/chrome/browser/media/we...
chrome/browser/media/webrtc/media_stream_devices_controller.cc:617: for
(std::string android_permission : android_permissions) {
On 2017/02/16 21:42:04, tommi (krómíum) wrote:
> please change to |const std::string& android_permission|
> (otherwise you create one more copy of the strings)
> 
> alternatively you can use auto:
> 
> for (const auto& permission : android_permissions) {
> ...

Done.

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

https://codereview.chromium.org/2682863002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:114:
for (auto android_permission : android_permissions) {
On 2017/02/16 21:42:04, tommi (krómíum) wrote:
> |const auto&| to avoid making copies
> (please check throughout in case there are more occurrences)

Done.  I did a scan of the change and these two are the only ones I saw.

[Ted C, 2017-02-17 21:38:07]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2017-02-17 21:38:07]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
cco3@chromium.org, mlamouri@chromium.org, tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/2682863002/#ps100001
(title: "Fix string copy issue")

[commit-bot: I haz the power, 2017-02-17 21:39:24]

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, 2017-02-17 23:06:58]

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

[commit-bot: I haz the power, 2017-02-17 23:07:00]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Ted C, 2017-02-18 00:10:56]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2017-02-18 00:10:56]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
mlamouri@chromium.org, qinmin@chromium.org, cco3@chromium.org
Link to the patchset: https://codereview.chromium.org/2682863002/#ps120001
(title: "Fix findbugs")

[commit-bot: I haz the power, 2017-02-18 00:12:28]

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, 2017-02-18 02:16:03]

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

[commit-bot: I haz the power, 2017-02-18 02:16:05]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Ted C, 2017-02-18 03:28:27]

The CQ bit was checked by tedchoc@chromium.org

[commit-bot: I haz the power, 2017-02-18 03:28:52]

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, 2017-02-18 04:15:05]

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

[commit-bot: I haz the power, 2017-02-18 04:15:06]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Ted C, 2017-02-21 05:03:07]

The CQ bit was checked by tedchoc@chromium.org

[commit-bot: I haz the power, 2017-02-21 05:03:37]

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, 2017-02-21 08:08:53]

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

[commit-bot: I haz the power, 2017-02-21 08:08:55]

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

[Ted C, 2017-02-21 18:02:54]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2017-02-21 18:02:54]

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

[commit-bot: I haz the power, 2017-02-21 18:03:30]

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, 2017-02-21 19:44:20]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487700174239520,
"parent_rev": "e451db7ebedfa9a2b43b99f4c789cdde7ca97421", "commit_rev":
"653256d7f577693757276034d109ac77de359705"}

[commit-bot: I haz the power, 2017-02-21 19:46:19]

Description was changed from

==========
[Android] Explicitly request all needed runtime permissions.

Previously, we only requested a single permission in
a particular permission group to get access to everything
in the group.  Going forward, it is a best practice to
request them all and rely on the framework to handle
granting.

This changes to requesting the groups to requesting all
needed runtime permissions.

BUG=666603
==========

to

==========
[Android] Explicitly request all needed runtime permissions.

Previously, we only requested a single permission in
a particular permission group to get access to everything
in the group.  Going forward, it is a best practice to
request them all and rely on the framework to handle
granting.

This changes to requesting the groups to requesting all
needed runtime permissions.

BUG=666603

Review-Url: https://codereview.chromium.org/2682863002
Cr-Commit-Position: refs/heads/master@{#451823}
Committed:
https://chromium.googlesource.com/chromium/src/+/653256d7f577693757276034d109...
==========

[commit-bot: I haz the power, 2017-02-21 19:46:20]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/653256d7f577693757276034d109...