VR: Factor transient timing out of UiSceneManager.

chrome/browser/android/vr_shell/ui_scene_manager.cc

Index: chrome/browser/android/vr_shell/ui_scene_manager.cc
diff --git a/chrome/browser/android/vr_shell/ui_scene_manager.cc b/chrome/browser/android/vr_shell/ui_scene_manager.cc
index 2258091639514f2298254f4f1e5da8238b9565e9..bc7f27999a24c636f202619e89378aaa3542a516 100644
--- a/chrome/browser/android/vr_shell/ui_scene_manager.cc
+++ b/chrome/browser/android/vr_shell/ui_scene_manager.cc
@@ -406,7 +406,8 @@ void UiSceneManager::CreateExitPrompt() {
 }
 
 void UiSceneManager::CreateToasts() {
-  std::unique_ptr<UiElement> element = base::MakeUnique<PresentationToast>(512);
+  auto element = base::MakeUnique<PresentationToast>(
+      512, base::TimeDelta::FromSeconds(kToastTimeoutSeconds));
   element->set_debug_id(kPresentationToast);
   element->set_id(AllocateId());
   element->set_fill(vr_shell::Fill::NONE);
@@ -430,14 +431,20 @@ void UiSceneManager::SetWebVrMode(bool web_vr,
       web_vr_show_toast_ == show_toast) {
     return;
   }
+  // If leaving WebVR, disable the toast. ConfigureScene will re-enable it if
+  // necessary.
+  if (!web_vr)
+    presentation_toast_->SetEnabled(false);

[bshe, 2017/06/27 14:57:09]

Do you need this? Isn't ConfigureScene set enable to correct state?

[cjgrant, 2017/06/28 19:00:55]

See the reworked version, where we need the few lines below.  Let me know if the
comment is not helpful.

+
   web_vr_mode_ = web_vr;
   web_vr_autopresented_ = auto_presented;
   web_vr_show_toast_ = show_toast;
-  toast_state_ = SET_FOR_WEB_VR;
   ConfigureScene();
   ConfigureSecurityWarnings();
   ConfigureTransientUrlBar();
-  ConfigurePresentationToast();
+
+  if (web_vr)

[bshe, 2017/06/27 14:57:09]

Should this be if (web_vr && show_toast)? It still works as we only enable the
toast when the above is true. But it is less readable compare to (web_vr &&
show_toast)

[cjgrant, 2017/06/28 19:00:55]

I think, as it is, this is better.  It's more clear with the IfEnabled() suffix
you suggested.

+    presentation_toast_->transience()->KickVisibility();
 }
 
 void UiSceneManager::ConfigureScene() {
@@ -499,6 +506,9 @@ void UiSceneManager::ConfigureScene() {
                                 -kBackgroundDistanceMultiplier);
   UpdateBackgroundColor();
 
+  presentation_toast_->SetEnabled((fullscreen_ && !web_vr_mode_) ||
+                                  (web_vr_mode_ && web_vr_show_toast_));
+
   // Configure other subsystems here as well. Ultimately, it would be nice if we
   // could configure all elements through ConfigureScene(), as the exact
   // conditions that control each element are getting complicated. More systems
@@ -569,9 +579,8 @@ void UiSceneManager::SetFullscreen(bool fullscreen) {
   if (fullscreen_ == fullscreen)
     return;
   fullscreen_ = fullscreen;
-  toast_state_ = SET_FOR_FULLSCREEN;
   ConfigureScene();
-  ConfigurePresentationToast();
+  presentation_toast_->transience()->KickVisibility();
 }
 
 void UiSceneManager::ConfigureSecurityWarnings() {
@@ -579,6 +588,7 @@ void UiSceneManager::ConfigureSecurityWarnings() {
   permanent_security_warning_->set_visible(enabled);
   transient_security_warning_->set_visible(enabled);
   if (enabled) {
+    security_warning_timer_.Stop();

[bshe, 2017/06/27 14:57:09]

Stop shouldn't be necessary I believe. Start would reset previous timer.

[cjgrant, 2017/06/28 19:00:55]

Done.

     security_warning_timer_.Start(
         FROM_HERE, base::TimeDelta::FromSeconds(kWarningTimeoutSeconds), this,
         &UiSceneManager::OnSecurityWarningTimer);
@@ -607,6 +617,7 @@ void UiSceneManager::ConfigureIndicators() {
       total_width += indicator->size().x();
     }
   }
+
   float x_position = -total_width / 2;
   for (UiElement* indicator : system_indicators_) {
     if (!indicator->visible())
@@ -619,30 +630,8 @@ void UiSceneManager::ConfigureIndicators() {
   }
 }
 
-void UiSceneManager::ConfigurePresentationToast() {
-  bool toast_visible = false;
-  switch (toast_state_) {
-    case SET_FOR_WEB_VR:
-      toast_visible = web_vr_show_toast_;
-      break;
-    case SET_FOR_FULLSCREEN:
-      toast_visible = fullscreen_;
-      break;
-    case UNCHANGED:
-      return;
-  }
-  presentation_toast_->set_visible(toast_visible);
-  if (toast_visible) {
-    presentation_toast_timer_.Start(
-        FROM_HERE, base::TimeDelta::FromSeconds(kToastTimeoutSeconds), this,
-        &UiSceneManager::OnPresentationToastTimer);
-  } else {
-    presentation_toast_timer_.Stop();
-  }
-  toast_state_ = UNCHANGED;
-}
-
 void UiSceneManager::OnSecurityWarningTimer() {
+  // TODO: Also do this one!

[bshe, 2017/06/27 14:57:09]

Add ldap or create a crbug?

[cjgrant, 2017/06/27 15:15:55]

I'm planning to do this before submitting.  I posted the CL to make sure you
approve of the method before changing all the other elements to use it.  There's
a follow-up patch coming.  And sorry, this wasn't as polished as it should have
been.  :)

[cjgrant, 2017/06/28 19:00:55]

Done.

   transient_security_warning_->set_visible(false);
 }
 
@@ -662,10 +651,6 @@ void UiSceneManager::OnTransientUrlBarTimer() {
   transient_url_bar_->set_visible(false);
 }
 
-void UiSceneManager::OnPresentationToastTimer() {
-  presentation_toast_->set_visible(false);
-}
-
 void UiSceneManager::OnBackButtonClicked() {
   browser_->NavigateBack();
 }