Ash Window Cycle UI - add initial show delay and fade in.

ash/common/wm/window_cycle_list.cc

Index: ash/common/wm/window_cycle_list.cc
diff --git a/ash/common/wm/window_cycle_list.cc b/ash/common/wm/window_cycle_list.cc
index a243819a2195d25acd3ff4a2603db5367733dd12..be73518d2cc54a9f5052d898feb1739f206644a9 100644
--- a/ash/common/wm/window_cycle_list.cc
+++ b/ash/common/wm/window_cycle_list.cc
@@ -17,6 +17,7 @@
 #include "ash/common/wm_window.h"
 #include "base/command_line.h"
 #include "ui/accessibility/ax_view_state.h"
+#include "ui/compositor/scoped_layer_animation_settings.h"
 #include "ui/views/background.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/label.h"
@@ -230,6 +231,13 @@ class WindowCycleView : public views::WidgetDelegateView {
     DCHECK(!windows.empty());
     SetPaintToLayer(true);
     layer()->SetFillsBoundsOpaquely(false);
+    layer()->SetOpacity(0.0);
+    {
+      ui::ScopedLayerAnimationSettings animate_fade(layer()->GetAnimator());
+      animate_fade.SetTransitionDuration(
+          base::TimeDelta::FromMilliseconds(100));

[sky, 2016/08/11 19:36:27]

Please document why this uses a shorter time than is typical.

[Evan Stade, 2016/08/11 20:00:11]

I don't know if there's really a reason beyond "this is what Sebastien thought
felt good". I guess you could go by the old axiom that any action should take at
most 250ms to happen, and with the 150ms initial delay this transition only has
100ms left to work with.

What kind of comment would you like to see for this admittedly arbitrary value?

[sky, 2016/08/11 20:04:44]

Fair enough.

+      layer()->SetOpacity(1.0);
+    }
 
     set_background(views::Background::CreateSolidBackground(
         SkColorSetA(SK_ColorBLACK, 0xCC)));
@@ -273,7 +281,6 @@ class WindowCycleView : public views::WidgetDelegateView {
 
     AddChildView(highlight_view_);
     AddChildView(mirror_container_);
-    SetTargetWindow(windows.front());
   }
 
   ~WindowCycleView() override {}
@@ -301,8 +308,7 @@ class WindowCycleView : public views::WidgetDelegateView {
   }
 
   void Layout() override {
-    // Possible if the last window is deleted.
-    if (!target_window_)
+    if (!target_window_ || bounds().IsEmpty())
       return;
 
     // The preview list (|mirror_container_|) starts flush to the left of
@@ -402,33 +408,8 @@ WindowCycleList::WindowCycleList(const WindowList& windows)
     window->AddObserver(this);
 
   if (ShouldShowUi()) {
-    cycle_view_ = new WindowCycleView(windows_);
-
-    WmWindow* root_window = WmShell::Get()->GetRootWindowForNewWindows();
-    views::Widget* widget = new views::Widget;
-    views::Widget::InitParams params;
-    params.delegate = cycle_view_;
-    params.type = views::Widget::InitParams::TYPE_WINDOW_FRAMELESS;
-    params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
-    params.opacity = views::Widget::InitParams::TRANSLUCENT_WINDOW;
-    params.accept_events = true;
-    // TODO(estade): make sure nothing untoward happens when the lock screen
-    // or a system modal dialog is shown.
-    root_window->GetRootWindowController()
-        ->ConfigureWidgetInitParamsForContainer(
-            widget, kShellWindowId_OverlayContainer, &params);
-    widget->Init(params);
-
-    // TODO(estade): right now this just extends past the edge of the screen if
-    // there are too many windows. Handle this more gracefully. Also, if
-    // the display metrics change, cancel the UI.
-    gfx::Rect widget_rect = widget->GetWorkAreaBoundsInScreen();
-    int widget_height = cycle_view_->GetPreferredSize().height();
-    widget_rect.set_y((widget_rect.height() - widget_height) / 2);
-    widget_rect.set_height(widget_height);
-    widget->SetBounds(widget_rect);
-    widget->Show();
-    cycle_ui_widget_.reset(widget);
+    show_ui_timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(150),

[sky, 2016/08/11 19:36:27]

Document why the delay.

[Evan Stade, 2016/08/11 20:00:11]

It is documented in the header file above the member declaration, is that
comment insufficient or do you think it should be moved here?

[sky, 2016/08/11 20:04:44]

What you have is fine. I clearly didn't look enough.

+                         this, &WindowCycleList::InitWindowCycleView);
   }
 }
 
