tablet: Make sure screenshot is deleted for popups type window.

See linked bug for why the types of windows are undesirable and should
also be fixed at the callsites.

But in the meantime, they can exist so we need to protect. For these
windows, they are part of the WindowCycleList. Since we group up
transient related windows in overview/alt+tab, those two surfaces are
seemingly unaffected. For tablet mode transition, a screenshot is
first created and then deleted either if no animation is run, or at
the end of TRANSFORM animation.

Since these windows cannot be maximized, they are actually given a
BOUNDS animation. The check to see if a window is animating should be
specifically check for TRANSFORM animation.

Test: added regression test
Fixed: 1096128
Change-Id: Ide585fae26f6ed7a96d3a181b399385f2fa461f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2250459
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779547}

ash/wm/tablet_mode/tablet_mode_controller.cc

@@ -39,6 +39,7 @@
 #include "components/viz/common/frame_sinks/copy_output_request.h"
 #include "components/viz/common/frame_sinks/copy_output_result.h"
 #include "third_party/khronos/GLES2/gl2.h"
+#include "ui/aura/client/aura_constants.h"
 #include "ui/aura/window_observer.h"
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/compositor/layer_animation_sequence.h"
@@ -153,9 +154,12 @@ bool HasActiveInternalDisplay() {
              display::Display::InternalDisplayId());
 }
 
