Remove usage of the generic multiple permissions update string.

Remove usage of the generic multiple permissions update string.

The only combination of multiple permissions to update that is ever used
is microphone and camera. This change introduces a custom string just
for that case and other usages are treated as an error (and CHECKed).

BUG=710362
Review-Url: https://codereview.chromium.org/2872853004
Cr-Commit-Position: refs/heads/master@{#473297}
Committed: https://chromium.googlesource.com/chromium/src/+/036e0cf6141f26632ddfe2aec1867418d3acdf9c

[benwells, 2017-05-09 13:47:59]

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

[commit-bot: I haz the power, 2017-05-09 13:48:22]

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, 2017-05-09 15:16:12]

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

[commit-bot: I haz the power, 2017-05-09 15:16:13]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[benwells, 2017-05-11 10:49:25]

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

[commit-bot: I haz the power, 2017-05-11 10:49:50]

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, 2017-05-11 12:14:30]

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

[commit-bot: I haz the power, 2017-05-11 12:14:30]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[benwells, 2017-05-11 13:12:10]

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

[benwells, 2017-05-11 13:12:10]

raymes - i'm not sure about using CHECK, but the alternative is to keep a known
bad string that we would hopefully never use. WDYT?

[raymes, 2017-05-14 23:22:25]

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

https://codereview.chromium.org/2872853004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java:107:
return getDeniedPermissionResourceId(contentSettingsTypesToPermissionsMap,
permission);
A few optional suggestions:

private static int updateDeniedPermissionResourceId(...) {
  int newDeniedStringId = ...;

  if (deniedCount == 1)
    return newDeniedStringId;

  boolean deniedCamera = 
      currentDeniedStringId == R.string.infobar_missing_camera_permission_text
|| 
      newDeniedStringId == R.string.infobar_missing_camera_permission_text;

  boolean deniedMic = 
      currentDeniedStringId ==
R.string.infobar_missing_microphone_permission_text || 
      newDeniedStringId == R.string.infobar_missing_microphone_permission_text;

  if (deniedMic && deniedCamera)
    return R.string.infobar_missing_microphone_camera_permissions_text;

  assert false : ...;
  return -1;
}

https://codereview.chromium.org/2872853004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java:146:
if (deniedCount > 0 && deniedStringId != -1 && requestableCount > 0
Could this happen?

https://codereview.chromium.org/2872853004/diff/40001/chrome/browser/permissi...
File chrome/browser/permissions/permission_update_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2872853004/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:73:
CHECK(false);
> raymes - i'm not sure about using CHECK, but the alternative is to keep a
known
> bad string that we would hopefully never use. WDYT?

Are you worried about people using this to request multiple permissions in the
future or that could actually happen now?

I feel fairly confident that it couldn't happen now. I would lean toward a
DCHECK.

[benwells, 2017-05-16 11:50:03]

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

[commit-bot: I haz the power, 2017-05-16 11:50:26]

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, 2017-05-16 13:23:41]

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

[commit-bot: I haz the power, 2017-05-16 13:23:42]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[benwells, 2017-05-16 15:21:31]

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

https://codereview.chromium.org/2872853004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java:107:
return getDeniedPermissionResourceId(contentSettingsTypesToPermissionsMap,
permission);
On 2017/05/14 23:22:24, raymes wrote:
> A few optional suggestions:
> 
> private static int updateDeniedPermissionResourceId(...) {
>   int newDeniedStringId = ...;
> 
>   if (deniedCount == 1)
>     return newDeniedStringId;
> 
>   boolean deniedCamera = 
>       currentDeniedStringId == R.string.infobar_missing_camera_permission_text
> || 
>       newDeniedStringId == R.string.infobar_missing_camera_permission_text;
> 
>   boolean deniedMic = 
>       currentDeniedStringId ==
> R.string.infobar_missing_microphone_permission_text || 
>       newDeniedStringId ==
R.string.infobar_missing_microphone_permission_text;
> 
>   if (deniedMic && deniedCamera)
>     return R.string.infobar_missing_microphone_camera_permissions_text;
> 
>   assert false : ...;
>   return -1;
> }

I've checked for ==1 first. I need to keep the check if the two strings are
equal, because if it is location that is denied it will come in twice with the
same string returned each time.

For checking that in the other case it is only mic and camera, I have a slight
preference to use the hash set because it is less repetition of long constants /
a bit more readable. Do you think it is important?

