Permissions: Allow control of individual requests in media permission infobars

Permissions: Allow control of individual requests in media permission infobars

This adds toggle switches to Camera and Microphone permission requests,
allowing the permissions to be allowed/blocked independently of each
other.

BUG=458606
Committed: https://crrev.com/2d1cc3ee786f0135b47d1258de67e166ac4eac9c
Cr-Commit-Position: refs/heads/master@{#398756}

[tsergeant, 2016-05-30 04:50:24]

Patchset #1 (id:1) has been deleted

[tsergeant, 2016-05-30 04:50:30]

Patchset #1 (id:20001) has been deleted

[tsergeant, 2016-05-30 04:51:17]

Description was changed from

==========
Permissions: Allow control of individual requests in media permission infobars

This allows Camera and Microphone permissions to be allowed/blocked
independently of each other.

BUG=458606
==========

to

==========
Permissions: Allow control of individual requests in media permission infobars

This adds toggle switches to Camera and Microphone permission requests,
allowing the permissions to be allowed/blocked independently of each
other.

BUG=458606
==========

[tsergeant, 2016-05-30 04:52:53]

tsergeant@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[tsergeant, 2016-05-30 04:52:54]

dfalcantara@, this is the follow-up to
https://codereview.chromium.org/1889643003/.

Can you please take a look?

https://codereview.chromium.org/2019573002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java
(right):

https://codereview.chromium.org/2019573002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java:321:
* @param toggleListener Listener to attach to checked change events on the
toggle.
dfalcantara@, I'm interested to know your thoughts about the best way to do
this.

Basically, I want to know when the i-th toggle in the UI is changed. Knowing
immediately (through an event) is preferable, but querying the state when the
infobar is closed would probably work too.

My current solution is to pass in a listener to the switch, and use buttonId = i
to distinguish between different switches.

[gone, 2016-05-31 21:20:42]

At a high level, I wonder if it makes more sense to pass the toggle states down
to the native side only once we know that the user hits OK/Allow.  All the
toggle states could passed as  a simple array instead of jumping between the JNI
boundaries each time the user toggles something.  It'd also make it less
complicated to understand _when_ a setting is being saved, and avoid having to
synchronize the current state of the Java UI with the separate boolean array in
native code.

https://chromiumcodereview.appspot.com/2019573002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java
(right):

https://chromiumcodereview.appspot.com/2019573002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:23:
private boolean[] mToggleStatus;
mPermissionState?  Would relate it to the other mPermission fields.

https://chromiumcodereview.appspot.com/2019573002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:25:
private final WindowAndroid mWindowAndroid;
private finals before privates

https://chromiumcodereview.appspot.com/2019573002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:49:
R.color.light_normal_color, mPermissionText[i], i, this, true)
Indentation here is wonky.  Why are you doing a findViewById, though?  You're
not saving it.

