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

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
Committed: https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c
Cr-Commit-Position: refs/heads/master@{#428943}

[dominickn, 2016-10-25 06:36:03]

dominickn@chromium.org changed reviewers:
+ dfalcantara@chromium.org, raymes@chromium.org

[dominickn, 2016-10-25 06:36:04]

Hey Dan, Raymes, can you take an initial look? I'm working on a test but thought
I'd put this up for comments on the implementation. I'll forward an email with
screenshots of what it looks like.

The big goal for M56 from the installabillity team is to experiment with a modal
permissions prompt on Android (we have UI review's blessing). This is a bit ugly
right now, but lshang is working on refactoring that will clean this up post
M56.

[dominickn, 2016-10-26 10:33:28]

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

[commit-bot: I haz the power, 2016-10-26 10:33:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-10-26 10:33:58]

On 2016/10/25 06:36:04, dominickn wrote:
> Hey Dan, Raymes, can you take an initial look? I'm working on a test but
thought
> I'd put this up for comments on the implementation. I'll forward an email with
> screenshots of what it looks like.
> 
> The big goal for M56 from the installabillity team is to experiment with a
modal
> permissions prompt on Android (we have UI review's blessing). This is a bit
ugly
> right now, but lshang is working on refactoring that will clean this up post
> M56.

Update: this now has a fairly substantial test. :)

Still waiting on UI feedback for the precise look and feel of the dialog, so I'm
looking for comments on the implementation here. :)

[commit-bot: I haz the power, 2016-10-26 11:32:33]

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

[commit-bot: I haz the power, 2016-10-26 11:32:34]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-10-27 02:55:58]

Description was changed from

==========
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, and is enabled behind the ModalPermissionPrompts
feature.

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.

BUG=658125
==========

to

==========
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.

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.

BUG=658125
==========

[dominickn, 2016-10-27 07:34:06]

Description was changed from

==========
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.

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.

BUG=658125
==========

to

==========
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
==========

[dominickn, 2016-10-27 07:47:44]

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

[commit-bot: I haz the power, 2016-10-27 07:48:21]

Dry run: 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, 2016-10-27 09:09:56]

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

[commit-bot: I haz the power, 2016-10-27 09:09:57]

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

[gone, 2016-10-27 20:33:35]

https://codereview.chromium.org/2446063002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/permission_dialog.xml (right):

https://codereview.chromium.org/2446063002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/permission_dialog.xml:20:
android:layout_marginTop="10dp" />
nit: move this margin up next to the other layout_ params

https://codereview.chromium.org/2446063002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogDelegate.java
(right):

https://codereview.chromium.org/2446063002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogDelegate.java:56:
}
nit: Stick newlines in between

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java
(right):

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:38:
private LinkedList<PermissionDialogDelegate> mRequestQueue;
Go abstract:
private List<PermissionDialogDelegate>();

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:47:
mRequestQueue = new LinkedList<PermissionDialogDelegate>();
Does this work?
mRequestQueue = new LinkedList<>();

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:55:
public static void createDialog(PermissionDialogDelegate delegate) {
1) @CalledByNative doesn't care if the method is private, and it doesn't seem
   to be called anywhere else:
   
   private static void

2) Can this method be renamed?  What it does is ambiguous when there's also a
queueDialog.

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:72:
public void scheduleDisplay() {
private void

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:89:
LayoutInflater inflator = LayoutInflater.from(activity);
inflater for spelling consistency?

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:122:
// There is no way for the user to make a once-off decision, so allow them to
dismiss
Can you clarify this comment?  I guess the toggle applies to record the decision
permanently even if it's not visible?

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:166:
if (messageText != null) fullString.append(messageText);
TextUtils.isEmpty

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/GeolocationTest.java
(right):

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/GeolocationTest.java:339:
runAllowTest(updateWaiter, "initiate_watchPosition()", 2, true, true, false,
false);
The number of updates is only 1 on line 172, isn't it?

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

https://codereview.chromium.org/2446063002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_dialog_delegate.cc:56: requesting_frame,
user_gesture, profile, callback));
indentation is off here and below