https://codereview.chromium.org/2872853004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java:146:
if (deniedCount > 0 && deniedStringId != -1 && requestableCount > 0
On 2017/05/14 23:22:24, raymes wrote:
> Could this happen?

Could this if be true? Or could deniedStringId == -1?

For the first question, yes. If the user says no to the chrome permission, this
block then shows the infobar again, and if the user says "Update permissions"
will ask for chrome permission again. It will keep doing this in a loop until
the user doesn't say "Update permissions" or says yes to the chrome permission.

For the second question, no it shouldn't happen. But if an error is ever
introduced in calling code, it might and we only have the asserts to stop the
code otherwise.

https://codereview.chromium.org/2872853004/diff/40001/chrome/browser/permissi...
File chrome/browser/permissions/permission_update_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2872853004/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_update_infobar_delegate_android.cc:73:
CHECK(false);
On 2017/05/14 23:22:24, raymes wrote:
> > raymes - i'm not sure about using CHECK, but the alternative is to keep a
> known
> > bad string that we would hopefully never use. WDYT?
> 
> Are you worried about people using this to request multiple permissions in the
> future or that could actually happen now?
> 
> I feel fairly confident that it couldn't happen now. I would lean toward a
> DCHECK.

I don't think it can happen now. But if it somehow does I don't want bad things
to happen in builds without DCHECK (e.g. a buffer overrun). I could just return
nullptr and it might crash later with a nullptr deref, or more probably just do
nothing (i.e. permission requests would be ignored).

Hmm it's probably safest to DCHECK and otherwise keep going. I'll do that.

[raymes, 2017-05-17 01:21:43]

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

https://codereview.chromium.org/2872853004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java:107:
return getDeniedPermissionResourceId(contentSettingsTypesToPermissionsMap,
permission);
On 2017/05/16 15:21:30, benwells wrote:
> On 2017/05/14 23:22:24, raymes wrote:
> > A few optional suggestions:
> > 
> > private static int updateDeniedPermissionResourceId(...) {
> >   int newDeniedStringId = ...;
> > 
> >   if (deniedCount == 1)
> >     return newDeniedStringId;
> > 
> >   boolean deniedCamera = 
> >       currentDeniedStringId ==
R.string.infobar_missing_camera_permission_text
> > || 
> >       newDeniedStringId == R.string.infobar_missing_camera_permission_text;
> > 
> >   boolean deniedMic = 
> >       currentDeniedStringId ==
> > R.string.infobar_missing_microphone_permission_text || 
> >       newDeniedStringId ==
> R.string.infobar_missing_microphone_permission_text;
> > 
> >   if (deniedMic && deniedCamera)
> >     return R.string.infobar_missing_microphone_camera_permissions_text;
> > 
> >   assert false : ...;
> >   return -1;
> > }
> 
> I've checked for ==1 first. I need to keep the check if the two strings are
> equal, because if it is location that is denied it will come in twice with the
> same string returned each time.
> 
> For checking that in the other case it is only mic and camera, I have a slight
> preference to use the hash set because it is less repetition of long constants
/
> a bit more readable. Do you think it is important?

It's not important :) I think it just comes down to preference (that's why I
listed them as optional suggestions)

https://codereview.chromium.org/2872853004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java:146:
if (deniedCount > 0 && deniedStringId != -1 && requestableCount > 0
On 2017/05/16 15:21:30, benwells wrote:
> On 2017/05/14 23:22:24, raymes wrote:
> > Could this happen?
> 
> Could this if be true? Or could deniedStringId == -1?
> 
> For the first question, yes. If the user says no to the chrome permission,
this
> block then shows the infobar again, and if the user says "Update permissions"
> will ask for chrome permission again. It will keep doing this in a loop until
> the user doesn't say "Update permissions" or says yes to the chrome
permission.
> 
> For the second question, no it shouldn't happen. But if an error is ever
> introduced in calling code, it might and we only have the asserts to stop the
> code otherwise.

Sorry I should have been more clear :) I was only really asking if
deniedStringId could == -1. What is the benefit of not taking this codepath if
it turns out to be -1? I guess I feel that in either case there is a bug in the
code and that it might be more obvious if it crashes or displays some nonsense?
It also just makes the code slightly simpler. What do you think?

[benwells, 2017-05-17 01:45:37]

