Disable permissions dialog in VR

Disable permissions dialog in VR

When in VR, permission request will be disabled so the user will
not be able to give new permissions.

BUG=715613
Review-Url: https://codereview.chromium.org/2904623002
Cr-Commit-Position: refs/heads/master@{#477972}
Committed: https://chromium.googlesource.com/chromium/src/+/ef246e190183ebadebd89dd7e88370231f8d49be

[asimjour1, 2017-05-24 12:49:19]

asimjour@chromium.org changed reviewers:
+ felt@chromium.org

[asimjour1, 2017-05-24 12:49:20]

Please take a look

[felt, 2017-05-24 14:10:52]

Description was changed from

==========
Disable permissions dialog in VR

When in VR, permission request will be disabled so the user will
not be able to give new permissions.

BUG=715613
==========

to

==========
Disable permissions dialog in VR

When in VR, permission request will be disabled so the user will
not be able to give new permissions.

BUG=715613
==========

[felt, 2017-05-24 14:10:52]

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

[felt, 2017-05-24 14:11:06]

lgtm. raymes, do you know if there are other places they will need to change?

[raymes, 2017-05-24 22:07:45]

https://codereview.chromium.org/2904623002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/2904623002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_manager.cc:321: #endif  //
BUILDFLAG(ENABLE_VR)
This will cancel all permission requests, even if they can be answered without a
prompt. e.g. if the user had previously granted geolocation, to the site, it
will still be denied. Is that what we want? 

Also this won't cover USB/Bluetooth as they use a different codepath right now.

We should also add a unittest for this check :)

[raymes, 2017-05-24 22:13:43]

https://codereview.chromium.org/2904623002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/2904623002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_manager.cc:321: #endif  //
BUILDFLAG(ENABLE_VR)
On 2017/05/24 22:07:45, raymes wrote:
> This will cancel all permission requests, even if they can be answered without
a
> prompt. e.g. if the user had previously granted geolocation, to the site, it
> will still be denied. Is that what we want? 
> 

I just noticed the related thread. I think the lack of indicators is a good
reason to cancel the request when in VR, even if it has been previously granted.
If the request is made prior to going into VR the grant will extend into VR
which seems ok.

> Also this won't cover USB/Bluetooth as they use a different codepath right
now.
> 
> We should also add a unittest for this check :)

[asimjour1, 2017-05-29 18:41:57]

https://codereview.chromium.org/2904623002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/2904623002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_manager.cc:321: #endif  //
BUILDFLAG(ENABLE_VR)
On 2017/05/24 22:13:43, raymes wrote:
> On 2017/05/24 22:07:45, raymes wrote:
> > This will cancel all permission requests, even if they can be answered
without
> a
> > prompt. e.g. if the user had previously granted geolocation, to the site, it
> > will still be denied. Is that what we want? 
> > 
> 
> I just noticed the related thread. I think the lack of indicators is a good
> reason to cancel the request when in VR, even if it has been previously
granted.
> If the request is made prior to going into VR the grant will extend into VR
> which seems ok.
> 
> > Also this won't cover USB/Bluetooth as they use a different codepath right
> now.
> > 
> > We should also add a unittest for this check :)
> 

Unittest is added.

Bluetooth chooser is disabled in Chrome (TabWebContentsDelegateAndroid)  using
VrTabHelper.

[raymes, 2017-05-30 00:32:33]

Thanks, looks good, just some small nits.

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

https://codereview.chromium.org/2904623002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager_unittest.cc:115: void TearDown()
override { RenderViewHostTestHarness::TearDown(); }
nit: methods always go above member variables

https://codereview.chromium.org/2904623002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager_unittest.cc:422:
base::Unretained(this))));
Please check callback_called() and callback_result() in both these cases

[asimjour1, 2017-05-30 18:27:45]

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

https://codereview.chromium.org/2904623002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager_unittest.cc:115: void TearDown()
override { RenderViewHostTestHarness::TearDown(); }
On 2017/05/30 00:32:32, raymes wrote:
> nit: methods always go above member variables

Done.

https://codereview.chromium.org/2904623002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager_unittest.cc:422:
base::Unretained(this))));
On 2017/05/30 00:32:32, raymes wrote:
> Please check callback_called() and callback_result() in both these cases