https://codereview.chromium.org/2446063002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_dialog_delegate.cc:79: // the delegate.
Comment needs reworking.  Not sure you can own the lifetime of a delegate
either...  Manages the lifecycle of the java delegate?  The native delegate?

https://codereview.chromium.org/2446063002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_dialog_delegate.cc:147: // wrapped
delegate has no owner (is hasn't been added as an infobar).
is hasn't

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

https://codereview.chromium.org/2446063002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_queue_controller.cc:130: if
(PermissionDialogDelegate::ShouldShowModalPrompt(user_gesture_)) {
This is really, really weird as you mentioned.  It'd be less weird if you
renamed the function something like PromptUser or something.

[dominickn, 2016-10-27 23:16:49]

Patchset #5 (id:80001) has been deleted

[dominickn, 2016-10-27 23:36:43]

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

[dominickn, 2016-10-27 23:37:13]

Thanks! I've now aligned the remember text toggle with the main message text in
the prompt to match infobars. That was the only feedback UI had. :)

https://codereview.chromium.org/2446063002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/permission_dialog.xml (right):

https://codereview.chromium.org/2446063002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/permission_dialog.xml:20:
android:layout_marginTop="10dp" />
On 2016/10/27 20:33:34, dfalcantara (slow 10.24) wrote:
> nit: move this margin up next to the other layout_ params

Done.

https://codereview.chromium.org/2446063002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogDelegate.java
(right):

https://codereview.chromium.org/2446063002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogDelegate.java:56:
}
On 2016/10/27 20:33:34, dfalcantara (slow 10.24) wrote:
> nit: Stick newlines in between

Done.

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java
(right):

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:38:
private LinkedList<PermissionDialogDelegate> mRequestQueue;
On 2016/10/27 20:33:35, dfalcantara (slow 10.24) wrote:
> Go abstract:
> private List<PermissionDialogDelegate>();

Done.

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:47:
mRequestQueue = new LinkedList<PermissionDialogDelegate>();
On 2016/10/27 20:33:35, dfalcantara (slow 10.24) wrote:
> Does this work?
> mRequestQueue = new LinkedList<>();

Done.

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:55:
public static void createDialog(PermissionDialogDelegate delegate) {
On 2016/10/27 20:33:35, dfalcantara (slow 10.24) wrote:
> 1) @CalledByNative doesn't care if the method is private, and it doesn't seem
>    to be called anywhere else:
>    
>    private static void
> 
> 2) Can this method be renamed?  What it does is ambiguous when there's also a
> queueDialog.

Done. I couldn't think of a better name, so I added a clarifying comment
(createDialog is nice from the other side of the JNI, and its meant to abstract
away the fact that the dialog might need to wait).

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:72:
public void scheduleDisplay() {
On 2016/10/27 20:33:35, dfalcantara (slow 10.24) wrote:
> private void

Done.

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:89:
LayoutInflater inflator = LayoutInflater.from(activity);
On 2016/10/27 20:33:35, dfalcantara (slow 10.24) wrote:
> inflater for spelling consistency?

Funny story, I initially had the type as LayoutInflator, and it took the longest
time to work out why that didn't compile.

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:122:
// There is no way for the user to make a once-off decision, so allow them to
dismiss
On 2016/10/27 20:33:35, dfalcantara (slow 10.24) wrote:
> Can you clarify this comment?  I guess the toggle applies to record the
decision
> permanently even if it's not visible?

Done.

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:166:
if (messageText != null) fullString.append(messageText);
On 2016/10/27 20:33:35, dfalcantara (slow 10.24) wrote:
> TextUtils.isEmpty

Done.

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/GeolocationTest.java
(right):

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/GeolocationTest.java:339:
runAllowTest(updateWaiter, "initiate_watchPosition()", 2, true, true, false,
false);
On 2016/10/27 20:33:35, dfalcantara (slow 10.24) wrote:
> The number of updates is only 1 on line 172, isn't it?

Not quite sure what you mean?

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

https://codereview.chromium.org/2446063002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_dialog_delegate.cc:56: requesting_frame,
user_gesture, profile, callback));
On 2016/10/27 20:33:35, dfalcantara (slow 10.24) wrote:
> indentation is off here and below

