VR: Update UiSceneManager with screen capturing flag

chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/vr_shell/ui_scene_manager.h"
 
 #include "base/macros.h"
 #include "base/test/scoped_task_environment.h"
 #include "chrome/browser/android/vr_shell/ui_elements/ui_element.h"
 #include "chrome/browser/android/vr_shell/ui_elements/ui_element_debug_id.h"
@@ skipping 182 matching lines @@
     bool should_be_visible = visible_in_browsing.find(element->debug_id()) !=
                              visible_in_browsing.end();
     EXPECT_EQ(should_be_visible, element->visible());
   }
   {
     SCOPED_TRACE("Exited Fullsceen");
     EXPECT_TRUE(ColorEquals(initial_background, scene_->GetBackgroundColor()));
   }
 }
 
+TEST_F(UiSceneManagerTest, UiUpdatesForWebVRChanges) {

[cjgrant, 2017/05/25 23:02:56]

I'm not sure it makes sense to include browser-mode here as well.  Ie, I could
see two tests, one that starts in browser and changes to WebVR, and one that
starts in WebVR, but I don't think they should check *what* is visible in
browser mode, only that nothing is visible in WebVR.

The fullscreen test above already covers regular mode, and this test is
therefore duplicating work (and the list of elements).  I still recommend the
original suggestion of entering WebVR and making sure nothing is visible, that's
it.  Adam, thoughts?

[amp, 2017/05/25 23:18:05]

I was thinking about this as well.  We could split this into one large test that
verifies visibility in all the modes, or split it into smaller tests that verify
specific things.

I think either way could work.  The way the tests are run I think it would be
clear which element fails (as the tests don't fail at the first incorrect
assertion, but check them all).  Otherwise I would say we should have smaller
tests so we can see what fails easier.

For WebVr specifically I would like to see two variations: one that starts in
WebVR and makes sure nothing is visible and then transitions to normal and make
sure they show up, and a second one that starts in normal mode, verifies things
are showing up, and then transitions to WebVR and verifies they are gone (and
maybe back again).

I suppose it might get a little verbose though with all the different
variations.

As long as everyone feels good about the test coverage in general we probably
don't need to make sure every possible transition is covered.  (for instance, we
can probably get by without a test transitioning from fullscreen to WebVR, but
then again maybe those are exactly the kind of scenario's most likely to go
wrong). 

[asimjour1, 2017/05/25 23:33:32]

Done.

+  std::set<UiElementDebugId> visible_in_browsing = {
+      UiElementDebugId::kContentQuad,
+      UiElementDebugId::kBackplane,
+      UiElementDebugId::kCeiling,
+      UiElementDebugId::kFloor,
+      UiElementDebugId::kFloorGrid,
+      UiElementDebugId::kUrlBar,
+      UiElementDebugId::kLoadingIndicator,
+      UiElementDebugId::kAudioCaptureIndicator,
+      UiElementDebugId::kVideoCaptureIndicator,
+      UiElementDebugId::kScreenCaptureIndicator};
+
+  MakeManager(kNotInCct, kNotInWebVr);
+  manager_->SetAudioCapturingIndicator(true);
+  manager_->SetVideoCapturingIndicator(true);
+  manager_->SetScreenCapturingIndicator(true);
+
+  // Everything should be visible by default.

[cjgrant, 2017/05/25 23:02:56]

As above, can skip.

[asimjour1, 2017/05/25 23:33:32]

Done.

+  for (const auto& element : scene_->GetUiElements()) {
+    SCOPED_TRACE(element->debug_id());
+    bool should_be_visible = visible_in_browsing.find(element->debug_id()) !=
+                             visible_in_browsing.end();
+    EXPECT_EQ(should_be_visible, element->visible());
+  }
+
+  // Transistion to web vr mode.
+  manager_->SetWebVrMode(true);
+  manager_->SetAudioCapturingIndicator(true);

[cjgrant, 2017/05/25 23:02:56]

No need to re-set these flags.

[asimjour1, 2017/05/25 23:33:32]

Done.

+  manager_->SetVideoCapturingIndicator(true);
+  manager_->SetScreenCapturingIndicator(true);
+
+  // All elements should be hidden.
+  for (const auto& element : scene_->GetUiElements()) {
+    SCOPED_TRACE(element->debug_id());

[cjgrant, 2017/05/25 23:02:56]

All elements should be invisible, so there's no logic needed here other than
"EXPECT_FALSE(element->visible())

[amp, 2017/05/25 23:18:05]

+1

[asimjour1, 2017/05/25 23:33:32]

Done.

+    bool should_be_hidden = visible_in_browsing.find(element->debug_id()) !=
+                            visible_in_browsing.end();
+    if (should_be_hidden)
+      EXPECT_FALSE(element->visible());
+  }
+
+  // Exit webvr mode.

[cjgrant, 2017/05/25 23:02:56]

And, no need for this block.

[amp, 2017/05/25 23:18:05]

This one I would argue it's nice to verify the return transition and make sure
things show up again.

[asimjour1, 2017/05/25 23:33:32]

Done.

[cjgrant, 2017/05/26 00:58:02]

Adam, could go either way, but with unit tests, it feels cleaner to me to focus
on a single mode at a time, with variants for the ways into a particular mode
(this could be parameterized if we were ambitious).  That was my motivation
anyway.

The browser mode test itself could be one big test that activates warnings, or
many small tests...

+  manager_->SetWebVrMode(false);
+  manager_->SetAudioCapturingIndicator(true);
+  manager_->SetVideoCapturingIndicator(true);
+  manager_->SetScreenCapturingIndicator(true);
+
+  // Everything should return to original state after leaving webvr.
+  for (const auto& element : scene_->GetUiElements()) {
+    SCOPED_TRACE(element->debug_id());
+    bool should_be_visible = visible_in_browsing.find(element->debug_id()) !=
+                             visible_in_browsing.end();
+    EXPECT_EQ(should_be_visible, element->visible());
+  }
+}
 }  // namespace vr_shell