https://chromiumcodereview.appspot.com/2019573002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:56:
public void onButtonClicked(final boolean isPrimaryButton) {
Doesn't this whole code block depend on whether they pushed OK or not?  If they
hit cancel, you shouldn't be pushing these settings through.

https://chromiumcodereview.appspot.com/2019573002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:59:
SparseIntArray selectedContentSettings = new SparseIntArray();
Why bother with a SparseIntArray here?  Seems like you'd do just fine with an
ArrayList because it's contiguous.

https://chromiumcodereview.appspot.com/2019573002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java
(right):

https://chromiumcodereview.appspot.com/2019573002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java:321:
* @param toggleListener Listener to attach to checked change events on the
toggle.
On 2016/05/30 04:52:54, tsergeant wrote:
> dfalcantara@, I'm interested to know your thoughts about the best way to do
> this.
> 
> Basically, I want to know when the i-th toggle in the UI is changed. Knowing
> immediately (through an event) is preferable, but querying the state when the
> infobar is closed would probably work too.
> 
> My current solution is to pass in a listener to the switch, and use buttonId =
i
> to distinguish between different switches.

Given that you should only persist settings when the user hits OK (and not when
the infobar autodismisses or the user hits X), I think it'd be preferable to
just grab the switch states at the point when we know the user hits the primary
button.

That said, I'm not sure it makes sense to pass in a toggleListener for most
cases and would prefer not amending the function prototype for that.  You may as
well just do a findViewById(switchId).setOnCheckedChangeListener()

https://chromiumcodereview.appspot.com/2019573002/diff/40001/chrome/browser/m...
File chrome/browser/media/media_stream_infobar_delegate_android.cc (right):

https://chromiumcodereview.appspot.com/2019573002/diff/40001/chrome/browser/m...
chrome/browser/media/media_stream_infobar_delegate_android.cc:100: // Accept
state 0 -> Audio, Accept state 1 -> Video.
Define these explicitly as global constants?

https://chromiumcodereview.appspot.com/2019573002/diff/40001/chrome/browser/u...
File chrome/browser/ui/android/infobars/grouped_permission_infobar.h (right):

https://chromiumcodereview.appspot.com/2019573002/diff/40001/chrome/browser/u...
chrome/browser/ui/android/infobars/grouped_permission_infobar.h:21: void
ToggleAccept(JNIEnv* env,
ToggleAccept is confusing.  Sounds like something is being toggled except you're
explicitly passing in the new state.  OnPermissionChanged?

[tsergeant, 2016-06-02 06:52:45]

Patchset #2 (id:60001) has been deleted

[tsergeant, 2016-06-02 07:01:13]

Patchset #2 (id:80001) has been deleted

[tsergeant, 2016-06-02 07:03:59]

Patchset #2 (id:100001) has been deleted

[tsergeant, 2016-06-02 07:04:36]

I've changed the JNI interface to send through the array in one go.

The original idea was to mirror the Desktop version of this UI, which passes
back toggles to its underlying PermissionBubbleView::Delegate as they are
changed. I don't think there's any reason why this needs to be the case, though.

Since the eventual plan is for the GroupedPermissionInfoBarDelegate to call back
into the PermissionBubbleView Delegate, I haven't changed the signature of
GroupedPermissionInfoBarDelegate::ToggleAccept (which matches
PermissionBubbleView::Delegate::ToggleAccept).

https://codereview.chromium.org/2019573002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java
(right):

https://codereview.chromium.org/2019573002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:23:
private boolean[] mToggleStatus;
On 2016/05/31 21:20:42, dfalcantara wrote:
> mPermissionState?  Would relate it to the other mPermission fields.

Done (Obsoleted by changes to switch handling)

https://codereview.chromium.org/2019573002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:25:
private final WindowAndroid mWindowAndroid;
On 2016/05/31 21:20:42, dfalcantara wrote:
> private finals before privates

Done.

https://codereview.chromium.org/2019573002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:49:
R.color.light_normal_color, mPermissionText[i], i, this, true)
On 2016/05/31 21:20:42, dfalcantara wrote:
> Indentation here is wonky.  Why are you doing a findViewById, though?  You're
> not saving it.

Welp, oops.

Done.

https://codereview.chromium.org/2019573002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:56:
public void onButtonClicked(final boolean isPrimaryButton) {
On 2016/05/31 21:20:42, dfalcantara wrote:
> Doesn't this whole code block depend on whether they pushed OK or not?  If
they
> hit cancel, you shouldn't be pushing these settings through.

Done.

https://codereview.chromium.org/2019573002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:59:
SparseIntArray selectedContentSettings = new SparseIntArray();
On 2016/05/31 21:20:42, dfalcantara wrote:
> Why bother with a SparseIntArray here?  Seems like you'd do just fine with an
> ArrayList because it's contiguous.

Done. (I played around with both originally, SparseIntArray lets me avoid
boxing/unboxing the Integer but other than that there's no real difference).

https://codereview.chromium.org/2019573002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java
(right):

https://codereview.chromium.org/2019573002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java:321:
* @param toggleListener Listener to attach to checked change events on the
toggle.
On 2016/05/31 21:20:42, dfalcantara wrote:
> On 2016/05/30 04:52:54, tsergeant wrote:
> > dfalcantara@, I'm interested to know your thoughts about the best way to do
> > this.
> > 
> > Basically, I want to know when the i-th toggle in the UI is changed. Knowing
> > immediately (through an event) is preferable, but querying the state when
the
> > infobar is closed would probably work too.
> > 
> > My current solution is to pass in a listener to the switch, and use buttonId
=
> i
> > to distinguish between different switches.
> 
> Given that you should only persist settings when the user hits OK (and not
when
> the infobar autodismisses or the user hits X), I think it'd be preferable to
> just grab the switch states at the point when we know the user hits the
primary
> button.
> 
> That said, I'm not sure it makes sense to pass in a toggleListener for most
> cases and would prefer not amending the function prototype for that.  You may
as
> well just do a findViewById(switchId).setOnCheckedChangeListener()

Done -- I'm now getting the switch values when the user hits OK, and sending
them down to native code in one array.

https://codereview.chromium.org/2019573002/diff/40001/chrome/browser/media/me...
File chrome/browser/media/media_stream_infobar_delegate_android.cc (right):

https://codereview.chromium.org/2019573002/diff/40001/chrome/browser/media/me...
chrome/browser/media/media_stream_infobar_delegate_android.cc:100: // Accept
state 0 -> Audio, Accept state 1 -> Video.
On 2016/05/31 21:20:42, dfalcantara wrote:
> Define these explicitly as global constants?

Done.

https://codereview.chromium.org/2019573002/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/grouped_permission_infobar.h (right):

https://codereview.chromium.org/2019573002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/grouped_permission_infobar.h:21: void
ToggleAccept(JNIEnv* env,
On 2016/05/31 21:20:42, dfalcantara wrote:
> ToggleAccept is confusing.  Sounds like something is being toggled except
you're
> explicitly passing in the new state.  OnPermissionChanged?

Done, changed to SetPermissionState

[gone, 2016-06-02 19:11:34]

lgtm, but you should get someone who has done C++ stuff more recently than me to
look over the C++ bits.

https://codereview.chromium.org/2019573002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java
(right):

https://codereview.chromium.org/2019573002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:70:
selectedContentSettings.add(
Don't break the line if it fits in 100 chars

https://codereview.chromium.org/2019573002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:78:
setContentSettings(mWindowAndroid, selectedArray);
Do you want this to happen if the native side doesn't sync with it?

[tsergeant, 2016-06-03 00:42:33]

Thanks, Dan.

+benwells@ for c/b/permissions and a check over the other C++ files.

https://codereview.chromium.org/2019573002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java
(right):

https://codereview.chromium.org/2019573002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:70:
selectedContentSettings.add(
On 2016/06/02 19:11:33, dfalcantara wrote:
> Don't break the line if it fits in 100 chars

Done.

https://codereview.chromium.org/2019573002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/GroupedPermissionInfoBar.java:78:
setContentSettings(mWindowAndroid, selectedArray);
On 2016/06/02 19:11:33, dfalcantara wrote:
> Do you want this to happen if the native side doesn't sync with it?

Good point, done.

[tsergeant, 2016-06-03 00:43:21]

tsergeant@chromium.org changed reviewers:
+ benwells@chromium.org

[tsergeant, 2016-06-03 00:43:22]

Actually +ben this time

(for c/b/permissions and a check over the other C++ files)

[benwells, 2016-06-05 23:53:06]

https://codereview.chromium.org/2019573002/diff/140001/chrome/browser/media/m...
File chrome/browser/media/media_stream_devices_controller.h (right):

https://codereview.chromium.org/2019573002/diff/140001/chrome/browser/media/m...
chrome/browser/media/media_stream_devices_controller.h:58: void
PermissionSplitResult(bool audio_accepted, bool video_accepted);
Nit: Can you think of a better name?

https://codereview.chromium.org/2019573002/diff/140001/chrome/browser/permiss...
File chrome/browser/permissions/grouped_permission_infobar_delegate.cc (right):

https://codereview.chromium.org/2019573002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/grouped_permission_infobar_delegate.cc:15:
accept_states_.resize(types_.size(), true);
Nit: Can you do this in the initializer list, like accept_states_(types_.size(),
true)?

[tsergeant, 2016-06-06 00:38:30]

https://codereview.chromium.org/2019573002/diff/140001/chrome/browser/media/m...
File chrome/browser/media/media_stream_devices_controller.h (right):

https://codereview.chromium.org/2019573002/diff/140001/chrome/browser/media/m...
chrome/browser/media/media_stream_devices_controller.h:58: void
PermissionSplitResult(bool audio_accepted, bool video_accepted);
On 2016/06/05 23:53:05, benwells wrote:
> Nit: Can you think of a better name?

I've renamed to GroupedRequestFinished. Do you think that is better?

https://codereview.chromium.org/2019573002/diff/140001/chrome/browser/permiss...
File chrome/browser/permissions/grouped_permission_infobar_delegate.cc (right):

https://codereview.chromium.org/2019573002/diff/140001/chrome/browser/permiss...
chrome/browser/permissions/grouped_permission_infobar_delegate.cc:15:
accept_states_.resize(types_.size(), true);
On 2016/06/05 23:53:05, benwells wrote:
> Nit: Can you do this in the initializer list, like
accept_states_(types_.size(),
> true)?

Done.

[benwells, 2016-06-06 01:10:02]

lgtm with a nit

https://codereview.chromium.org/2019573002/diff/140001/chrome/browser/media/m...
File chrome/browser/media/media_stream_devices_controller.h (right):

https://codereview.chromium.org/2019573002/diff/140001/chrome/browser/media/m...
chrome/browser/media/media_stream_devices_controller.h:58: void
PermissionSplitResult(bool audio_accepted, bool video_accepted);
On 2016/06/06 00:38:30, tsergeant wrote:
> On 2016/06/05 23:53:05, benwells wrote:
> > Nit: Can you think of a better name?
> 
> I've renamed to GroupedRequestFinished. Do you think that is better?

Yep, much better!

https://codereview.chromium.org/2019573002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/grouped_permission_infobar_delegate.cc (right):

https://codereview.chromium.org/2019573002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/grouped_permission_infobar_delegate.cc:16:
accept_states_(types_.size(), true) {
Super nit: {} on same line to match destructor.

[tsergeant, 2016-06-06 02:43:54]

tsergeant@chromium.org changed reviewers:
+ sergeyu@chromium.org

[tsergeant, 2016-06-06 02:43:55]

+sergeyu@ for chrome/browser/media

https://codereview.chromium.org/2019573002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/grouped_permission_infobar_delegate.cc (right):

https://codereview.chromium.org/2019573002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/grouped_permission_infobar_delegate.cc:16:
accept_states_(types_.size(), true) {
On 2016/06/06 01:10:02, benwells wrote:
> Super nit: {} on same line to match destructor.

Done.

[Sergey Ulanov, 2016-06-07 21:06:54]

https://codereview.chromium.org/2019573002/diff/180001/chrome/browser/media/m...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/2019573002/diff/180001/chrome/browser/media/m...
chrome/browser/media/media_stream_devices_controller.cc:302: void
MediaStreamDevicesController::GroupedRequestFinished(bool audio_accepted,
Do we still need PermissionGranted() and PermissionDenied()?
It seems wrong to have these tree functions that all do the same thing, i.e.
handle permission request result.

[tsergeant, 2016-06-07 23:31:59]

https://codereview.chromium.org/2019573002/diff/180001/chrome/browser/media/m...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/2019573002/diff/180001/chrome/browser/media/m...
chrome/browser/media/media_stream_devices_controller.cc:302: void
MediaStreamDevicesController::GroupedRequestFinished(bool audio_accepted,
On 2016/06/07 21:06:54, Sergey Ulanov wrote:
> Do we still need PermissionGranted() and PermissionDenied()?
> It seems wrong to have these tree functions that all do the same thing, i.e.
> handle permission request result.

I think they are still necessary at the moment. On Android, we'd be able to
remove them in this patch. On desktop, though, MediaStreamDevicesController ties
into the permission bubbles framework (through PermissionBubbleRequest), which
calls PermissionBubbleRequest::PermissionGranted/PermissionDenied when a
decision is made.

The plan (following on from this patch) is to have Android also use Permission
Bubbles, which will allow us to remove this function and get back to just
PermissionGranted and PermissionDenied.

[Sergey Ulanov, 2016-06-08 18:51:29]

lgtm

[tsergeant, 2016-06-08 23:50:40]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2016-06-08 23:50:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/2019573002/#ps180001
(title: "Bracket nit")

[commit-bot: I haz the power, 2016-06-08 23:51:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2019573002/180001

[commit-bot: I haz the power, 2016-06-09 01:15:29]

Committed patchset #5 (id:180001)

[commit-bot: I haz the power, 2016-06-09 01:16:59]

Description was changed from

==========
Permissions: Allow control of individual requests in media permission infobars

This adds toggle switches to Camera and Microphone permission requests,
allowing the permissions to be allowed/blocked independently of each
other.

BUG=458606
==========

to

==========
Permissions: Allow control of individual requests in media permission infobars

This adds toggle switches to Camera and Microphone permission requests,
allowing the permissions to be allowed/blocked independently of each
other.

BUG=458606

Committed: https://crrev.com/2d1cc3ee786f0135b47d1258de67e166ac4eac9c
Cr-Commit-Position: refs/heads/master@{#398756}
==========

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

Patchset 5 (id:??) landed as
https://crrev.com/2d1cc3ee786f0135b47d1258de67e166ac4eac9c
Cr-Commit-Position: refs/heads/master@{#398756}