Done.

https://codereview.chromium.org/2446063002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_dialog_delegate.cc:79: // the delegate.
On 2016/10/27 20:33:35, dfalcantara (slow 10.24) wrote:
> Comment needs reworking.  Not sure you can own the lifetime of a delegate
> either...  Manages the lifecycle of the java delegate?  The native delegate?

Done.

https://codereview.chromium.org/2446063002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_dialog_delegate.cc:147: // wrapped
delegate has no owner (is hasn't been added as an infobar).
On 2016/10/27 20:33:35, dfalcantara (slow 10.24) wrote:
> is hasn't

Done.

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

https://codereview.chromium.org/2446063002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_queue_controller.cc:130: if
(PermissionDialogDelegate::ShouldShowModalPrompt(user_gesture_)) {
On 2016/10/27 20:33:35, dfalcantara (slow 10.24) wrote:
> This is really, really weird as you mentioned.  It'd be less weird if you
> renamed the function something like PromptUser or something.

Done.

[commit-bot: I haz the power, 2016-10-27 23:37:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gone, 2016-10-27 23:52:20]

lgtm, but I'm really looking forward to that logic being pulled out.

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java
(right):

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:89:
LayoutInflater inflator = LayoutInflater.from(activity);
On 2016/10/27 23:37:12, dominickn wrote:
> On 2016/10/27 20:33:35, dfalcantara (slow 10.24) wrote:
> > inflater for spelling consistency?
> 
> Funny story, I initially had the type as LayoutInflator, and it took the
longest
> time to work out why that didn't compile.

I'll spare you from the stream of jokes I was about to make about English
spelling differences.

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/GeolocationTest.java
(right):

https://codereview.chromium.org/2446063002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/GeolocationTest.java:339:
runAllowTest(updateWaiter, "initiate_watchPosition()", 2, true, true, false,
false);
On 2016/10/27 23:37:13, dominickn wrote:
> On 2016/10/27 20:33:35, dfalcantara (slow 10.24) wrote:
> > The number of updates is only 1 on line 172, isn't it?
> 
> Not quite sure what you mean?

Clarified over chat; I meant line 172 in the old file.  The new function call
here replaces the old one and now waits for 2 updates.

[commit-bot: I haz the power, 2016-10-28 00:54:30]

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

[commit-bot: I haz the power, 2016-10-28 00:54:31]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-10-30 22:55:39]

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

[commit-bot: I haz the power, 2016-10-30 22:55:52]

Dry run: 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, 2016-10-30 23:49:43]

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

[commit-bot: I haz the power, 2016-10-30 23:49:44]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-10-31 02:01:45]

Hey Dom, I took a high-level look. A few thoughts
1) Just to confirm, Java and C++ code can continue to execute while there is a
modal dialog showing, correct? This will be important for reasoning about
lifetimes, etc. 

2) Some scenarios to consider/test:
-What will happen if a tab gets navigated while the dialog is showing? Will
things work in a sensible way? 
-What will happen if a user switches applications on android and then comes
back? 
-What about if they switch applications and then re-enter chrome by loading a
different page (e.g. clicking a link in another application)? 
-What about if they then change back to the original tab?

3) This CL is quite large and there is a lot going on. Could we find a way to
split it up? That might be just separating the Java side/C++ side/test out. It
just means that each part is a smaller chunk to reason about and comment on.
Even just the changes in permission_queue_controller.cc seem like they need to
be carefully checked to make sure that it plays well with the existing logic and
so hooking things up in there could even be a separate CL in itself.

