[vr] Close exit prompt when clicking on background

chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc

Index: chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc
diff --git a/chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc b/chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc
index 81c6c736ecf674c70074ad54919d761fb42af3ee..d518e1c10df1ea89b52c1f9c51edf04bd19e0ac0 100644
--- a/chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc
+++ b/chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc
@@ -61,6 +61,8 @@ class MockBrowserInterface : public VrBrowserInterface {
 
 std::set<UiElementDebugId> kElementsVisibleInBrowsing = {
     kContentQuad, kBackplane, kCeiling, kFloor, kUrlBar, kLoadingIndicator};
+std::set<UiElementDebugId> kElementsVisibleWithExitPrompt = {

[cjgrant, 2017/06/06 21:21:56]

If this is only used in one test, it could live there.  Your call.

[ymalik, 2017/06/07 14:55:59]

Its used by two tests. The primary button click and and the backplane.

+    kExitPrompt, kExitPromptBackplane, kCeiling, kFloor};
 
 }  // namespace
 
@@ -90,6 +92,15 @@ class UiSceneManagerTest : public testing::Test {
     return element ? element->visible() : false;
   }
 
+  void VerifyElementsVisible(std::set<UiElementDebugId> elements) {

[cjgrant, 2017/06/06 21:21:56]

const&

[ymalik, 2017/06/07 14:55:58]

Done.

+    for (const auto& element : scene_->GetUiElements()) {
+      SCOPED_TRACE(element->debug_id());

[cjgrant, 2017/06/06 21:21:56]

If an element is wrong, does the trace still identify the right point of failure
(ie. the line on which this method is called)?  Do we need nested SCOPED_TRACE()
calls?  Just curious.

[amp, 2017/06/06 21:42:43]

Yes it fails on a specific line number, but then this scoped_trace will also
output with that failure.

It is immensely helpful in debugging loops (to me at least).

The line number is given, but it also gives you the id of the last item
processed (which you have to look up in the enum, but that's not too hard).

It will tell you exactly what element isn't doing what you expected.

[amp, 2017/06/06 21:46:22]

Oh wait, realized the question was if this would show the call site line number.
 No, it won't.

We need to add a SCOPED_TRACE('[test name]') before each call of '
VerifyElementsVisible' and then when something fails here it will tell us which
test failed.

We should also keep this one in the loop as well (you can have multiple in a
test).

[ymalik, 2017/06/07 14:55:58]

Ah good catch.

I don't think SCOPED_TRACE('[test name]') adds much value since we'll already
know which test failed. I think passing in a debug string is better because it
saves us an extra line before the call.

[cjgrant, 2017/06/07 16:03:06]

I played with this a bit, and I think what you have is optimal.  Docs seem to
suggest adding SCOPED_TRACE calls before every helper function call, but that's
cumbersome too.  This seems better.

[amp, 2017/06/07 17:18:41]

Cumbersome, but it's not really useful if you don't call it before the helper
function call (it won't be able to tell which test it was that failed).  See my
comment on latest patch set.

+      bool should_be_visible =
+          elements.find(element->debug_id()) != elements.end();
+      EXPECT_EQ(should_be_visible, element->visible());
+    }
+  }
+
   base::test::ScopedTaskEnvironment scoped_task_environment_;
   std::unique_ptr<MockBrowserInterface> browser_;
   std::unique_ptr<UiScene> scene_;
@@ -212,43 +223,21 @@ TEST_F(UiSceneManagerTest, UiUpdatesForFullscreenChanges) {
 
   // Hold onto the background color to make sure it changes.
   SkColor initial_background = scene_->GetBackgroundColor();
+  VerifyElementsVisible(kElementsVisibleInBrowsing);
 
-  for (const auto& element : scene_->GetUiElements()) {
-    SCOPED_TRACE(element->debug_id());
-    bool should_be_visible =
-        kElementsVisibleInBrowsing.find(element->debug_id()) !=
-        kElementsVisibleInBrowsing.end();
-    EXPECT_EQ(should_be_visible, element->visible());
-  }
-
-  // Transistion to fullscreen.
+  // In fullscreen mode, content elements should be visible, control elements

[amp, 2017/06/06 21:42:43]

My latest change (about to go in) changes this again, just an FYI.

[ymalik, 2017/06/07 14:55:59]

Acknowledged.

+  // should be hidden.
   manager_->SetFullscreen(true);
-
-  // Content elements should be visible, control elements should be hidden.
-  for (const auto& element : scene_->GetUiElements()) {
-    SCOPED_TRACE(element->debug_id());
-    bool should_be_visible = visible_in_fullscreen.find(element->debug_id()) !=
-                             visible_in_fullscreen.end();
-    EXPECT_EQ(should_be_visible, element->visible());
-  }
-
+  VerifyElementsVisible(visible_in_fullscreen);
   {
     SCOPED_TRACE("Entered Fullsceen");
     // Make sure background has changed for fullscreen.
     EXPECT_NE(initial_background, scene_->GetBackgroundColor());
   }
 
-  // Exit fullscreen.
-  manager_->SetFullscreen(false);
-
   // Everything should return to original state after leaving fullscreen.
-  for (const auto& element : scene_->GetUiElements()) {
-    SCOPED_TRACE(element->debug_id());
-    bool should_be_visible =
-        kElementsVisibleInBrowsing.find(element->debug_id()) !=
-        kElementsVisibleInBrowsing.end();
-    EXPECT_EQ(should_be_visible, element->visible());
-  }
+  manager_->SetFullscreen(false);
+  VerifyElementsVisible(kElementsVisibleInBrowsing);
   {
     SCOPED_TRACE("Exited Fullsceen");
     EXPECT_EQ(initial_background, scene_->GetBackgroundColor());
@@ -256,39 +245,37 @@ TEST_F(UiSceneManagerTest, UiUpdatesForFullscreenChanges) {
 }
 
 TEST_F(UiSceneManagerTest, UiUpdatesExitPrompt) {
-  std::set<UiElementDebugId> visible_when_prompting = {kExitPrompt, kBackplane,
-                                                       kCeiling, kFloor};
   MakeManager(kNotInCct, kNotInWebVr);
 
   manager_->SetWebVrSecureOrigin(true);
 
   // Initial state.
-  for (const auto& element : scene_->GetUiElements()) {
-    SCOPED_TRACE(element->debug_id());
-    bool should_be_visible =
-        kElementsVisibleInBrowsing.find(element->debug_id()) !=
-        kElementsVisibleInBrowsing.end();
-    EXPECT_EQ(should_be_visible, element->visible());
-  }
+  VerifyElementsVisible(kElementsVisibleInBrowsing);
 
   // Exit prompt visible state.
   manager_->OnSecurityIconClicked();
-  for (const auto& element : scene_->GetUiElements()) {
-    SCOPED_TRACE(element->debug_id());
-    bool should_be_visible = visible_when_prompting.find(element->debug_id()) !=
-                             visible_when_prompting.end();
-    EXPECT_EQ(should_be_visible, element->visible());
-  }
+  VerifyElementsVisible(kElementsVisibleWithExitPrompt);
 
   // Back to initial state.
   manager_->OnExitPromptPrimaryButtonClicked();
-  for (const auto& element : scene_->GetUiElements()) {
-    SCOPED_TRACE(element->debug_id());
-    bool should_be_visible =
-        kElementsVisibleInBrowsing.find(element->debug_id()) !=
-        kElementsVisibleInBrowsing.end();
-    EXPECT_EQ(should_be_visible, element->visible());
-  }
+  VerifyElementsVisible(kElementsVisibleInBrowsing);
+}
+
+TEST_F(UiSceneManagerTest, BackplaneClickClosesExitPrompt) {
+  MakeManager(kNotInCct, kNotInWebVr);
+
+  manager_->SetWebVrSecureOrigin(true);
+
+  // Initial state.
+  VerifyElementsVisible(kElementsVisibleInBrowsing);
+
+  // Exit prompt visible state.
+  manager_->OnSecurityIconClicked();
+  VerifyElementsVisible(kElementsVisibleWithExitPrompt);
+
+  // Back to initial state.
+  manager_->OnExitPromptBackplaneClicked();
+  VerifyElementsVisible(kElementsVisibleInBrowsing);
 }
 
 TEST_F(UiSceneManagerTest, UiUpdatesForWebVR) {
@@ -300,10 +287,7 @@ TEST_F(UiSceneManagerTest, UiUpdatesForWebVR) {
   manager_->SetScreenCapturingIndicator(true);
 
   // All elements should be hidden.
-  for (const auto& element : scene_->GetUiElements()) {
-    SCOPED_TRACE(element->debug_id());
-    EXPECT_FALSE(element->visible());
-  }
+  VerifyElementsVisible({});
 }
 
 TEST_F(UiSceneManagerTest, UiUpdateTransitionToWebVR) {
@@ -317,10 +301,7 @@ TEST_F(UiSceneManagerTest, UiUpdateTransitionToWebVR) {
   manager_->SetWebVrSecureOrigin(true);
 
   // All elements should be hidden.
-  for (const auto& element : scene_->GetUiElements()) {
-    SCOPED_TRACE(element->debug_id());
-    EXPECT_FALSE(element->visible());
-  }
+  VerifyElementsVisible({});

[cjgrant, 2017/06/06 21:21:56]

Thanks for adjusting all these call sites!  code_size--;

[ymalik, 2017/06/07 14:55:59]

Acknowledged.

 }
 
 TEST_F(UiSceneManagerTest, CaptureIndicatorsVisibility) {