@@ -437,12 +418,12 @@ WindowCycleList::~WindowCycleList() {
   for (WmWindow* window : windows_)
     window->RemoveObserver(this);
 
-  if (showing_window_)
+  if (showing_window_) {
     showing_window_->CancelRestore();
-
-  if (cycle_view_ && cycle_view_->target_window()) {
-    cycle_view_->target_window()->Show();
-    cycle_view_->target_window()->GetWindowState()->Activate();
+  } else if (!windows_.empty()) {
+    WmWindow* target_window = windows_[current_index_];
+    target_window->Show();
+    target_window->GetWindowState()->Activate();
   }
 }
 
@@ -468,14 +449,17 @@ void WindowCycleList::Step(WindowCycleController::Direction direction) {
   current_index_ = (current_index_ + windows_.size()) % windows_.size();
   DCHECK(windows_[current_index_]);
 
-  if (cycle_view_) {
-    cycle_view_->SetTargetWindow(windows_[current_index_]);
-    return;
+  if (ShouldShowUi()) {
+    if (current_index_ > 1)
+      InitWindowCycleView();
+
+    if (cycle_view_)
+      cycle_view_->SetTargetWindow(windows_[current_index_]);
+  } else {
+    // Make sure the next window is visible.
+    showing_window_.reset(new ScopedShowWindow);
+    showing_window_->Show(windows_[current_index_]);
   }
-
-  // Make sure the next window is visible.
-  showing_window_.reset(new ScopedShowWindow);
-  showing_window_->Show(windows_[current_index_]);
 }
 
 void WindowCycleList::OnWindowDestroying(WmWindow* window) {
@@ -509,4 +493,37 @@ bool WindowCycleList::ShouldShowUi() {
              switches::kAshEnableWindowCycleUi);
 }
 
+void WindowCycleList::InitWindowCycleView() {
+  if (cycle_view_)
+    return;
+
+  cycle_view_ = new WindowCycleView(windows_);
+  cycle_view_->SetTargetWindow(windows_[current_index_]);
+
+  WmWindow* root_window = WmShell::Get()->GetRootWindowForNewWindows();
+  views::Widget* widget = new views::Widget;
+  views::Widget::InitParams params;
+  params.delegate = cycle_view_;

[sky, 2016/08/11 19:36:26]

It's very common in ash code to set a name on these widgets to help identify
them later on. I'm suggesting you do something like:

params.name = "WindowCycleList";

This way if you dump the window tree you'll see WindowCycleList.

[Evan Stade, 2016/08/11 20:00:10]

Done.

+  params.type = views::Widget::InitParams::TYPE_WINDOW_FRAMELESS;
+  params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  params.opacity = views::Widget::InitParams::TRANSLUCENT_WINDOW;
+  params.accept_events = true;
+  // TODO(estade): make sure nothing untoward happens when the lock screen
+  // or a system modal dialog is shown.
+  root_window->GetRootWindowController()->ConfigureWidgetInitParamsForContainer(
+      widget, kShellWindowId_OverlayContainer, &params);
+  widget->Init(params);
+
+  // TODO(estade): right now this just extends past the edge of the screen if
+  // there are too many windows. Handle this more gracefully. Also, if
+  // the display metrics change, cancel the UI.
+  gfx::Rect widget_rect = widget->GetWorkAreaBoundsInScreen();
+  int widget_height = cycle_view_->GetPreferredSize().height();
+  widget_rect.set_y((widget_rect.height() - widget_height) / 2);
+  widget_rect.set_height(widget_height);
+  widget->SetBounds(widget_rect);
+  widget->Show();
+  cycle_ui_widget_.reset(widget);
+}
+
 }  // namespace ash