4) It might be good to get lshang/kcarattini to help out with the reviews?
Especially since I'm not an expert with this code anyway.

Thanks!
Raymes

[dominickn, 2016-10-31 02:14:42]

On 2016/10/31 02:01:45, raymes wrote:
> Hey Dom, I took a high-level look. A few thoughts
> 1) Just to confirm, Java and C++ code can continue to execute while there is a
> modal dialog showing, correct? This will be important for reasoning about
> lifetimes, etc. 

Yes, this is correct. Dialogs are deliberately async on Android.

> 2) Some scenarios to consider/test:
> -What will happen if a tab gets navigated while the dialog is showing? Will
> things work in a sensible way? 

The dialog remains on screen until the user responds to it. The page underneath
will navigate. The permission will be set and persisted if that option is
selected.

Permission bubbles/infobars currently dismiss on navigation. With the change to
modals, I think this behaviour makes sense (we really want to see if making the
user make a choice makes a difference).

> -What will happen if a user switches applications on android and then comes
> back? 

The modal will be on the screen when the user comes back to Chrome (it's tied to
the Chrome app so it'll go away with Chrome upon switching)

> -What about if they switch applications and then re-enter chrome by loading a
> different page (e.g. clicking a link in another application)? 
> -What about if they then change back to the original tab?

That usually opens a new tab. If it doesn't, see the navigation case above (and
it really should open a new tab so it doesn't flush your current state)

> 3) This CL is quite large and there is a lot going on. Could we find a way to
> split it up? That might be just separating the Java side/C++ side/test out. It
> just means that each part is a smaller chunk to reason about and comment on.
> Even just the changes in permission_queue_controller.cc seem like they need to
> be carefully checked to make sure that it plays well with the existing logic
and
> so hooking things up in there could even be a separate CL in itself.

The Java side and C++ side are linked together. It doesn't really make sense to
split them out - it means landing untested code that isn't built and isn't
called by anything. Also, it doesn't make sense to piecemeal land the feature
and a do-nothing PermissionPromptDelegate because that's a tiny fraction of this
CL.

The only thing that can reasonably be split out is the test, which is a self
contained change. However, I like having the test in the same CL if possible
(and it's often the first comment for a big change - where is the test).

> 4) It might be good to get lshang/kcarattini to help out with the reviews?
> Especially since I'm not an expert with this code anyway.

Are you suggested they review before you rubber stamp?

[dominickn, 2016-10-31 02:18:44]

dominickn@chromium.org changed reviewers:
+ lshang@chromium.org

[dominickn, 2016-10-31 02:18:44]

+lshang - raymes wants you to take a look thanks.

[raymes, 2016-10-31 02:32:44]

> The Java side and C++ side are linked together. It doesn't really make sense
to
> split them out - it means landing untested code that isn't built and isn't
> called by anything. 

I disagree with this as a general principle. Doing a good job of reviewing and
iterating on 1000+ lines in one go is very hard. It's not ideal to land code
that is temporarily unused but it's better than doing a bad review of a very
large CL. I think it's better to find a sensible way to break it up.

