|
|
Created:
3 years, 10 months ago by Ted C Modified:
3 years, 10 months ago Reviewers:
benwells, tommi (sloooow) - chröme, mlamouri (slow - plz ping), juncai, qinmin, raymes, cco3 CC:
agrieve+watch_chromium.org, chfremer+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, jdonnelly+watch_chromium.org, mcasas+watch+vc_chromium.org, mlamouri+watch-permissions_chromium.org, raymes+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[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
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address mlamouri@ comments #
Total comments: 4
Patch Set 3 : Rebase #Patch Set 4 : Nit #
Total comments: 6
Patch Set 5 : Rebase #Patch Set 6 : Fix string copy issue #Patch Set 7 : Fix findbugs #Patch Set 8 : Rebase #Messages
Total messages: 43 (17 generated)
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 ==========
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?
Bluetooth should be requesting just COARSE location as it is now.
Assuming I've been added for ExternalNavigationDelegateImpl.java, it lgtm, but I may not be the best to tell.
Thanks tedchoc@. Are you able to describe more concretely the impact of this change for each permission that may be requested?
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.
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.
lgtm
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.
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.
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?
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.
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 :)
tedchoc@chromium.org changed reviewers: + tommi@chromium.org
+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.
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
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)
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.
The CQ bit was checked by tedchoc@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by tedchoc@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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)
The CQ bit was checked by tedchoc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by tedchoc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by tedchoc@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487700174239520, "parent_rev": "e451db7ebedfa9a2b43b99f4c789cdde7ca97421", "commit_rev": "653256d7f577693757276034d109ac77de359705"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/653256d7f577693757276034d109... |