Done.

[raymes, 2017-05-31 00:56:00]

lgtm

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

https://codereview.chromium.org/2904623002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.cc:299: }
nit: you can just do this before we create the PendingRequest (on line ~287) and
avoid calling CancelPermissionRequest.

if (vr_shell::VrTabHelper::IsInVr(web_contents)) {
  callback.Run(std::vector<ContentSetting>(permissions.size(),
CONTENT_SETTING_BLOCK));
  return kNoPendingOperation;
}

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

https://codereview.chromium.org/2904623002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager_unittest.cc:108: void TearDown()
override { RenderViewHostTestHarness::TearDown(); }
nit: sorry I didn't notice this before, but these functions shouldn't be needed.
They only call the superclass functions which will be called anyway. Please
delete these 2 lines.

[asimjour1, 2017-05-31 15:06:12]

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

https://codereview.chromium.org/2904623002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.cc:299: }
On 2017/05/31 00:56:00, raymes wrote:
> nit: you can just do this before we create the PendingRequest (on line ~287)
and
> avoid calling CancelPermissionRequest.
> 
> if (vr_shell::VrTabHelper::IsInVr(web_contents)) {
>   callback.Run(std::vector<ContentSetting>(permissions.size(),
> CONTENT_SETTING_BLOCK));
>   return kNoPendingOperation;
> }

Done. I moved it to line 289 (web_contents is needed here)

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

https://codereview.chromium.org/2904623002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager_unittest.cc:108: void TearDown()
override { RenderViewHostTestHarness::TearDown(); }
On 2017/05/31 00:56:00, raymes wrote:
> nit: sorry I didn't notice this before, but these functions shouldn't be
needed.
> They only call the superclass functions which will be called anyway. Please
> delete these 2 lines.

Done.

[asimjour1, 2017-05-31 15:06:33]

The CQ bit was checked by asimjour@chromium.org

[asimjour1, 2017-05-31 15:06:33]

The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org,
raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2904623002/#ps60001
(title: "CancelPermissionRequest is removed")

[commit-bot: I haz the power, 2017-05-31 15:06:58]

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-31 15:28:12]

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

[commit-bot: I haz the power, 2017-05-31 15:28:13]

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

[raymes, 2017-05-31 23:10:08]

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

https://codereview.chromium.org/2904623002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.cc:292: if
(vr_shell::VrTabHelper::IsInVr(web_contents)) {
This is missing:
 callback.Run(std::vector<ContentSetting>(permissions.size(),
CONTENT_SETTING_BLOCK));

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

https://codereview.chromium.org/2904623002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager_unittest.cc:396:
PermissionType::GEOLOCATION, web_contents()->GetMainFrame(),
nit: either use |contents| everywhere or web_contents() everywhere

[asimjour1, 2017-06-06 17:58:56]

Sorry, I had a mistake before that was causing the test to fail. The new patch
addresses the comments and fixes the threading issue.

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

https://codereview.chromium.org/2904623002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.cc:292: if
(vr_shell::VrTabHelper::IsInVr(web_contents)) {
On 2017/05/31 23:10:07, raymes wrote:
> This is missing:
>  callback.Run(std::vector<ContentSetting>(permissions.size(),
> CONTENT_SETTING_BLOCK));

Done.

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

https://codereview.chromium.org/2904623002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager_unittest.cc:396:
PermissionType::GEOLOCATION, web_contents()->GetMainFrame(),
On 2017/05/31 23:10:08, raymes wrote:
> nit: either use |contents| everywhere or web_contents() everywhere

Done.

[raymes, 2017-06-06 23:34:42]

lgtm

https://codereview.chromium.org/2904623002/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_manager_unittest.cc (right):

https://codereview.chromium.org/2904623002/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager_unittest.cc:418:
SetPermission(CONTENT_SETTINGS_TYPE_NOTIFICATIONS, CONTENT_SETTING_ALLOW);
nit: this isn't needed

https://codereview.chromium.org/2904623002/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager_unittest.cc:429:
SetPermission(CONTENT_SETTINGS_TYPE_NOTIFICATIONS, CONTENT_SETTING_ALLOW);
nit: this isn't needed

[asimjour1, 2017-06-07 15:11:26]

https://codereview.chromium.org/2904623002/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_manager_unittest.cc (right):

https://codereview.chromium.org/2904623002/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager_unittest.cc:418:
SetPermission(CONTENT_SETTINGS_TYPE_NOTIFICATIONS, CONTENT_SETTING_ALLOW);
On 2017/06/06 23:34:42, raymes wrote:
> nit: this isn't needed

Done.

https://codereview.chromium.org/2904623002/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager_unittest.cc:429:
SetPermission(CONTENT_SETTINGS_TYPE_NOTIFICATIONS, CONTENT_SETTING_ALLOW);
On 2017/06/06 23:34:42, raymes wrote:
> nit: this isn't needed

Done.

[asimjour1, 2017-06-07 15:12:58]

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

[commit-bot: I haz the power, 2017-06-07 15:13:18]

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-06-07 15:26:48]

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

[commit-bot: I haz the power, 2017-06-07 15:26:50]

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

[asimjour1, 2017-06-07 15:41:19]

The CQ bit was checked by asimjour@chromium.org

[asimjour1, 2017-06-07 15:41:19]

The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org,
raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2904623002/#ps120001
(title: "kNoPendingOperation behind ENABLE_VR flag")

[commit-bot: I haz the power, 2017-06-07 15:42:17]

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-06-07 16:40:11]

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

