VR: Disable system indicators in full-screen mode.

VR: Disable system indicators in full-screen mode.

BUG=733010
Review-Url: https://codereview.chromium.org/2953073003
Cr-Commit-Position: refs/heads/master@{#482046}
Committed: https://chromium.googlesource.com/chromium/src/+/d45ae95cca9ad16c5e18114565c323e3e094b557

[cjgrant, 2017-06-22 21:45:06]

cjgrant@chromium.org changed reviewers:
+ amp@chromium.org, asimjour@chromium.org

[cjgrant, 2017-06-22 21:45:06]

Folks, please take a look when you can.

[asimjour1, 2017-06-23 13:28:41]

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

https://codereview.chromium.org/2953073003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:395:
manager_->SetFullscreen(true);
We miss the transition from Fullscreen to WebVr.

[amp, 2017-06-23 18:36:59]

lgtm, a comment and question below, but not major.

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

https://codereview.chromium.org/2953073003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:501: ConfigureIndicators();
Add a comment about why the indicators need to be configure here (instead of
with the reset of the ConfigureXXX calls).

Alternatively, should we move the ConfigureXXX calls into ConfigureScene?

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

https://codereview.chromium.org/2953073003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:383:
manager_->SetAudioCapturingIndicator(true);
Does anything different happen if some of the indicators are enabled and some
are disabled?

[cjgrant, 2017-06-23 19:38:26]

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

https://codereview.chromium.org/2953073003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:501: ConfigureIndicators();
On 2017/06/23 18:36:59, amp wrote:
> Add a comment about why the indicators need to be configure here (instead of
> with the reset of the ConfigureXXX calls).
> 
> Alternatively, should we move the ConfigureXXX calls into ConfigureScene?

I'll add the comment.  At a high-level, I see ConfigureScene as a dumping ground
to configure everything.  We have these other smaller helper methods to more
efficiently update certain things when bits of information change, but it should
be fair game to call all of those in here too.  The reason we can't is that the
original webvr warnings will get messed up - their timers will reset every time
the scene changes, IIRC.  When we circle back and refactor the transience stuff,
this should be cleaner.

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

https://codereview.chromium.org/2953073003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:383:
manager_->SetAudioCapturingIndicator(true);
On 2017/06/23 18:36:59, amp wrote:
> Does anything different happen if some of the indicators are enabled and some
> are disabled?

Nope, but, Amir and I talked about additional testing to make sure that enabling
one at a time shows one at a time.  Basically to make sure the plumbing from API
to individual indicator is correct, but that seems slightly too trivial to be
valuable.  A la "1 + 2 = 3.  Unit test that 1 + 2 really is 3".

https://codereview.chromium.org/2953073003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:395:
manager_->SetFullscreen(true);
On 2017/06/23 13:28:41, asimjour1 wrote:
> We miss the transition from Fullscreen to WebVr.

Discussed offline.  The intent isn't really to cover all transitions, as much as
transitions that should cause shows/hides.

[cjgrant, 2017-06-23 21:17:14]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-06-23 21:17:15]

The patchset sent to the CQ was uploaded after l-g-t-m from amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2953073003/#ps20001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-06-23 21:17:38]

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-23 21:17:39]

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

[commit-bot: I haz the power, 2017-06-23 21:17:40]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[bshe, 2017-06-23 21:25:05]

On 2017/06/23 21:17:40, commit-bot: I haz the power wrote:
> No L-G-T-M from a valid reviewer yet.
> CQ run can only be started once the patch has received an L-G-T-M from a full
> committer.
> Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
> full super star committer.
> Committers are members of the group "project-chromium-committers".
> Note that this has nothing to do with OWNERS files.

rubber stamp lgtm

[cjgrant, 2017-06-23 21:26:10]

The CQ bit was checked by cjgrant@chromium.org

[commit-bot: I haz the power, 2017-06-23 21:26:38]

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-23 22:15:19]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1498253170034470,
"parent_rev": "198b9e6b983a55bcce7bc299a4fde8c8611ef6f8", "commit_rev":
"d45ae95cca9ad16c5e18114565c323e3e094b557"}

[commit-bot: I haz the power, 2017-06-23 22:15:34]

Description was changed from

==========
VR: Disable system indicators in full-screen mode.

BUG=733010
==========

to

==========
VR: Disable system indicators in full-screen mode.

BUG=733010

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

[commit-bot: I haz the power, 2017-06-23 22:15:35]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d45ae95cca9ad16c5e18114565c3...