Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(170)

Issue 2226633002: Add a feature to display a persistence toggle for permission prompts on Android. (Closed)

Created:
4 years, 4 months ago by dominickn
Modified:
4 years, 4 months ago
CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, dfalcantara+watch_chromium.org, dewittj
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add a feature to display a persistence toggle for permission prompts on Android. This CL adds a disabled by default feature which shows a "Remember my decision" toggle on permission prompts on Android. This is designed to test whether a more explicit representation of permission persistence (and better control over it) will encourage users to make decisions on these prompts more often. It is intended to experiment with this feature from M54. A new PermissionInfoBar class is added to implement the optional toggle. Permission-specific code in ConfirmInfoBar is moved into the new class, which will also be beneficial in the future if permission prompts stop being infobars. The permission decision is changed from a boolean allowed/not allowed to the PermissionAction enum already used for permission metrics reporting. New metrics to measure the toggle position of prompts which are granted or denied are also added in this CL to measure whether or not users actually interact with the toggle (or if merely seeing it is sufficient to change the wider permission action metrics). The toggle is explicitly disabled for all permissions except geolocation, which currently generates the most prompts on Android. A subsequent CL will implement a corresponding checkbox for desktop permission prompts using the same feature. The desktop permission decision will also be changed to a PermissionAction. BUG=632269 Committed: https://crrev.com/6957a9ca3f6db210572d6b54dcbcf69baeb978fd Cr-Commit-Position: refs/heads/master@{#412177}

Patch Set 1 #

Total comments: 1

Patch Set 2 : Split out implementation into a PermissionInfoBar #

Patch Set 3 : Remove a bunch of unnecessary includes #

Patch Set 4 : Add metrics #

Patch Set 5 : Fix null error #

Total comments: 6

Patch Set 6 : Reviewer comments #

Total comments: 2

Patch Set 7 : Fix inherited call #

Patch Set 8 : Fix compile #

Total comments: 21

Patch Set 9 : Address comments #

Total comments: 10

Patch Set 10 : Address comments, do not show toggle for notifications #

Total comments: 5

