New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 658125 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Feature
Team-Security-UX

Blocked on:
issue 668700

Blocking:
issue 606138



Sign in to add a comment

Implement a modal permission prompt on Android

Project Member Reported by dominickn@chromium.org, Oct 21 2016

Issue description

Implement, behind a feature flag, a modal permission prompt in place of the existing infobar. This is for experimentation purposes to see what effect forcing permission decisions has on users.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/67e6efd22bbef2baea0ac32fb603659f2ee93275

commit 67e6efd22bbef2baea0ac32fb603659f2ee93275
Author: dominickn <dominickn@chromium.org>
Date: Tue Oct 25 01:04:56 2016

Remove PermissionInfoBarDelegate from desktop builds.

This CL compiles PermissionInfoBarDelegate only on Android. It also
refactors the delegate subclasses to move their Create methods to
PermissionInfoBarDelegate, simplifying the PermissionsQueueController.

This refactor simplifies the work necessary to allow infobars to be
switched out for modal permission prompt dialogs by centralising the
delegate creation logic.

BUG= 658125 

Review-Url: https://codereview.chromium.org/2443463003
Cr-Commit-Position: refs/heads/master@{#427202}

[modify] https://crrev.com/67e6efd22bbef2baea0ac32fb603659f2ee93275/chrome/browser/BUILD.gn
[modify] https://crrev.com/67e6efd22bbef2baea0ac32fb603659f2ee93275/chrome/browser/geolocation/geolocation_infobar_delegate_android.cc
[modify] https://crrev.com/67e6efd22bbef2baea0ac32fb603659f2ee93275/chrome/browser/geolocation/geolocation_infobar_delegate_android.h
[modify] https://crrev.com/67e6efd22bbef2baea0ac32fb603659f2ee93275/chrome/browser/media/midi_permission_infobar_delegate_android.cc
[modify] https://crrev.com/67e6efd22bbef2baea0ac32fb603659f2ee93275/chrome/browser/media/midi_permission_infobar_delegate_android.h
[modify] https://crrev.com/67e6efd22bbef2baea0ac32fb603659f2ee93275/chrome/browser/media/protected_media_identifier_infobar_delegate_android.cc
[modify] https://crrev.com/67e6efd22bbef2baea0ac32fb603659f2ee93275/chrome/browser/media/protected_media_identifier_infobar_delegate_android.h
[modify] https://crrev.com/67e6efd22bbef2baea0ac32fb603659f2ee93275/chrome/browser/notifications/notification_permission_infobar_delegate.cc
[modify] https://crrev.com/67e6efd22bbef2baea0ac32fb603659f2ee93275/chrome/browser/notifications/notification_permission_infobar_delegate.h
[modify] https://crrev.com/67e6efd22bbef2baea0ac32fb603659f2ee93275/chrome/browser/permissions/permission_infobar_delegate.cc
[modify] https://crrev.com/67e6efd22bbef2baea0ac32fb603659f2ee93275/chrome/browser/permissions/permission_infobar_delegate.h
[modify] https://crrev.com/67e6efd22bbef2baea0ac32fb603659f2ee93275/chrome/browser/permissions/permission_queue_controller.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ee761a16f9819c6b610eb3f15b095cb607394d2c

commit ee761a16f9819c6b610eb3f15b095cb607394d2c
Author: dominickn <dominickn@chromium.org>
Date: Tue Nov 01 06:10:11 2016

Implement a modal permission dialog on Android gated by a feature.

This CL implements a modal prompt for geolocation, notifications,
protected media, and MIDI permission requests on Android. This prompt is
disabled by default behind the ModalPermissionPrompts feature. It can be
triggered when the feature is enabled and a permission request is made
with a user gesture. Optionally, a variations parameter can disable the
user gesture requirement.

The new prompt style attracts more attention than an infobar as it
sits in the middle of the screen and dims the content behind it. It is
intended to experiment with this to a small percentage of users to see
if it improves the permission decision rate without detracting from the
user experience. In particular, it is hoped that the more prominent
prompt will lead to more permission decisions being made, which will
reduce the overall number of prompts triggered.

Permissions may be asynchronously requested. Previously, this was
handled on Android by stacking requests for different permissions in
different infobars behind one another. However, dialogs can only be
shown once at a time, which the C++ PermissionQueueController has no way
of doing. Instead, a lightweight Java-side PermissionDialogController is
introduced which queues up multiple requests and displays them one at
a time.

For simplicity, the modal dialog delegate class simply wraps the
existing PermissionInfoBarDelegate class when the ModalPermissionPrompts
feature is active. Upcoming refactoring will eliminate all
PermissionInfoBarDelegate subclasses, and consolidate the information in
those classes into a new PermissionPromptAndroid class which will be
wrapped by GroupedPermissionInfoBarDelegate. At that time, the
PermissionDialogDelegate will be changed to wrap PermissionPromptAndroid
as well.

The Geolocation Android test is revamped to test that infobars, dialogs,
and gestures interact and trigger appropriately.

BUG= 658125 

Review-Url: https://codereview.chromium.org/2446063002
Cr-Commit-Position: refs/heads/master@{#428943}

[add] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/android/java/res/layout/permission_dialog.xml
[modify] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java
[add] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java
[add] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogDelegate.java
[modify] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/android/java/strings/android_chrome_strings.grd
[modify] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/android/java_sources.gni
[modify] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/android/javatests/src/org/chromium/chrome/browser/GeolocationTest.java
[modify] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/browser/BUILD.gn
[modify] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/browser/android/chrome_jni_registrar.cc
[add] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/browser/permissions/permission_dialog_delegate.cc
[add] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/browser/permissions/permission_dialog_delegate.h
[modify] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/browser/permissions/permission_infobar_delegate.cc
[modify] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/browser/permissions/permission_infobar_delegate.h
[modify] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/browser/permissions/permission_queue_controller.cc
[modify] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/common/chrome_features.cc
[modify] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/chrome/common/chrome_features.h
[modify] https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c/content/test/data/android/geolocation.html

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ac471ccfa681f20e8d19db5728d62aae1b27f483

commit ac471ccfa681f20e8d19db5728d62aae1b27f483
Author: dominickn <dominickn@chromium.org>
Date: Tue Nov 01 22:21:18 2016

Consolidate Android permissions prompt testing.

This CL extracts the common methods and setup requirements from
GeolocationTest and MediaPermissionsTest into a new
PermissionTestCaseBase, and makes the two concrete tests inherit from
the test case base. This consolidates the UI testing for permission
prompts on Android and makes it easier to add new tests.

BUG= 658125 

Review-Url: https://codereview.chromium.org/2456243002
Cr-Commit-Position: refs/heads/master@{#429134}

[modify] https://crrev.com/ac471ccfa681f20e8d19db5728d62aae1b27f483/chrome/android/java_sources.gni
[delete] https://crrev.com/3c3223b3a1e3c570aab417f3c89eb0410341c5b8/chrome/android/javatests/src/org/chromium/chrome/browser/GeolocationTest.java
[delete] https://crrev.com/3c3223b3a1e3c570aab417f3c89eb0410341c5b8/chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/MediaPermissionsTest.java
[add] https://crrev.com/ac471ccfa681f20e8d19db5728d62aae1b27f483/chrome/android/javatests/src/org/chromium/chrome/browser/permissions/GeolocationTest.java
[add] https://crrev.com/ac471ccfa681f20e8d19db5728d62aae1b27f483/chrome/android/javatests/src/org/chromium/chrome/browser/permissions/MediaTest.java
[add] https://crrev.com/ac471ccfa681f20e8d19db5728d62aae1b27f483/chrome/android/javatests/src/org/chromium/chrome/browser/permissions/PermissionTestCaseBase.java

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b2df0eda3699122aa2f3fea7a76c261f91d274b2

commit b2df0eda3699122aa2f3fea7a76c261f91d274b2
Author: dominickn <dominickn@chromium.org>
Date: Thu Nov 03 02:00:27 2016

Add flags for permission prompt experiments.

This CL adds entries to chrome://flags to explicitly enable:
    a) modal permission prompts on Android; and
    b) the persistence toggle in permission prompts on Windows,
       ChromeOS, Linux, and Android

BUG= 658125 

Review-Url: https://codereview.chromium.org/2471093002
Cr-Commit-Position: refs/heads/master@{#429511}

[modify] https://crrev.com/b2df0eda3699122aa2f3fea7a76c261f91d274b2/chrome/app/generated_resources.grd
[modify] https://crrev.com/b2df0eda3699122aa2f3fea7a76c261f91d274b2/chrome/browser/about_flags.cc
[modify] https://crrev.com/b2df0eda3699122aa2f3fea7a76c261f91d274b2/tools/metrics/histograms/histograms.xml

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 10 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c70672a6e41ed5fa46bee5df7f0bdc99cbbf0476

commit c70672a6e41ed5fa46bee5df7f0bdc99cbbf0476
Author: dominickn <dominickn@chromium.org>
Date: Thu Nov 10 02:11:41 2016

Implement modal permission prompts for media permissions on Android.

This CL adds support for camera and microphone permission requests on
Android using a modal dialog prompt. This is controlled by the
ModalPermissionPrompts feature, which is disabled by default. The
intention is to test this new prompt to a small percentage of users to
determine its effectiveness in improving the overall permissions request
flow.

This follows crrev.com/2446063002, which implements the modal dialog
for all other permissions on Android.

Currently, modal prompts use a PermissionDialogDelegate, which wraps the
PermissionInfoBarDelegate class and allows the Java-side dialog UI to
communicate with the native C++ permissions layer.

Upcoming refactoring work will eliminate the per-permission
InfoBarDelegate classes, and simply allow the generic
PermissionInfoBarDelegate class to wrap a PermissionPromptAndroid object
which will provide all permission-specific data. At that point,
PermissionDialogDelegate will also change to analogously wrap
PermissionPromptAndroid.

New Android media permissions UI tests are introduced to test the modal
dialogs under various configurations, including with and without the
optional persistence toggle.

BUG= 658125 

Review-Url: https://codereview.chromium.org/2462963002
Cr-Commit-Position: refs/heads/master@{#431142}

[modify] https://crrev.com/c70672a6e41ed5fa46bee5df7f0bdc99cbbf0476/chrome/android/javatests/src/org/chromium/chrome/browser/permissions/MediaTest.java
[modify] https://crrev.com/c70672a6e41ed5fa46bee5df7f0bdc99cbbf0476/chrome/browser/media/webrtc/media_stream_infobar_delegate_android.h
[modify] https://crrev.com/c70672a6e41ed5fa46bee5df7f0bdc99cbbf0476/chrome/browser/media/webrtc/permission_bubble_media_access_handler.cc
[modify] https://crrev.com/c70672a6e41ed5fa46bee5df7f0bdc99cbbf0476/chrome/browser/permissions/permission_dialog_delegate.cc
[modify] https://crrev.com/c70672a6e41ed5fa46bee5df7f0bdc99cbbf0476/chrome/browser/permissions/permission_dialog_delegate.h
[modify] https://crrev.com/c70672a6e41ed5fa46bee5df7f0bdc99cbbf0476/content/test/data/android/media_permissions.html

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/05fa07ae53aa3ce6beeb2b79def845b73eb18939

commit 05fa07ae53aa3ce6beeb2b79def845b73eb18939
Author: dominickn <dominickn@chromium.org>
Date: Fri Nov 11 02:04:21 2016

Allow modal permission prompts on Android to request system permissions.

The current modal permission prompt does not prompt the user to update
Android's system permissions if Chromium does not have that permission.
For instance, a site requests location permission, the user grants it,
but the app does not have the permission. In this case, the permission
dialog needs to prompt the user to update Android's permissions. The
existing infobar implementation already does this.

This CL factors the Android system permissions request code out of
PermissionInfoBar into a new AndroidPermissionRequester class. That
allows the class to be reused for modal permission dialogs in
PermissionDialogController.

BUG= 658125 

Review-Url: https://codereview.chromium.org/2496473003
Cr-Commit-Position: refs/heads/master@{#431459}

[modify] https://crrev.com/05fa07ae53aa3ce6beeb2b79def845b73eb18939/chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java
[add] https://crrev.com/05fa07ae53aa3ce6beeb2b79def845b73eb18939/chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java
[modify] https://crrev.com/05fa07ae53aa3ce6beeb2b79def845b73eb18939/chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java
[modify] https://crrev.com/05fa07ae53aa3ce6beeb2b79def845b73eb18939/chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogDelegate.java
[modify] https://crrev.com/05fa07ae53aa3ce6beeb2b79def845b73eb18939/chrome/android/java_sources.gni
[modify] https://crrev.com/05fa07ae53aa3ce6beeb2b79def845b73eb18939/chrome/browser/permissions/permission_dialog_delegate.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Nov 16 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/491d3509af0f0ba16bca665a91014c9d8aeb2b80

commit 491d3509af0f0ba16bca665a91014c9d8aeb2b80
Author: dominickn <dominickn@chromium.org>
Date: Wed Nov 16 00:18:32 2016

Replace the use of WindowAndroid with Tab in permissions code.

The WindowAndroid that a tab is associated with can change due to
reparenting (e.g. moving from a Chrome Custom Tab to the Chrome
Activity). It is not safe to cache the window.

This CL mostly replaces WindowAndroid references in permissions code to
references to Tab, with a late-as-possible getting of WindowAndroid only
when it is needed.

It also renames instances of content settings vectors to content
settings types vectors for clarity.

BUG= 658125 
TBR=sergeyu@chromium.org

Review-Url: https://codereview.chromium.org/2493013002
Cr-Commit-Position: refs/heads/master@{#432309}

[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java
[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java
[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java
[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/android/java/src/org/chromium/chrome/browser/infobar/PermissionInfoBar.java
[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java
[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java
[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogDelegate.java
[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc
[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/browser/media/webrtc/media_stream_infobar_delegate_android.h
[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/browser/permissions/permission_dialog_delegate.cc
[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/browser/permissions/permission_infobar_delegate.cc
[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/browser/permissions/permission_infobar_delegate.h
[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/browser/ui/android/infobars/confirm_infobar.cc
[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/browser/ui/android/infobars/confirm_infobar.h
[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/browser/ui/android/infobars/grouped_permission_infobar.cc
[modify] https://crrev.com/491d3509af0f0ba16bca665a91014c9d8aeb2b80/chrome/browser/ui/android/infobars/permission_infobar.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Nov 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/97672a1b334602c3634b635625d8896da3a58ba2

commit 97672a1b334602c3634b635625d8896da3a58ba2
Author: dominickn <dominickn@chromium.org>
Date: Fri Nov 18 02:57:50 2016

Free the native delegate when the user accepts a modal permission dialog.

This corrects an oversight where PermissionDialogDelegate#destroy was
not called when allowing a modal permission prompt, leaking to a memory
leak.

BUG= 658125 

Review-Url: https://codereview.chromium.org/2508103002
Cr-Commit-Position: refs/heads/master@{#433035}

[modify] https://crrev.com/97672a1b334602c3634b635625d8896da3a58ba2/chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java

Comment 9 by joh...@chromium.org, Nov 25 2016

Blockedon: 668700
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a70629e5ddc767a972c9c5789da4d330a2fcd1a7

commit a70629e5ddc767a972c9c5789da4d330a2fcd1a7
Author: johnme <johnme@chromium.org>
Date: Fri Nov 25 14:59:19 2016

[permissions.MediaTest] Mark testMicrophonePersistenceOffDialog flaky

TBR=dominickn@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 668700 , 658125 

Review-Url: https://codereview.chromium.org/2530173002
Cr-Commit-Position: refs/heads/master@{#434503}

[modify] https://crrev.com/a70629e5ddc767a972c9c5789da4d330a2fcd1a7/chrome/android/javatests/src/org/chromium/chrome/browser/permissions/MediaTest.java

Blocking: 606138
Cc: -lshang@chromium.org
Status: Fixed (was: Started)
This is now implemented and complete.

Sign in to add a comment