[vr] Deny permission requests in VR mode

[vr] Deny permission requests in VR mode

These currently imply monocularly rendered content which is quite
disruptive in VR. Until the UI for these requests is properly supported
in VR, we should deny them.

BUG=728421

[Ian Vollick, 2017-07-07 13:07:01]

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

[commit-bot: I haz the power, 2017-07-07 13:07:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ian Vollick, 2017-07-07 14:01:05]

Description was changed from

==========
[vr] Deny permission requests in VR mode

These currently imply monocularly rendered content which is quite
disruptive in VR. Until the UI for these requests is properly supported
in VR, we should deny the permission requests.

BUG=728421
==========

to

==========
[vr] Deny permission requests in VR mode

These currently imply monocularly rendered content which is quite
disruptive in VR. Until the UI for these requests is properly supported
in VR, we should deny them.

BUG=728421
==========

[commit-bot: I haz the power, 2017-07-07 15:01:03]

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

[commit-bot: I haz the power, 2017-07-07 15:01:04]

Dry run: This issue passed the CQ dry run.

[Ian Vollick, 2017-07-10 13:09:35]

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

[Ian Vollick, 2017-07-10 13:09:36]

Hey! One issue we'd run into was that some permission bubbles, although not
displayed in VR, would be present upon the user's return to clank. It seemed
that suppressing the request in the PRM (and preventing it from being enqueued
at all) would do the trick.

Please let me know if there's a better way to go about this.

[raymes, 2017-07-11 01:43:22]

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

[raymes, 2017-07-11 01:43:23]

Hmm, it doesn't feel ideal to do this at the PermissionRequestManager level
because at that point the code assumes that the only reason something will be
denied/dismissed is because a user action is taken. That has practical
consequences for permission requests, for example if something is dismissed
several times, it won't be allowed to be requested again by the site for 7 days.
It also impacts metrics recorded. But the alternative of putting checks at every
callsite doesn't seem to scale well either! 

In the long term we probably want everything to go through PermissionManager. In
the short term maybe we can add a special function to PermissionRequest which is
called when a request is added that can't be handled right now? 

+timloh what are your thoughts?

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

https://codereview.chromium.org/2975593002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.cc:145:
request->PermissionDenied();
nit: you may want to change this to Cancelled() since it wasn't an active denial
by the user.

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

https://codereview.chromium.org/2975593002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager_unittest.cc:665:
manager_->set_permission_requests_disabled(true);
I think it would be better to simulate VR being enabled, as in
PermissionManagerTest.SuppressPermissionRequests
(permission_manager_unittest.cc). That would avoid needing the virtual function
and test subclass.

[raymes, 2017-07-11 01:44:38]

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

https://codereview.chromium.org/2975593002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.cc:145:
request->PermissionDenied();
On 2017/07/11 01:43:23, raymes wrote:
> nit: you may want to change this to Cancelled() since it wasn't an active
denial
> by the user.

Ignore this - after I thought through it more I think it's better to add a
separate function to PermissionRequest for this case.

[raymes, 2017-07-11 03:16:28]

> In the long term we probably want everything to go through PermissionManager.
In
> the short term maybe we can add a special function to PermissionRequest which
is
> called when a request is added that can't be handled right now? 

A different idea would be to return a bool from AddRequest indicating that the
request could/could not be added and use WARN_UNUSED_RESULT to ensure callsites
do the right thing.

[Timothy Loh, 2017-07-11 03:37:54]

I chatted with Raymes offline and we think it'd be nicer for the
non-PermissionManager callsites to explicitly handle VR and for the
PermissionRequestManager to DCHECK that VR is off. AIUI there are only 3 request
types that use the PRM directly: QuotaPermissionRequest,
RegisterProtocolHandlerPermissionRequest and DownloadPermissionRequest. We'll
make new permissions added in the future go through the PermissionManager so the
check won't need to be added to more places later.