Patch Set 11 : Address nits, verbal change to make it geolocation-only #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+459 lines, -302 lines) Patch
M chrome/android/java/res/values/ids.xml View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java View 1 5 chunks +4 lines, -164 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java View 1 1 chunk +1 line, -1 line 0 comments Download
A + chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java View 1 2 3 4 5 6 7 7 chunks +50 lines, -38 lines 0 comments Download
M chrome/android/java/strings/android_chrome_strings.grd View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/android/java_sources.gni View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/geolocation/geolocation_infobar_delegate_android.h View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/geolocation/geolocation_infobar_delegate_android.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/media/midi_permission_infobar_delegate_android.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/media/protected_media_identifier_infobar_delegate_android.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/notifications/notification_permission_infobar_delegate.h View 1 2 10 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/notifications/notification_permission_infobar_delegate.cc View 1 10 2 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/permissions/permission_context_base_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -2 lines 0 comments Download
M chrome/browser/permissions/permission_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 4 chunks +26 lines, -8 lines 0 comments Download
M chrome/browser/permissions/permission_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +35 lines, -10 lines 0 comments Download
M chrome/browser/permissions/permission_queue_controller.h View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -3 lines 0 comments Download
M chrome/browser/permissions/permission_queue_controller.cc View 1 2 3 4 5 6 7 8 5 chunks +27 lines, -17 lines 0 comments Download
M chrome/browser/permissions/permission_uma_util.h View 1 2 3 4 5 6 7 8 3 chunks +12 lines, -16 lines 0 comments Download
M chrome/browser/permissions/permission_uma_util.cc View 1 2 3 4 5 6 7 8 1 chunk +98 lines, -0 lines 1 comment Download
M chrome/browser/permissions/permission_util.h View 1 2 3 4 5 6 7 8 1 chunk +14 lines, -0 lines 0 comments Download
M chrome/browser/ui/android/infobars/confirm_infobar.h View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/android/infobars/confirm_infobar.cc View 1 2 3 chunks +11 lines, -30 lines 0 comments Download
M chrome/browser/ui/android/infobars/infobar_android.h View 1 2 3 4 5 1 chunk +0 lines, -2 lines 0 comments Download
A chrome/browser/ui/android/infobars/permission_infobar.h View 1 2 3 4 5 1 chunk +33 lines, -0 lines 0 comments Download
A chrome/browser/ui/android/infobars/permission_infobar.cc View 1 2 3 4 5 6 7 8 9 1 chunk +78 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/common/chrome_features.h View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/common/chrome_features.cc View 1 2 3 4 5 6 7 1 chunk +6 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 4 chunks +25 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 68 (43 generated)
dominickn
WDYT? I tried to implement this in the least intrusive way possible. I haven't fully ...
4 years, 4 months ago (2016-08-08 07:53:54 UTC) #2
gone
https://chromiumcodereview.appspot.com/2226633002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java File chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java (right): https://chromiumcodereview.appspot.com/2226633002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java#newcode86 chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java:86: private void setShowPersistenceToggle(boolean showPersistenceToggle) { Yeah, you shouldn't put ...
4 years, 4 months ago (2016-08-08 23:04:02 UTC) #3
dominickn
On 2016/08/08 23:04:02, dfalcantara wrote: > https://chromiumcodereview.appspot.com/2226633002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java > File > chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java > (right): > > ...
4 years, 4 months ago (2016-08-09 03:40:17 UTC) #4
dominickn
On 2016/08/09 03:40:17, dominickn wrote: > On 2016/08/08 23:04:02, dfalcantara wrote: > > > https://chromiumcodereview.appspot.com/2226633002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/infobar/ConfirmInfoBar.java ...
4 years, 4 months ago (2016-08-09 03:53:09 UTC) #5
gone
On 2016/08/09 03:53:09, dominickn wrote: > On 2016/08/09 03:40:17, dominickn wrote: > > On 2016/08/08 ...
4 years, 4 months ago (2016-08-09 06:47:52 UTC) #6
dominickn
PTAL - now factored out into a new PermissionInfoBar and wired through to the permissions ...
4 years, 4 months ago (2016-08-09 08:16:47 UTC) #14
gone
Surprisingly large cleanup. Didn't realize we had so many different permissions to bother a user ...
4 years, 4 months ago (2016-08-10 17:47:53 UTC) #24
tsergeant
I'm very happy about cleaning up the permissions stuff in ConfirmInfoBar 🙌 https://chromiumcodereview.appspot.com/2226633002/diff/80001/chrome/browser/permissions/permission_uma_util.cc File chrome/browser/permissions/permission_uma_util.cc ...
4 years, 4 months ago (2016-08-11 00:21:50 UTC) #25
dominickn
Thanks! https://codereview.chromium.org/2226633002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java (right): https://codereview.chromium.org/2226633002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java#newcode168 chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:168: * ConfirmInfoBar. On 2016/08/10 17:47:53, dfalcantara wrote: > ...
4 years, 4 months ago (2016-08-11 04:43:49 UTC) #27
gone
https://chromiumcodereview.appspot.com/2226633002/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java File chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java (right): https://chromiumcodereview.appspot.com/2226633002/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java#newcode209 chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java:209: private void onButtonClickedInternal(boolean isPrimaryButton) { Does it make sense ...
4 years, 4 months ago (2016-08-12 18:42:11 UTC) #31
dominickn
+raymes: PTAL at permissions, thanks! https://codereview.chromium.org/2226633002/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java File chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java (right): https://codereview.chromium.org/2226633002/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java#newcode209 chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java:209: private void onButtonClickedInternal(boolean isPrimaryButton) ...
4 years, 4 months ago (2016-08-14 23:33:05 UTC) #35
gone
lgtm
4 years, 4 months ago (2016-08-15 20:43:51 UTC) #42
raymes
https://chromiumcodereview.appspot.com/2226633002/diff/140001/chrome/browser/permissions/permission_context_base_unittest.cc File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://chromiumcodereview.appspot.com/2226633002/diff/140001/chrome/browser/permissions/permission_context_base_unittest.cc#newcode138 chrome/browser/permissions/permission_context_base_unittest.cc:138: PermissionAction decision = DISMISSED; Since we're not just using ...
4 years, 4 months ago (2016-08-16 01:44:54 UTC) #43
dominickn
Thanks! https://codereview.chromium.org/2226633002/diff/140001/chrome/browser/permissions/permission_context_base_unittest.cc File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2226633002/diff/140001/chrome/browser/permissions/permission_context_base_unittest.cc#newcode138 chrome/browser/permissions/permission_context_base_unittest.cc:138: PermissionAction decision = DISMISSED; On 2016/08/16 01:44:53, raymes ...
4 years, 4 months ago (2016-08-16 03:06:50 UTC) #44
raymes
lgtm https://codereview.chromium.org/2226633002/diff/140001/chrome/browser/permissions/permission_queue_controller.cc File chrome/browser/permissions/permission_queue_controller.cc (right): https://codereview.chromium.org/2226633002/diff/140001/chrome/browser/permissions/permission_queue_controller.cc#newcode235 chrome/browser/permissions/permission_queue_controller.cc:235: } On 2016/08/16 03:06:50, dominickn wrote: > On ...
4 years, 4 months ago (2016-08-16 03:39:30 UTC) #45
dominickn
Thanks! +dewittj: PTAL at notification_infobar_delegate +isherman: PTAL at histograms +thestig: PTAL at chrome/common, and RS ...
4 years, 4 months ago (2016-08-16 04:15:43 UTC) #49
Lei Zhang
lgtm https://codereview.chromium.org/2226633002/diff/180001/chrome/browser/geolocation/geolocation_infobar_delegate_android.h File chrome/browser/geolocation/geolocation_infobar_delegate_android.h (right): https://codereview.chromium.org/2226633002/diff/180001/chrome/browser/geolocation/geolocation_infobar_delegate_android.h#newcode13 chrome/browser/geolocation/geolocation_infobar_delegate_android.h:13: class InfoBarService; BTW, the Google C++ Style Guide ...
4 years, 4 months ago (2016-08-16 04:25:49 UTC) #50
dominickn
On 2016/08/16 04:25:49, Lei Zhang wrote: > lgtm > > https://codereview.chromium.org/2226633002/diff/180001/chrome/browser/geolocation/geolocation_infobar_delegate_android.h > File chrome/browser/geolocation/geolocation_infobar_delegate_android.h (right): ...
4 years, 4 months ago (2016-08-16 04:36:58 UTC) #51
Ilya Sherman
Metrics LGTM % a nit: https://codereview.chromium.org/2226633002/diff/180001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2226633002/diff/180001/tools/metrics/histograms/histograms.xml#newcode39904 tools/metrics/histograms/histograms.xml:39904: +<histogram name="Permissions.Prompt.Accepted.Persisted" enum="BooleanEnabled"> Please ...
4 years, 4 months ago (2016-08-16 04:41:54 UTC) #52
Lei Zhang
On 2016/08/16 04:36:58, dominickn wrote: > The Chromium style guide explicitly prefers forward declarations (and ...
4 years, 4 months ago (2016-08-16 04:42:02 UTC) #53
dominickn
Thanks! dewittj: moved you to CC list as the change in notifications/ has gone back ...
4 years, 4 months ago (2016-08-16 04:50:16 UTC) #56
raymes
lgtm https://codereview.chromium.org/2226633002/diff/200001/chrome/browser/permissions/permission_uma_util.cc File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2226633002/diff/200001/chrome/browser/permissions/permission_uma_util.cc#newcode545 chrome/browser/permissions/permission_uma_util.cc:545: } nit: maybe we should just have geolocation ...
4 years, 4 months ago (2016-08-16 04:51:58 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2226633002/200001
4 years, 4 months ago (2016-08-16 06:02:42 UTC) #64
commit-bot: I haz the power
Committed patchset #11 (id:200001)
4 years, 4 months ago (2016-08-16 06:06:55 UTC) #66
commit-bot: I haz the power
4 years, 4 months ago (2016-08-16 06:08:39 UTC) #68
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/6957a9ca3f6db210572d6b54dcbcf69baeb978fd
Cr-Commit-Position: refs/heads/master@{#412177}

Powered by Google App Engine
This is Rietveld 408576698