On 2017/05/17 01:21:43, raymes wrote:
>
https://codereview.chromium.org/2872853004/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java
> (right):
> 
>
https://codereview.chromium.org/2872853004/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java:107:
> return getDeniedPermissionResourceId(contentSettingsTypesToPermissionsMap,
> permission);
> On 2017/05/16 15:21:30, benwells wrote:
> > On 2017/05/14 23:22:24, raymes wrote:
> > > A few optional suggestions:
> > > 
> > > private static int updateDeniedPermissionResourceId(...) {
> > >   int newDeniedStringId = ...;
> > > 
> > >   if (deniedCount == 1)
> > >     return newDeniedStringId;
> > > 
> > >   boolean deniedCamera = 
> > >       currentDeniedStringId ==
> R.string.infobar_missing_camera_permission_text
> > > || 
> > >       newDeniedStringId ==
R.string.infobar_missing_camera_permission_text;
> > > 
> > >   boolean deniedMic = 
> > >       currentDeniedStringId ==
> > > R.string.infobar_missing_microphone_permission_text || 
> > >       newDeniedStringId ==
> > R.string.infobar_missing_microphone_permission_text;
> > > 
> > >   if (deniedMic && deniedCamera)
> > >     return R.string.infobar_missing_microphone_camera_permissions_text;
> > > 
> > >   assert false : ...;
> > >   return -1;
> > > }
> > 
> > I've checked for ==1 first. I need to keep the check if the two strings are
> > equal, because if it is location that is denied it will come in twice with
the
> > same string returned each time.
> > 
> > For checking that in the other case it is only mic and camera, I have a
slight
> > preference to use the hash set because it is less repetition of long
constants
> /
> > a bit more readable. Do you think it is important?
> 
> It's not important :) I think it just comes down to preference (that's why I
> listed them as optional suggestions)
> 
>
https://codereview.chromium.org/2872853004/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java:146:
> if (deniedCount > 0 && deniedStringId != -1 && requestableCount > 0
> On 2017/05/16 15:21:30, benwells wrote:
> > On 2017/05/14 23:22:24, raymes wrote:
> > > Could this happen?
> > 
> > Could this if be true? Or could deniedStringId == -1?
> > 
> > For the first question, yes. If the user says no to the chrome permission,
> this
> > block then shows the infobar again, and if the user says "Update
permissions"
> > will ask for chrome permission again. It will keep doing this in a loop
until
> > the user doesn't say "Update permissions" or says yes to the chrome
> permission.
> > 
> > For the second question, no it shouldn't happen. But if an error is ever
> > introduced in calling code, it might and we only have the asserts to stop
the
> > code otherwise.
> 
> Sorry I should have been more clear :) I was only really asking if
> deniedStringId could == -1. What is the benefit of not taking this codepath if
> it turns out to be -1? I guess I feel that in either case there is a bug in
the
> code and that it might be more obvious if it crashes or displays some
nonsense?
> It also just makes the code slightly simpler. What do you think?

OK, simplified by removing this.

[raymes, 2017-05-17 01:47:58]

lgtm, thanks!

[benwells, 2017-05-17 02:29:15]

On 2017/05/17 01:47:58, raymes wrote:
> lgtm, thanks!

Just checked, the worse that can happen if this stuff is used incorrectly is a
crash. No security vulns here. Sending to owners now...

[benwells, 2017-05-17 02:29:29]

Description was changed from

==========
Remove usage of the generic multiple permissions update string.

THe only combination of multiple permissions to update that is ever used
is microphone and camera. This change introduces a custom string just
for that case and other usages are treated as an error (and CHECKed).

BUG=710362
==========

to

==========
Remove usage of the generic multiple permissions update string.

The only combination of multiple permissions to update that is ever used
is microphone and camera. This change introduces a custom string just
for that case and other usages are treated as an error (and CHECKed).

BUG=710362
==========

[benwells, 2017-05-17 02:30:29]

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

[benwells, 2017-05-17 02:30:29]

+tedchoc as owner of the java code and android strings

[Ted C, 2017-05-17 14:19:03]

https://codereview.chromium.org/2872853004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java
(right):

