Ensure compositing test preference overrides are reset between tests

content/shell/browser/layout_test/blink_test_controller.cc

Index: content/shell/browser/layout_test/blink_test_controller.cc
diff --git a/content/shell/browser/layout_test/blink_test_controller.cc b/content/shell/browser/layout_test/blink_test_controller.cc
index eb1a9139eaf109cf88ed84778ee8be2fcd8739ec..c78a5a5d8b66d03fa7c04524448bee7f734e7654 100644
--- a/content/shell/browser/layout_test/blink_test_controller.cc
+++ b/content/shell/browser/layout_test/blink_test_controller.cc
@@ -222,6 +222,7 @@ BlinkTestController* BlinkTestController::Get() {
 BlinkTestController::BlinkTestController()
     : main_window_(NULL),
       test_phase_(BETWEEN_TESTS),
+      is_compositing_test_(false),
       is_leak_detection_enabled_(
           base::CommandLine::ForCurrentProcess()->HasSwitch(
               switches::kEnableLeakDetection)),
@@ -311,6 +312,12 @@ bool BlinkTestController::PrepareForLayoutTest(
         ->WasResized();
     RenderViewHost* render_view_host =
         main_window_->web_contents()->GetRenderViewHost();
+    if (is_compositing_test_) {

[Xianzhu, 2016/08/04 15:53:26]

Can we just omit this condition, to ensure the prefs are reset whatever prefs
are overridden?

Also a question: when is BlinkTestController::OverrideWebkitPrefs() called? I
saw prefs are override in it. Should it be called for every test?

[pdr., 2016/08/08 21:29:40]

Good idea. I thought that might be too slow but found we actually did it before
https://crrev.com/d4c23ca618abaeeeba8b7f707bb2b1cb4460d642.
The !main_window_ codepath seems to be hit for a process's first test run which
will call OverrideWebkitPrefs through the following:
RenderViewHostImpl::GetWebkitPreferences
RenderViewHostImpl::OnWebkitPreferencesChanged
RenderViewHostImpl::ComputeWebkitPrefs
LayoutTestContentBrowserClient::OverrideWebkitPrefs

For subsequent test runs (main_window_ != null), we'll now explicitly override
the preferences every time with the latest patch.

+      // Force the existing host to recompute default preferences and pickup
+      // compositing preference overrides.
+      render_view_host->OnWebkitPreferencesChanged();
+      default_prefs_ = render_view_host->GetWebkitPreferences();
+    }
     render_view_host->UpdateWebkitPreferences(default_prefs_);

[Xianzhu, 2016/08/04 16:04:52]

Would it work to just change the above call to the following and omit the added
code line 315-320?

  render_view_host->OnWebkitPreferencesChanged();
  default_prefs_ = render_view_host->GetWebkitPreferences()

     HandleNewRenderFrameHost(render_view_host->GetMainFrame());
 
@@ -334,7 +341,14 @@ bool BlinkTestController::ResetAfterLayoutTest() {
   printer_->CloseStderr();
   did_send_initial_test_configuration_ = false;
   test_phase_ = BETWEEN_TESTS;
-  is_compositing_test_ = false;
+  if (is_compositing_test_) {
+    // Compositing tests change preferences so explicitly reset them.
+    ApplyLayoutTestDefaultPreferences(&default_prefs_);
+    RenderViewHost* render_view_host =
+        main_window_->web_contents()->GetRenderViewHost();
+    render_view_host->UpdateWebkitPreferences(default_prefs_);
+    is_compositing_test_ = false;

[Xianzhu, 2016/08/04 16:04:52]

1. Is the above code necessary as we always update preferences before starting a
layout test?

2. If this is necessary, should we put is_compositing_test_ = false at the
beginning of this block?

+  }
   enable_pixel_dumping_ = false;
   expected_pixel_hash_.clear();
   test_url_ = GURL();