-bool IsTransformAnimationSequence(ui::LayerAnimationSequence* sequence) {
+// Returns true if |sequence| has the same properties as the ones we care about
+// for the tablet transition animation.
+bool ShouldObserveSequence(ui::LayerAnimationSequence* sequence) {
   DCHECK(sequence);
-  return sequence->properties() & ui::LayerAnimationElement::TRANSFORM;
+  return sequence->properties() &
+         TabletModeController::GetObservedTabletTransitionProperty();
 }
 
 std::unique_ptr<ui::Layer> CreateLayerFromScreenshotResult(
@@ -390,6 +394,12 @@ void TabletModeController::SetUseScreenshotForTest(bool use_screenshot) {
   use_screenshot_for_test = use_screenshot;
 }
 
+// static
+ui::LayerAnimationElement::AnimatableProperty
+TabletModeController::GetObservedTabletTransitionProperty() {
+  return ui::LayerAnimationElement::TRANSFORM;
+}
+
 TabletModeController::TabletModeController()
     : event_blocker_(std::make_unique<InternalInputDevicesEventBlocker>()),
       tablet_mode_usage_interval_start_time_(base::Time::Now()),
@@ -747,7 +757,7 @@ void TabletModeController::OnLayerAnimationStarted(
 
 void TabletModeController::OnLayerAnimationAborted(
     ui::LayerAnimationSequence* sequence) {
-  if (!fps_counter_ || !IsTransformAnimationSequence(sequence))
+  if (!fps_counter_ || !ShouldObserveSequence(sequence))
     return;
 
   StopObservingAnimation(/*record_stats=*/false, /*delete_screenshot=*/true);
@@ -760,7 +770,7 @@ void TabletModeController::OnLayerAnimationEnded(
   // stats/screenshot in those cases.
   // TODO(sammiequon): We may want to remove the |fps_counter_| check and
   // simplify things since those are edge cases.
-  if (!fps_counter_ || !IsTransformAnimationSequence(sequence))
+  if (!fps_counter_ || !ShouldObserveSequence(sequence))
     return;
 
   StopObservingAnimation(/*record_stats=*/true, /*delete_screenshot=*/true);
@@ -768,7 +778,7 @@ void TabletModeController::OnLayerAnimationEnded(
 
 void TabletModeController::OnLayerAnimationScheduled(
     ui::LayerAnimationSequence* sequence) {
-  if (!IsTransformAnimationSequence(sequence))
+  if (!ShouldObserveSequence(sequence))
     return;
 
   if (!fps_counter_) {

ash/wm/tablet_mode/tablet_mode_controller.h

@@ -24,6 +24,7 @@
 #include "base/timer/timer.h"
 #include "chromeos/dbus/power/power_manager_client.h"
 #include "ui/aura/window_occlusion_tracker.h"
+#include "ui/compositor/layer_animation_element.h"
 #include "ui/compositor/layer_animation_observer.h"
 #include "ui/compositor/layer_observer.h"
 #include "ui/compositor/layer_tree_owner.h"
@@ -75,10 +76,6 @@ class ASH_EXPORT TabletModeController
       public ui::LayerAnimationObserver,
       public ui::LayerObserver {
  public:
-  // Enable or disable using a screenshot for testing as it makes the
-  // initialization flow async, which makes most tests harder to write.
-  static void SetUseScreenshotForTest(bool use_screenshot);
-
   // Used for keeping track if the user wants the machine to behave as a
   // clamshell/tablet regardless of hardware orientation.
   // TODO(oshima): Move this to common place.
@@ -91,6 +88,15 @@ class ASH_EXPORT TabletModeController
   // Public so it can be used by unit tests.
   constexpr static char kLidAngleHistogramName[] = "Ash.TouchView.LidAngle";
 
+  // Enable or disable using a screenshot for testing as it makes the
+  // initialization flow async, which makes most tests harder to write.
+  static void SetUseScreenshotForTest(bool use_screenshot);
+
+  // Returns the the animation property that we observe when transitioning from
+  // clamshell to tablet mode.
+  static ui::LayerAnimationElement::AnimatableProperty
+  GetObservedTabletTransitionProperty();
+
   TabletModeController();
   ~TabletModeController() override;
 

ash/wm/tablet_mode/tablet_mode_controller_unittest.cc

@@ -1737,14 +1737,12 @@ TEST_P(TabletModeControllerScreenshotTest, FromOverviewNoScreenshot) {
   ShellTestApi().WaitForOverviewAnimationState(
       OverviewAnimationState::kEnterAnimationComplete);
 
-  // Enter tablet mode. This triggers an overview exit, so |window2| will be
-  // animating from the overview exit animation. Therefore, we do not show the
-  // screenshot.
+  // Enter tablet mode while in overview. There should be no screenshot at any
+  // time.
   TabletMode::Waiter waiter(/*enable=*/true);
   SetTabletMode(true);
   EXPECT_FALSE(IsScreenshotShown());
   EXPECT_TRUE(IsShelfOpaque());
-  EXPECT_TRUE(window2->layer()->GetAnimator()->is_animating());
 
   waiter.Wait();
   EXPECT_FALSE(IsScreenshotShown());
@@ -1858,6 +1856,27 @@ TEST_P(TabletModeControllerScreenshotTest, NoCrashWhenExitingWithoutWaiting) {
   EXPECT_FALSE(IsShelfOpaque());
 }
 
+// Tests that the screenshot gets deleted after transition with a transient
+// child as the top window that is not resizeable but positionable. Note that
+// creating such windows is not desirable, but is possible so we need this
+// regression test. See https://crbug.com/1096128.
+TEST_P(TabletModeControllerScreenshotTest, TransientChildTypeWindow) {
+  // Create a window with a transient child that is of WINDOW_TYPE_POPUP.
+  auto window = CreateTestWindow(gfx::Rect(200, 200));
+  auto child = CreateTestWindow(gfx::Rect(200, 200));
+  child->SetProperty(aura::client::kResizeBehaviorKey,
+                     aura::client::kResizeBehaviorCanResize);
+  ::wm::AddTransientChild(window.get(), child.get());
+
+  window->layer()->GetAnimator()->StopAnimating();
+  child->layer()->GetAnimator()->StopAnimating();
+
+  SetTabletMode(true);
+  ShellTestApi().WaitForWindowFinishAnimating(child.get());
+  EXPECT_FALSE(IsScreenshotShown());
+  EXPECT_TRUE(IsShelfOpaque());
+}
+
 INSTANTIATE_TEST_SUITE_P(All, TabletModeControllerTest, testing::Bool());
 INSTANTIATE_TEST_SUITE_P(All,
                          TabletModeControllerInitedFromPowerManagerClientTest,

ash/wm/tablet_mode/tablet_mode_window_manager.cc

@@ -23,6 +23,7 @@
 #include "ash/wm/splitview/split_view_controller.h"
 #include "ash/wm/splitview/split_view_utils.h"
 #include "ash/wm/tablet_mode/scoped_skip_user_session_blocked_check.h"
+#include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "ash/wm/tablet_mode/tablet_mode_toggle_fullscreen_event_handler.h"
 #include "ash/wm/tablet_mode/tablet_mode_window_state.h"
 #include "ash/wm/window_state.h"
@@ -34,6 +35,7 @@
 #include "base/command_line.h"
 #include "base/stl_util.h"
 #include "ui/aura/client/aura_constants.h"
+#include "ui/compositor/layer_animation_element.h"
 #include "ui/display/screen.h"
 
 namespace ash {
@@ -121,10 +123,14 @@ class ScopedObserveWindowAnimation {
     if (!window_)
       return;
 
-    // Stops observing if |window_| is not animating, or if it is not tracked by
-    // TabletModeWindowManager. When this object is destroyed while exiting
-    // tablet mode, |window_| is no longer tracked, so skip that check.
-    if (window_->layer()->GetAnimator()->is_animating() &&
+    const bool is_animating =
+        window_->layer()->GetAnimator()->IsAnimatingProperty(
+            TabletModeController::GetObservedTabletTransitionProperty());
+    // Stops observing if |window_| is not animating the property we care about,
+    // or if it is not tracked by TabletModeWindowManager. When this object is
+    // destroyed while exiting tablet mode, |window_| is no longer tracked, so
+    // skip that check.
+    if (is_animating &&
         (exiting_tablet_mode_ || manager_->IsTrackingWindow(window_))) {
       return;
     }