[raymes, 2016-11-01 02:56:46]

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_dialog_delegate.cc (right):

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_dialog_delegate.cc:66: break;
Could we factor this switch statement out as out as common code with
PermissionInfoBarDelegate::Create ?

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_dialog_delegate.cc:86: bool
PermissionDialogDelegate::PromptUser(bool has_user_gesture) {
As discussed, this might be better as ShouldUseModalDialog (or something like
that) so it's clear it returns a bool.

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_dialog_delegate.cc:90: // Enforce a user
gesture requirement by default. Only relax this if
"Enforce a user gesture requirement by default" is a little confusing. Perhaps
something like
"Only use modals when the prompt is triggered by a user gesture, unless the
kModalParamsUserGestureKey param is set to "false" ?

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_dialog_delegate.cc:94: if (require_gesture
== "0" || require_gesture == "false")
How come "0" and "false" are both special? Could we just test for one of those?

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_dialog_delegate.cc:96: return
has_user_gesture;
Hmm, this means that you could get an infobar or a modal prompt at the same
time. That seems quite weird and like we would probably never ship that? What's
the danger in always having modals?

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_queue_controller.cc (right):

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_queue_controller.cc:243:
PermissionDialogDelegate::PromptUser(i->has_gesture())) {
Is this check strictly necessary? Won't has_infobar be false for the request if
we created a modal for it?

[dominickn, 2016-11-01 03:30:46]

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

[dominickn, 2016-11-01 03:30:59]

Thanks! :)

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_dialog_delegate.cc (right):

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_dialog_delegate.cc:66: break;
On 2016/11/01 02:56:46, raymes wrote:
> Could we factor this switch statement out as out as common code with
> PermissionInfoBarDelegate::Create ?

I thought about it when I was implementing it, and it wasn't pretty.
PermissionInfoBarDelegate::Create returns an infobar pointer, which we don't
want to do here (the InfoBarService may return a nullptr, so that's a signal
that something went wrong in the infobar case).

Another option is to someone combine the two Create methods into one, e.g.
moving PermissionInfoBarDelegate's construction inside
PermissionDialogDelegate::Create. That seems premature at this stage - it may be
something we do in the future when we have a definitive decision on whether or
not we keep the modals.

