aura: DCHECK when occlusion is recomputed too many times.

Before this CL, recomputing occlusion too many times before occlusion
states became stable caused a call to DumpWithoutCrashing(). Now that
we didn't get any crash reports in the last 6 months, this CL replaces
the DumpWithoutCrashing() with a simple DCHECK.

Bug: 813076
Change-Id: I2ec8c5fa89a1646367035c3585df091a9a4f76cc
Reviewed-on: https://chromium-review.googlesource.com/1168024Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581887}

ui/aura/test/window_occlusion_tracker_test_api.cc

@@ -16,14 +16,5 @@ int WindowOcclusionTrackerTestApi::GetNumTimesOcclusionRecomputed() const {
   return WindowOcclusionTracker::GetInstance()->num_times_occlusion_recomputed_;
 }
 
-bool WindowOcclusionTrackerTestApi::WasOcclusionRecomputedTooManyTimes() {
-  const bool local_was_occlusion_recomputed_too_many_times =
-      WindowOcclusionTracker::GetInstance()
-          ->was_occlusion_recomputed_too_many_times_;
-  WindowOcclusionTracker::GetInstance()
-      ->was_occlusion_recomputed_too_many_times_ = false;
-  return local_was_occlusion_recomputed_too_many_times;
-}
-
 }  // namespace test
 }  // namespace aura

ui/aura/test/window_occlusion_tracker_test_api.h

@@ -18,10 +18,6 @@ class WindowOcclusionTrackerTestApi {
   // Returns the number of times that occlusion was recomputed in this process.
   int GetNumTimesOcclusionRecomputed() const;
 
-  // Returns true if WindowOcclusionTracker had to recompute occlusion too many
-  // times before becoming stable since the last call to this.
-  bool WasOcclusionRecomputedTooManyTimes();
-
  private:
   DISALLOW_COPY_AND_ASSIGN(WindowOcclusionTrackerTestApi);
 };

ui/aura/window_occlusion_tracker.cc

@@ -6,7 +6,6 @@
 
 #include "base/auto_reset.h"
 #include "base/containers/adapters.h"
-#include "base/debug/dump_without_crashing.h"
 #include "base/stl_util.h"
 #include "third_party/skia/include/core/SkRect.h"
 #include "third_party/skia/include/core/SkRegion.h"
@@ -356,23 +355,13 @@ void WindowOcclusionTracker::MarkRootWindowAsDirtyAndMaybeComputeOcclusionIf(
 
 void WindowOcclusionTracker::MarkRootWindowAsDirty(
     RootWindowState* root_window_state) {
-  root_window_state->dirty = true;
+  // If a root window is marked as dirty and occlusion states have already been
+  // recomputed |kMaxRecomputeOcclusion| times, it means that they are not
+  // stabilizing.
+  DCHECK_LT(num_times_occlusion_recomputed_in_current_step_,
+            kMaxRecomputeOcclusion);
 
-  // Generate a crash report when a root window is marked as dirty and occlusion
-  // states have been recomputed |kMaxRecomputeOcclusion| times, because it
-  // indicates that they are not stabilizing. Don't report it when
-  // |num_times_occlusion_recomputed_in_current_step_| is greater than
-  // |kMaxRecomputeOcclusion| to avoid generating multiple reports from the same
-  // client.
-  //
-  // TODO(fdoray): Remove this once we are confident that occlusion states are
-  // stable after |kMaxRecomputeOcclusion| iterations in production.
-  // https://crbug.com/813076
-  if (num_times_occlusion_recomputed_in_current_step_ ==
-      kMaxRecomputeOcclusion) {
-    was_occlusion_recomputed_too_many_times_ = true;
-    base::debug::DumpWithoutCrashing();
-  }
+  root_window_state->dirty = true;
 }
 
 bool WindowOcclusionTracker::WindowOrParentIsAnimated(Window* window) const {

ui/aura/window_occlusion_tracker.h

@@ -206,11 +206,6 @@ class AURA_EXPORT WindowOcclusionTracker : public ui::LayerAnimationObserver,
   // recomputed occlusion states. Always 0 when not in MaybeComputeOcclusion().
   int num_times_occlusion_recomputed_in_current_step_ = 0;
 
-  // Set to true when occlusion is recomputed too many times before it becomes
-  // stable. Reset in
-  // WindowOcclusionTrackerTestApi::WasOcclusionRecomputedTooManyTimes().
-  bool was_occlusion_recomputed_too_many_times_ = false;
-
   DISALLOW_COPY_AND_ASSIGN(WindowOcclusionTracker);
 };
 

ui/aura/window_occlusion_tracker_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "base/macros.h"
 #include "base/run_loop.h"
+#include "base/test/gtest_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/aura/env.h"
 #include "ui/aura/test/aura_test_base.h"
@@ -1448,8 +1449,7 @@ class WindowDelegateChangingWindowVisibility : public MockWindowDelegate {
 }  // namespace
 
 // Verify that if a window changes its visibility every time it is notified that
-// its occlusion state changed, the occlusion state of all IsVisible() windows
-// is set to VISIBLE and no infinite loop is entered.
+// its occlusion state changed, a DCHECK occurs.
 TEST_F(WindowOcclusionTrackerTest, OcclusionStatesDontBecomeStable) {
   test::WindowOcclusionTrackerTestApi test_api;
 
@@ -1489,13 +1489,11 @@ TEST_F(WindowOcclusionTrackerTest, OcclusionStatesDontBecomeStable) {
   // Once the maximum number of times that occlusion can be recomputed is
   // reached, the occlusion state of all IsVisible() windows should be set to
   // VISIBLE.
-  delegate_a->set_expectation(Window::OcclusionState::VISIBLE);
-  delegate_d->set_expectation(Window::OcclusionState::HIDDEN);
-  EXPECT_FALSE(test_api.WasOcclusionRecomputedTooManyTimes());
-  window_d->Hide();
-  EXPECT_TRUE(test_api.WasOcclusionRecomputedTooManyTimes());
-  EXPECT_FALSE(delegate_a->is_expecting_call());
-  EXPECT_FALSE(delegate_d->is_expecting_call());
+  EXPECT_DCHECK_DEATH({
+    delegate_a->set_expectation(Window::OcclusionState::VISIBLE);
+    delegate_d->set_expectation(Window::OcclusionState::HIDDEN);
+    window_d->Hide();
+  });
 }
 
 // Verify that the occlusion states are correctly updated when a branch of the
@@ -1606,11 +1604,9 @@ TEST_F(WindowOcclusionTrackerTest,
 
   delegate_a->set_expectation(Window::OcclusionState::HIDDEN);
   delegate_b->set_expectation(Window::OcclusionState::HIDDEN);
-  EXPECT_FALSE(test_api.WasOcclusionRecomputedTooManyTimes());
-  window_b->Hide();
   // Hiding a child to |window_a| and hiding it shouldn't cause occlusion to be
-  // recomputed too many times.
-  EXPECT_FALSE(test_api.WasOcclusionRecomputedTooManyTimes());
+  // recomputed too many times (i.e. the call below shouldn't DCHECK).
+  window_b->Hide();
   EXPECT_FALSE(delegate_a->is_expecting_call());
   EXPECT_FALSE(delegate_b->is_expecting_call());
 }