[commit-bot: I haz the power, 2017-06-07 16:40:12]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[asimjour1, 2017-06-07 18:03:32]

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

[asimjour1, 2017-06-07 18:05:16]

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

[commit-bot: I haz the power, 2017-06-07 18:05:39]

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-06-07 19:28:13]

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

[commit-bot: I haz the power, 2017-06-07 19:28:15]

Dry run: This issue passed the CQ dry run.

[Ian Vollick, 2017-06-07 19:40:46]

On 2017/06/07 16:40:12, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

vr_shell/ lgtm with nit.

Please add a comment explaining when vr_tab_helper can be NULL.

[asimjour1, 2017-06-07 19:55:04]

The CQ bit was checked by asimjour@chromium.org

[asimjour1, 2017-06-07 19:55:05]

The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org,
raymes@chromium.org, vollick@chromium.org
Link to the patchset: https://codereview.chromium.org/2904623002/#ps160001
(title: "Add comment")

[commit-bot: I haz the power, 2017-06-07 19:55:31]

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-06-07 21:14:19]

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

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

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_...)

[asimjour1, 2017-06-08 12:48:36]

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

[commit-bot: I haz the power, 2017-06-08 12:48:48]

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-06-08 13:38:20]

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

[commit-bot: I haz the power, 2017-06-08 13:38:21]

Dry run: This issue passed the CQ dry run.

[asimjour, 2017-06-08 15:01:46]

The CQ bit was checked by asimjour@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 15:02:04]

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-06-08 15:05:41]

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

[commit-bot: I haz the power, 2017-06-08 15:05:42]

Dry run: This issue passed the CQ dry run.

[asimjour1, 2017-06-08 15:09:38]

The CQ bit was checked by asimjour@chromium.org

[commit-bot: I haz the power, 2017-06-08 15:09:53]

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-06-08 15:14:22]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496934578398280,
"parent_rev": "5ad826e376bfff71fb0864ef84a6f3439d198391", "commit_rev":
"ef246e190183ebadebd89dd7e88370231f8d49be"}

[commit-bot: I haz the power, 2017-06-08 15:14:35]

Description was changed from

==========
Disable permissions dialog in VR

When in VR, permission request will be disabled so the user will
not be able to give new permissions.

BUG=715613
==========

to

==========
Disable permissions dialog in VR

When in VR, permission request will be disabled so the user will
not be able to give new permissions.

BUG=715613

Review-Url: https://codereview.chromium.org/2904623002
Cr-Commit-Position: refs/heads/master@{#477972}
Committed:
https://chromium.googlesource.com/chromium/src/+/ef246e190183ebadebd89dd7e883...
==========

[commit-bot: I haz the power, 2017-06-08 15:14:37]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/ef246e190183ebadebd89dd7e883...