VR: Update UiSceneManager with audio/video capturing flags

VR: Update UiSceneManager with audio/video capturing flags

UiSceneManager is updated to receive the capturing flags and show
the indicators accordingly.

BUG=722836
Review-Url: https://codereview.chromium.org/2890563003
Cr-Commit-Position: refs/heads/master@{#472592}
Committed: https://chromium.googlesource.com/chromium/src/+/60119e7d384db63360b0e6f4f50edb7dcebf57e5

[asimjour1, 2017-05-16 18:56:29]

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

[asimjour1, 2017-05-16 18:56:30]

PTAL

[cjgrant, 2017-05-16 19:09:27]

cjgrant@chromium.org changed reviewers:
+ cjgrant@chromium.org

[cjgrant, 2017-05-16 19:09:28]

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:292:
audio_input_indicator_->set_visible(is_capturing);
Should these members be called audio_capture_indicator, etc, to match?

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.h (right):

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.h:46: void
SetAudioCapturingIndicator(bool is_capturing);
These could go up with the other setters.  We might as well remove the
"currently stubbed" as it's no longer true, and just alphabetize all the Set
methods.

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.h:48: void
SetScreenCapturingIndicator(bool is_capturing);
Up to you, but based on method name, argument "bool enabled" seems plenty clear
and shorter. :)

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:554:
gl_thread_->task_runner()->PostTask(
These calls should go through the ui_ interface.  See other similar calls.  We
should only be using this direct approach for things that are directly dependent
on the GL implementation.  Note that you'll have to add methods to the interface
as well.

The point of all this is to keep this module cleaner, hide the cross-thread
implementation, and make pieces more testable.

[acondor_, 2017-05-16 19:11:40]

acondor@google.com changed reviewers:
+ acondor@google.com

[acondor_, 2017-05-16 19:11:41]

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:292:
audio_input_indicator_->set_visible(is_capturing);
On 2017/05/16 19:09:27, cjgrant wrote:
> Should these members be called audio_capture_indicator, etc, to match?

yes, my bad.

[asimjour1, 2017-05-16 20:13:56]

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:292:
audio_input_indicator_->set_visible(is_capturing);
On 2017/05/16 19:09:27, cjgrant wrote:
> Should these members be called audio_capture_indicator, etc, to match?

Done.

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.h (right):

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.h:46: void
SetAudioCapturingIndicator(bool is_capturing);
On 2017/05/16 19:09:27, cjgrant wrote:
> These could go up with the other setters.  We might as well remove the
> "currently stubbed" as it's no longer true, and just alphabetize all the Set
> methods.

Moved them next to the other setters, but I rather to keep them together instead
of sorting.

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.h:48: void
SetScreenCapturingIndicator(bool is_capturing);
On 2017/05/16 19:09:27, cjgrant wrote:
> Up to you, but based on method name, argument "bool enabled" seems plenty
clear
> and shorter. :)