https://codereview.chromium.org/2872853004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java:137:
deniedStringId = updateDeniedPermissionResourceId(deniedStringId,
With the multiple hops, I found this somewhat hard to follow.  What do you think
about something like the following:

First pull out something like:

private static int getContentSettingType(SparseArray<String[]>
contentSettingsTypesToPermissionsMap, String permission) {
   return // for loop logic in getDeniedPermissionResourceId
}

Then here we would just collect all of the denied content settings in a Set. 
Then once we iterate over the permission list, it would be something like:

if (missingContentSettings.size() == 2 && .contains(CAMERA) && .contains(MIC)) {
} else if (missingContentSettings.size() == 1) {
    if (.contains(LOCATION)
    else if (.contains(CAMERA)
    else if (.contains(MIC)
} else {
    assert false : "Invalid combination of missing content settings: " +
missingContentSettings;
}

Also, what are your thoughts on if the permission is denied and we can't request
it?  Should we include it in the permission update string consideration?

[benwells, 2017-05-19 01:46:21]

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

[commit-bot: I haz the power, 2017-05-19 01:47:31]

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, 2017-05-19 03:12:29]

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

[commit-bot: I haz the power, 2017-05-19 03:12:30]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-05-19 03:13:34]

https://codereview.chromium.org/2872853004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java
(right):

https://codereview.chromium.org/2872853004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java:137:
deniedStringId = updateDeniedPermissionResourceId(deniedStringId,
On 2017/05/17 14:19:02, Ted C wrote:
> With the multiple hops, I found this somewhat hard to follow.  What do you
think
> about something like the following:
> 
> First pull out something like:
> 
> private static int getContentSettingType(SparseArray<String[]>
> contentSettingsTypesToPermissionsMap, String permission) {
>    return // for loop logic in getDeniedPermissionResourceId
> }
> 
> Then here we would just collect all of the denied content settings in a Set. 
> Then once we iterate over the permission list, it would be something like:
> 
> if (missingContentSettings.size() == 2 && .contains(CAMERA) && .contains(MIC))
{
> } else if (missingContentSettings.size() == 1) {
>     if (.contains(LOCATION)
>     else if (.contains(CAMERA)
>     else if (.contains(MIC)
> } else {
>     assert false : "Invalid combination of missing content settings: " +
> missingContentSettings;
> }

That is so much nicer :)

> 
> Also, what are your thoughts on if the permission is denied and we can't
request
> it?  Should we include it in the permission update string consideration?

How about if any of the denied permissions aren't requestable we don't bother
asking? Alternatively, we could just ask for the ones that are requestable and
ignore the rest.

I think the only way we can get in this state is if a site requests mic+camera,
chrome needs both of these but the user has told Android to only let chrome asks
for one of them. This feels like a rare edge case, and if it happens I don't
think it matters too much.

https://codereview.chromium.org/2872853004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java
(right):

https://codereview.chromium.org/2872853004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java:138:
!= -1 : "Invalid combination of missing content settings: "
This looks weird ... it's what git cl format did ...

[Ted C, 2017-05-19 17:19:23]

lgtm

https://codereview.chromium.org/2872853004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java
(right):

https://codereview.chromium.org/2872853004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/permissions/AndroidPermissionRequester.java:138:
!= -1 : "Invalid combination of missing content settings: "
On 2017/05/19 03:13:33, benwells wrote:
> This looks weird ... it's what git cl format did ...

I've started to accept weird things to avoid trying to fight clang format.  This
isn't so egregious IMO

[benwells, 2017-05-19 20:27:00]

The CQ bit was checked by benwells@chromium.org

[benwells, 2017-05-19 20:27:01]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2872853004/#ps100001
(title: "Feedback")

[commit-bot: I haz the power, 2017-05-19 20:27:34]

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, 2017-05-19 20:34:25]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495225620923160,
"parent_rev": "72a888510bce89f8ff61cdbc4f11583c1a66708d", "commit_rev":
"036e0cf6141f26632ddfe2aec1867418d3acdf9c"}

[commit-bot: I haz the power, 2017-05-19 20:34:39]

Description was changed from

==========
Remove usage of the generic multiple permissions update string.

The only combination of multiple permissions to update that is ever used
is microphone and camera. This change introduces a custom string just
for that case and other usages are treated as an error (and CHECKed).

BUG=710362
==========

to

==========
Remove usage of the generic multiple permissions update string.

The only combination of multiple permissions to update that is ever used
is microphone and camera. This change introduces a custom string just
for that case and other usages are treated as an error (and CHECKed).

BUG=710362

Review-Url: https://codereview.chromium.org/2872853004
Cr-Commit-Position: refs/heads/master@{#473297}
Committed:
https://chromium.googlesource.com/chromium/src/+/036e0cf6141f26632ddfe2aec186...
==========

[commit-bot: I haz the power, 2017-05-19 20:34:41]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/036e0cf6141f26632ddfe2aec186...