For now, I kept it separate because it felt safer to keep the two prompt types
as separate as possible for now. When we figure out what we go with, that seems
like a safer time to determine what we should do. WDYT?

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_dialog_delegate.cc:86: bool
PermissionDialogDelegate::PromptUser(bool has_user_gesture) {
On 2016/11/01 02:56:46, raymes wrote:
> As discussed, this might be better as ShouldUseModalDialog (or something like
> that) so it's clear it returns a bool.

Done.

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_dialog_delegate.cc:94: if (require_gesture
== "0" || require_gesture == "false")
On 2016/11/01 02:56:46, raymes wrote:
> How come "0" and "false" are both special? Could we just test for one of
those?

Done.

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_dialog_delegate.cc:96: return
has_user_gesture;
On 2016/11/01 02:56:46, raymes wrote:
> Hmm, this means that you could get an infobar or a modal prompt at the same
> time. That seems quite weird and like we would probably never ship that?
What's
> the danger in always having modals?

The UI team actually told us that the original intention for permission prompts
on Android was to use a modal for "good" prompts (i.e. with user gesture) and an
infobar for "bad" prompts. The latter was implemented, but the former never was.
This is implemented to match that original spec, with the Finch control to let
us roll out the universal version.

The danger of always having modals is onload prompts - your page fires a
permission request and the first thing you see is a modal when you load it. That
would be pretty annoying.

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_queue_controller.cc (right):

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_queue_controller.cc:243:
PermissionDialogDelegate::PromptUser(i->has_gesture())) {
On 2016/11/01 02:56:46, raymes wrote:
> Is this check strictly necessary? Won't has_infobar be false for the request
if
> we created a modal for it?

Good point. Removed.

[commit-bot: I haz the power, 2016-11-01 03:31:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-11-01 03:47:44]

And I have another question, if there is a modal prompt shown in tab_1, and go
to the same website in tab_2, make the prompt shown again and click 'allow' or
'block' to it, will the prompt in tab_1 get notified and closed?

https://codereview.chromium.org/2446063002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java
(right):

https://codereview.chromium.org/2446063002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:73:
mDialog = null;
As discussed, this line should be removed to show new prompt only after user has
made action to the current prompt.

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_queue_controller.cc (right):

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_queue_controller.cc:130: if
(PermissionDialogDelegate::PromptUser(user_gesture_)) {
I'd rename this method as ShouldPromptUser(), since it's checking whether
eligible to show the prompt and returns a bool. The current name is like action
taken.

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_queue_controller.cc:323: if
(ArePermissionRequestsForSameTab(i->id(), id) && i->has_infobar())
Should also check if already showing modal prompt here? For example, if there is
already a notification modal prompt showing here, and another notification
request comes, we shouldn't show the same requests twice.

https://codereview.chromium.org/2446063002/diff/120001/content/test/data/andr...
File content/test/data/android/geolocation.html (right):

https://codereview.chromium.org/2446063002/diff/120001/content/test/data/andr...
content/test/data/android/geolocation.html:26: // a click event listener to run
a specified method (which may be changed by
wrap lines to 80-column limit

[raymes, 2016-11-01 03:50:46]

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_dialog_delegate.cc (right):

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_dialog_delegate.cc:66: break;
On 2016/11/01 03:30:58, dominickn wrote:
> On 2016/11/01 02:56:46, raymes wrote:
> > Could we factor this switch statement out as out as common code with
> > PermissionInfoBarDelegate::Create ?
> 
> I thought about it when I was implementing it, and it wasn't pretty.
> PermissionInfoBarDelegate::Create returns an infobar pointer, which we don't
> want to do here (the InfoBarService may return a nullptr, so that's a signal
> that something went wrong in the infobar case).
> 
> Another option is to someone combine the two Create methods into one, e.g.
> moving PermissionInfoBarDelegate's construction inside
> PermissionDialogDelegate::Create. That seems premature at this stage - it may
be
> something we do in the future when we have a definitive decision on whether or
> not we keep the modals.
> 
> For now, I kept it separate because it felt safer to keep the two prompt types
> as separate as possible for now. When we figure out what we go with, that
seems
> like a safer time to determine what we should do. WDYT?

Could we just have this switch statement as the common code? What I mean is
adding a 
std::unique_ptr<PermissionInfoBarDelegate>
PermissionInfoBarDelegate::CreateDelegate(...) which is called from here and
inside PermissionInfoBarDelegate::Create ?

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_dialog_delegate.cc:96: return
has_user_gesture;
On 2016/11/01 03:30:58, dominickn wrote:
> On 2016/11/01 02:56:46, raymes wrote:
> > Hmm, this means that you could get an infobar or a modal prompt at the same
> > time. That seems quite weird and like we would probably never ship that?
> What's
> > the danger in always having modals?
> 
> The UI team actually told us that the original intention for permission
prompts
> on Android was to use a modal for "good" prompts (i.e. with user gesture) and
an
> infobar for "bad" prompts. The latter was implemented, but the former never
was.
> This is implemented to match that original spec, with the Finch control to let
> us roll out the universal version.
> 
> The danger of always having modals is onload prompts - your page fires a
> permission request and the first thing you see is a modal when you load it.
That
> would be pretty annoying.

I see - that still doesn't seem like a great experience to me, but that can be
discussed more later :)

[commit-bot: I haz the power, 2016-11-01 04:17:06]

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

[commit-bot: I haz the power, 2016-11-01 04:17:07]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-11-01 04:43:48]

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

[commit-bot: I haz the power, 2016-11-01 04:43:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-11-01 04:44:00]

Thanks!

https://codereview.chromium.org/2446063002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java
(right):

https://codereview.chromium.org/2446063002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:73:
mDialog = null;
On 2016/11/01 03:47:44, lshang wrote:
> As discussed, this line should be removed to show new prompt only after user
has
> made action to the current prompt.

Done.

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_dialog_delegate.cc (right):

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_dialog_delegate.cc:66: break;
On 2016/11/01 03:50:46, raymes wrote:
> On 2016/11/01 03:30:58, dominickn wrote:
> > On 2016/11/01 02:56:46, raymes wrote:
> > > Could we factor this switch statement out as out as common code with
> > > PermissionInfoBarDelegate::Create ?
> > 
> > I thought about it when I was implementing it, and it wasn't pretty.
> > PermissionInfoBarDelegate::Create returns an infobar pointer, which we don't
> > want to do here (the InfoBarService may return a nullptr, so that's a signal
> > that something went wrong in the infobar case).
> > 
> > Another option is to someone combine the two Create methods into one, e.g.
> > moving PermissionInfoBarDelegate's construction inside
> > PermissionDialogDelegate::Create. That seems premature at this stage - it
may
> be
> > something we do in the future when we have a definitive decision on whether
or
> > not we keep the modals.
> > 
> > For now, I kept it separate because it felt safer to keep the two prompt
types
> > as separate as possible for now. When we figure out what we go with, that
> seems
> > like a safer time to determine what we should do. WDYT?
> 
> Could we just have this switch statement as the common code? What I mean is
> adding a 
> std::unique_ptr<PermissionInfoBarDelegate>
> PermissionInfoBarDelegate::CreateDelegate(...) which is called from here and
> inside PermissionInfoBarDelegate::Create ?

Ah, I see. That makes sense, thanks!

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_dialog_delegate.cc:90: // Enforce a user
gesture requirement by default. Only relax this if
On 2016/11/01 02:56:46, raymes wrote:
> "Enforce a user gesture requirement by default" is a little confusing. Perhaps
> something like
> "Only use modals when the prompt is triggered by a user gesture, unless the
> kModalParamsUserGestureKey param is set to "false" ?

Done.

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_queue_controller.cc (right):

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_queue_controller.cc:130: if
(PermissionDialogDelegate::PromptUser(user_gesture_)) {
On 2016/11/01 03:47:44, lshang wrote:
> I'd rename this method as ShouldPromptUser(), since it's checking whether
> eligible to show the prompt and returns a bool. The current name is like
action
> taken.

Done.

https://codereview.chromium.org/2446063002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_queue_controller.cc:323: if
(ArePermissionRequestsForSameTab(i->id(), id) && i->has_infobar())
On 2016/11/01 03:47:44, lshang wrote:
> Should also check if already showing modal prompt here? For example, if there
is
> already a notification modal prompt showing here, and another notification
> request comes, we shouldn't show the same requests twice.

Hmm, that logic doesn't work, because the request for a modal prompt is already
added to the queue of things that we're checking in this method (which is why I
didn't do it originally). But I think you're right in that there's an edge case
here. Instead, I've added a has_dialog() field. That should do the trick,
thanks.

https://codereview.chromium.org/2446063002/diff/120001/content/test/data/andr...
File content/test/data/android/geolocation.html (right):

https://codereview.chromium.org/2446063002/diff/120001/content/test/data/andr...
content/test/data/android/geolocation.html:26: // a click event listener to run
a specified method (which may be changed by
On 2016/11/01 03:47:44, lshang wrote:
> wrap lines to 80-column limit

Done.

[dominickn, 2016-11-01 04:59:50]

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

[commit-bot: I haz the power, 2016-11-01 05:00:05]

Dry run: 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, 2016-11-01 05:50:45]

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

[commit-bot: I haz the power, 2016-11-01 05:50:45]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-11-01 05:55:29]

lgtm - I'm looking forward to being able to delete the queue controller though!

[lshang, 2016-11-01 06:02:59]

lgtm!

[dominickn, 2016-11-01 06:05:17]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-11-01 06:05:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2446063002/#ps180001
(title: "Move delegate creation and dispatch into constructor")

[commit-bot: I haz the power, 2016-11-01 06:05:33]

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, 2016-11-01 06:10:41]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2016-11-01 06:12:53]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c
Cr-Commit-Position: refs/heads/master@{#428943}
==========

[commit-bot: I haz the power, 2016-11-01 06:12:55]

Patchset 9 (id:??) landed as
https://crrev.com/ee761a16f9819c6b610eb3f15b095cb607394d2c
Cr-Commit-Position: refs/heads/master@{#428943}