Fix flakiness pixeltests on bots

On bot, often we can only catch black screen for dialog. I found a similar
bug crbug.com/687387 related to gpu. To repro the same result locally,
pass in --disable-gpu when running test locally.
To fix the flakiness, we don't catch the screen from HDC. Instead, we
catch from Layer::RequestCopyOfOutput.
This change also refactor some code to reuse some existing lib.

browser_tests.exe
  --build-revision=fd1bb443
  --test-launcher-filter-file=testing/buildbot/filters/pixel_browser_tests.filter
  --browser-ui-tests-verify-pixels
  --enable-pixel-output-in-tests
  --disable-gpu
  --no-luci-auth
can pass.

TEST: tested multiple times the tests won't catch black screen anymore.
Bug: 958242
Change-Id: Ie7df0e304fb74eb1d2b6c013612529dcdf549605
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1879506
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709634}

chrome/browser/ui/test/test_browser_dialog.cc

@@ -23,10 +23,10 @@
 
 #if defined(TOOLKIT_VIEWS)
 #include "base/strings/strcat.h"
+#include "ui/compositor/test/draw_waiter_for_test.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
 #include "ui/views/test/widget_test.h"
-#include "ui/views/widget/widget_observer.h"
 #endif
 
 namespace {
@@ -56,18 +56,6 @@ class WidgetCloser {
   DISALLOW_COPY_AND_ASSIGN(WidgetCloser);
 };
 
-class CompositorObserverEnd : public ui::CompositorObserver {
- public:
-  void OnCompositingEnded(ui::Compositor* compositor) override {
-    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
-                                                  run_loop_.QuitClosure());
-  }
-  void WaitForComplete() { run_loop_.Run(); }
-
- private:
-  base::RunLoop run_loop_;
-};
-
 #endif  // defined(TOOLKIT_VIEWS)
 
 }  // namespace
@@ -85,13 +73,6 @@ void TestBrowserDialog::PreShow() {
   UpdateWidgets();
 }
 
-void CompareAsync(const BrowserSkiaGoldPixelDiff* pixel_diff,
-                  const std::string& name,
-                  const views::View* view,
-                  bool* result) {
-  *result = pixel_diff->CompareScreenshot(name, view);
-}
-
 // This returns true if exactly one views widget was shown that is a dialog or
 // has a name matching the test-specified name, and if that window is in the
 // work area (if |should_verify_dialog_bounds_| is true).
@@ -120,26 +101,15 @@ bool TestBrowserDialog::VerifyUi() {
 #if !defined(OS_MACOSX)
   if (pixel_diff_) {
     // Wait for painting complete.
-    auto* compositor = dialog_widget->GetNativeView()->layer()->GetCompositor();
-    CompositorObserverEnd obend;
-    compositor->AddObserver(&obend);
-    obend.WaitForComplete();
-    compositor->RemoveObserver(&obend);
+    auto* compositor = dialog_widget->GetCompositor();
+    ui::DrawWaiterForTest::WaitForCompositingEnded(compositor);
 
-    base::ScopedAllowBlockingForTesting allow_blocking;
     pixel_diff_->Init(dialog_widget, "BrowserUiDialog");
     auto* test_info = testing::UnitTest::GetInstance()->current_test_info();
     const std::string test_name =
         base::StrCat({test_info->test_case_name(), "_", test_info->name()});
-    bool result = false;
-    base::RunLoop run_loop;
-    base::ThreadTaskRunnerHandle::Get()->PostTaskAndReply(
-        FROM_HERE,
-        base::BindOnce(CompareAsync, pixel_diff_.get(), test_name,
-                       dialog_widget->GetContentsView(), &result),
-        run_loop.QuitClosure());
-    run_loop.Run();
-    if (!result)
+    if (!pixel_diff_->CompareScreenshot(test_name,
+                                        dialog_widget->GetContentsView()))
       return false;
   }
 #endif  // OS_MACOSX

chrome/test/pixel/browser_skia_gold_pixel_diff.cc

@@ -5,6 +5,7 @@
 #include "chrome/test/pixel/browser_skia_gold_pixel_diff.h"
 
 #include "base/logging.h"
+#include "base/run_loop.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ -13,6 +14,18 @@
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 
+#if defined(USE_AURA)
+#include "ui/snapshot/snapshot_aura.h"
+#endif
+
+void SnapshotCallback(base::RunLoop* run_loop,
+                      gfx::Image* ret_image,
+                      gfx::Image image) {
+  *ret_image = image;
+  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                run_loop->QuitClosure());
+}
+
 BrowserSkiaGoldPixelDiff::BrowserSkiaGoldPixelDiff() = default;
 
 BrowserSkiaGoldPixelDiff::~BrowserSkiaGoldPixelDiff() = default;
@@ -28,12 +41,16 @@ bool BrowserSkiaGoldPixelDiff::GrabWindowSnapshotInternal(
     gfx::NativeWindow window,
     const gfx::Rect& snapshot_bounds,
     gfx::Image* image) const {
-  bool ret = ui::GrabWindowSnapshot(window, snapshot_bounds, image);
-  if (!ret) {
-    LOG(WARNING) << "Grab snapshot failed";
-    return false;
-  }
-  return true;
+  base::RunLoop run_loop;
+#if defined(USE_AURA)
+  ui::GrabWindowSnapshotAsyncAura(
+#else
+  ui::GrabWindowSnapshotAsync(
+#endif
+      window, snapshot_bounds,
+      base::BindOnce(&SnapshotCallback, &run_loop, image));
+  run_loop.Run();
+  return !image->IsEmpty();
 }
 
 bool BrowserSkiaGoldPixelDiff::CompareScreenshot(
@@ -48,6 +65,7 @@ bool BrowserSkiaGoldPixelDiff::CompareScreenshot(
   gfx::Image image;
   bool ret = GrabWindowSnapshotInternal(widget_->GetNativeWindow(), rc, &image);
   if (!ret) {
+    LOG(ERROR) << "Grab screenshot failed.";
     return false;
   }
   return SkiaGoldPixelDiff::CompareScreenshot(screenshot_name,

testing/buildbot/filters/pixel_browser_tests.filter

@@ -25,3 +25,6 @@ ProfileSigninConfirmationDialogTest.*
 TabGroupEditorBubbleViewDialogBrowserTest.*
 TabHoverCardBubbleViewBrowserTest.*
 OutdatedUpgradeBubbleTest.*
+
+# This test uses random network port and shows it on ui.
+-ContentSettingBubbleDialogTest.InvokeUi_popups