Reflect device status in geolocation permissions.request calls.

Reflect device status in geolocation permissions.request calls.

In some situations origins may have geolocation permission, but the user
will see prompts, or geolocation won't work. In these cases, the
permissions API should indicate this, with either ASK or DENIED.

The situations are as follows:
1. When the device has location turned off
2. When Chrome does not have geolocation permission at the Androd level.

In both of these cases, Chrome may be able to prompt the user to rectify
the problem and if possible will do so when permission is requested.

This patch does not correctly model the second situation, as the plumbing
required to see if Chrome is allowed to prompt is not implemented. In
this case prompt is always return ASK, even if Chrome cannot prompt and
requests will fail. This is better than returning GRANTED however, as
it is closer to the truth.

BUG=672301
Review-Url: https://codereview.chromium.org/2743603002
Cr-Commit-Position: refs/heads/master@{#456003}
Committed: https://chromium.googlesource.com/chromium/src/+/59547521e04b9b5ec38d51588d7eab562ac0c630

[benwells, 2017-03-09 07:23:53]

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

[commit-bot: I haz the power, 2017-03-09 07:24: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, 2017-03-09 08:34:26]

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

[commit-bot: I haz the power, 2017-03-09 08:34:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[benwells, 2017-03-09 23:16:25]

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

[raymes, 2017-03-10 02:53:31]

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context_android.cc:173:
result.content_setting = CanShowLocationSettingsDialog(
Maybe we should update the result.status here, otherwise it may be confusing for
callers (although it's not used now). Same below. If you don't want to add extra
reasons right now, you could just return UNSPECIFIED.

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context_android.cc:176: :
CONTENT_SETTING_BLOCK;
nit: This might be slightly clearer in the expanded form:
if (CanShowLocationSettingsDialog(requesting_origin, true /* user_gesture */)
  result.content_setting = CONTENT_SETTING_ASK;
else
  result.content_setting = CONTENT_SETTING_BLOCK;

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context_android.h
(right):

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context_android.h:74:
PermissionResult UpdatePermissionStatusWithDeviceStatus(
Something for future thought: I wonder if this would be better as just returning
the device permission status directly and then we combine them at the higher
layer, e.g. PermissionManager. I think we may end up duplicating logic for
combining them across permissions.

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:1117:
CONTENT_SETTING_ALLOW);
nit: Should we add a case for each of these where the setting is the default
(e.g. ASK)?

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.cc:442:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
I think this may not work for MIDI anymore (need to add the constant permission
check). It's unfortunate that no tests picked up on this.

I think it would be better to call GetPermissionStatusHelper rather than
checking the context.

[benwells, 2017-03-10 04:19:20]

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context_android.cc:173:
result.content_setting = CanShowLocationSettingsDialog(
On 2017/03/10 02:53:31, raymes wrote:
> Maybe we should update the result.status here, otherwise it may be confusing
for
> callers (although it's not used now). Same below. If you don't want to add
extra
> reasons right now, you could just return UNSPECIFIED.

Done.

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context_android.cc:176: :
CONTENT_SETTING_BLOCK;
On 2017/03/10 02:53:31, raymes wrote:
> nit: This might be slightly clearer in the expanded form:
> if (CanShowLocationSettingsDialog(requesting_origin, true /* user_gesture */)
>   result.content_setting = CONTENT_SETTING_ASK;
> else
>   result.content_setting = CONTENT_SETTING_BLOCK;

I've come to terms with the ternary operator, after years of avoiding it. But
.... Done.

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context_android.h
(right):

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context_android.h:74:
PermissionResult UpdatePermissionStatusWithDeviceStatus(
On 2017/03/10 02:53:31, raymes wrote:
> Something for future thought: I wonder if this would be better as just
returning
> the device permission status directly and then we combine them at the higher
> layer, e.g. PermissionManager. I think we may end up duplicating logic for
> combining them across permissions.

Acknowledged.

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:1117:
CONTENT_SETTING_ALLOW);
On 2017/03/10 02:53:31, raymes wrote:
> nit: Should we add a case for each of these where the setting is the default
> (e.g. ASK)?

Done.

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/2743603002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.cc:442:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2017/03/10 02:53:31, raymes wrote:
> I think this may not work for MIDI anymore (need to add the constant
permission
> check). It's unfortunate that no tests picked up on this.
> 
> I think it would be better to call GetPermissionStatusHelper rather than
> checking the context.

As discussed this should all be OK as is.

[raymes, 2017-03-10 04:37:13]

lgtm thanks!

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

https://codereview.chromium.org/2743603002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.cc:448: //
GetPermissionStatusForPermissionsAPI.
One thought I had is that we may just want to leave the content-API as-is and
include the device status in it and instead just change the Chrome-API to
GetPermissionStatusNoDeviceStatus or something. Anyway not important for this
CL.

[benwells, 2017-03-10 04:39:51]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2017-03-10 04:40:06]

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-03-10 05:30:04]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489120791208840,
"parent_rev": "089c67c27a4ea8c5a1aeafcc7674bff96298a1e3", "commit_rev":
"59547521e04b9b5ec38d51588d7eab562ac0c630"}

[commit-bot: I haz the power, 2017-03-10 05:31:09]

Description was changed from

==========
Reflect device status in geolocation permissions.request calls.

In some situations origins may have geolocation permission, but the user
will see prompts, or geolocation won't work. In these cases, the
permissions API should indicate this, with either ASK or DENIED.

The situations are as follows:
1. When the device has location turned off
2. When Chrome does not have geolocation permission at the Androd level.

In both of these cases, Chrome may be able to prompt the user to rectify
the problem and if possible will do so when permission is requested.

This patch does not correctly model the second situation, as the plumbing
required to see if Chrome is allowed to prompt is not implemented. In
this case prompt is always return ASK, even if Chrome cannot prompt and
requests will fail. This is better than returning GRANTED however, as
it is closer to the truth.

BUG=672301
==========

to

==========
Reflect device status in geolocation permissions.request calls.

In some situations origins may have geolocation permission, but the user
will see prompts, or geolocation won't work. In these cases, the
permissions API should indicate this, with either ASK or DENIED.

The situations are as follows:
1. When the device has location turned off
2. When Chrome does not have geolocation permission at the Androd level.

In both of these cases, Chrome may be able to prompt the user to rectify
the problem and if possible will do so when permission is requested.

This patch does not correctly model the second situation, as the plumbing
required to see if Chrome is allowed to prompt is not implemented. In
this case prompt is always return ASK, even if Chrome cannot prompt and
requests will fail. This is better than returning GRANTED however, as
it is closer to the truth.

BUG=672301

Review-Url: https://codereview.chromium.org/2743603002
Cr-Commit-Position: refs/heads/master@{#456003}
Committed:
https://chromium.googlesource.com/chromium/src/+/59547521e04b9b5ec38d51588d7e...
==========

[commit-bot: I haz the power, 2017-03-10 05:31:10]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/59547521e04b9b5ec38d51588d7e...