Done.

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2890563003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:554:
gl_thread_->task_runner()->PostTask(
On 2017/05/16 19:09:27, cjgrant wrote:
> These calls should go through the ui_ interface.  See other similar calls.  We
> should only be using this direct approach for things that are directly
dependent
> on the GL implementation.  Note that you'll have to add methods to the
interface
> as well.
> 
> The point of all this is to keep this module cleaner, hide the cross-thread
> implementation, and make pieces more testable.

Done.

[cjgrant, 2017-05-16 21:31:39]

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_interface.h (right):

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.h:35: virtual void
SetVideoCapturingIndicator(bool enabled) = 0;
These need to be added to the scene manager unit test mock as well.  To verify:

- build vr_shell_unittests
- $OBJ/bin/run_vr_shell_unittests

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:292:
audio_capture_indicator_->set_visible(enabled);
As of now, theses will show up in WebVR.  You could:
- Make an indicator class that takes "enabled" and "in_web_vr" to set its own
visibility, or
- Do some specific logic like the HTTP warnings do (ie. logic lives in the scene
manager)

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.h (right):

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.h:38: // These methods are
currently stubbed.
See previous comment.  At least, this comment needs to come out now.

[amp, 2017-05-16 22:24:44]

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:292:
audio_capture_indicator_->set_visible(enabled);
On 2017/05/16 21:31:38, cjgrant wrote:
> As of now, theses will show up in WebVR.  You could:
> - Make an indicator class that takes "enabled" and "in_web_vr" to set its own
> visibility, or
> - Do some specific logic like the HTTP warnings do (ie. logic lives in the
scene
> manager)

Is there a possibility that they need to show up in WebVR?

Who makes that call, security?

[asimjour1, 2017-05-17 14:42:20]

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_interface.h (right):

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.h:35: virtual void
SetVideoCapturingIndicator(bool enabled) = 0;
On 2017/05/16 21:31:38, cjgrant wrote:
> These need to be added to the scene manager unit test mock as well.  To
verify:
> 
> - build vr_shell_unittests
> - $OBJ/bin/run_vr_shell_unittests
> 

Done.

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:292:
audio_capture_indicator_->set_visible(enabled);
On 2017/05/16 22:24:44, amp wrote:
> On 2017/05/16 21:31:38, cjgrant wrote:
> > As of now, theses will show up in WebVR.  You could:
> > - Make an indicator class that takes "enabled" and "in_web_vr" to set its
own
> > visibility, or
> > - Do some specific logic like the HTTP warnings do (ie. logic lives in the
> scene
> > manager)
> 
> Is there a possibility that they need to show up in WebVR?
> 
> Who makes that call, security?

This looks to be out of scope of this CL. I'll start a bug to track this issue.

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.h (right):

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.h:38: // These methods are
currently stubbed.
On 2017/05/16 21:31:38, cjgrant wrote:
> See previous comment.  At least, this comment needs to come out now.

Done.

[cjgrant, 2017-05-17 14:57:31]

lgtm

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2890563003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:292:
audio_capture_indicator_->set_visible(enabled);
On 2017/05/17 14:42:20, asimjour1 wrote:
> On 2017/05/16 22:24:44, amp wrote:
> > On 2017/05/16 21:31:38, cjgrant wrote:
> > > As of now, theses will show up in WebVR.  You could:
> > > - Make an indicator class that takes "enabled" and "in_web_vr" to set its
> own
> > > visibility, or
> > > - Do some specific logic like the HTTP warnings do (ie. logic lives in the
> > scene
> > > manager)
> > 
> > Is there a possibility that they need to show up in WebVR?
> > 
> > Who makes that call, security?
> 
> This looks to be out of scope of this CL. I'll start a bug to track this
issue.

If they do show up in WebVR, presumably they have to be headlocked as well.

[asimjour1, 2017-05-17 17:19:22]

The CQ bit was checked by asimjour@chromium.org

[commit-bot: I haz the power, 2017-05-17 17:20:48]

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-17 17:26:50]

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

[commit-bot: I haz the power, 2017-05-17 17:26:52]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[asimjour1, 2017-05-17 21:24:30]

The CQ bit was checked by asimjour@chromium.org

[asimjour1, 2017-05-17 21:24:31]

The patchset sent to the CQ was uploaded after l-g-t-m from cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2890563003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-17 21:25:35]

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-17 22:38:03]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495056270662980,
"parent_rev": "71c48e725ea0e290239499512213f5096bdb92a1", "commit_rev":
"60119e7d384db63360b0e6f4f50edb7dcebf57e5"}

[commit-bot: I haz the power, 2017-05-17 22:38:15]

Description was changed from

==========
VR: Update UiSceneManager with audio/video capturing flags

UiSceneManager is updated to receive the capturing flags and show
the indicators accordingly.

BUG=722836
==========

to

==========
VR: Update UiSceneManager with audio/video capturing flags

UiSceneManager is updated to receive the capturing flags and show
the indicators accordingly.

BUG=722836

Review-Url: https://codereview.chromium.org/2890563003
Cr-Commit-Position: refs/heads/master@{#472592}
Committed:
https://chromium.googlesource.com/chromium/src/+/60119e7d384db63360b0e6f4f50e...
==========

[commit-bot: I haz the power, 2017-05-17 22:38:16]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/60119e7d384db63360b0e6f4f50e...