Ensure compositing test preference overrides are reset between tests

content/shell/browser/layout_test/blink_test_controller.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/shell/browser/layout_test/blink_test_controller.h"
 
 #include <stddef.h>
 
 #include <iostream>
 #include <set>
@@ skipping 204 matching lines @@
 
 // static
 BlinkTestController* BlinkTestController::Get() {
   DCHECK(instance_);
   return instance_;
 }
 
 BlinkTestController::BlinkTestController()
     : main_window_(NULL),
       test_phase_(BETWEEN_TESTS),
+      is_compositing_test_(false),
       is_leak_detection_enabled_(
           base::CommandLine::ForCurrentProcess()->HasSwitch(
               switches::kEnableLeakDetection)),
       crash_when_leak_found_(false),
       devtools_frontend_(NULL),
       render_process_host_observer_(this) {
   CHECK(!instance_);
   instance_ = this;
 
   if (is_leak_detection_enabled_) {
@@ skipping 69 matching lines @@
         ->GetRenderViewHost()
         ->GetWidget()
         ->GetView()
         ->SetSize(initial_size_);
     main_window_->web_contents()
         ->GetRenderViewHost()
         ->GetWidget()
         ->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());
 
     NavigationController::LoadURLParams params(test_url);
     params.transition_type = ui::PageTransitionFromInt(
         ui::PAGE_TRANSITION_TYPED | ui::PAGE_TRANSITION_FROM_ADDRESS_BAR);
     params.should_clear_history_list = true;
     main_window_->web_contents()->GetController().LoadURLWithParams(params);
     main_window_->web_contents()->Focus();
   }
   main_window_->web_contents()->GetRenderViewHost()->GetWidget()->SetActive(
       true);
   main_window_->web_contents()->GetRenderViewHost()->GetWidget()->Focus();
   return true;
 }
 
 bool BlinkTestController::ResetAfterLayoutTest() {
   DCHECK(CalledOnValidThread());
   printer_->PrintTextFooter();
   printer_->PrintImageFooter();
   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();
   prefs_ = WebPreferences();
   should_override_prefs_ = false;
 
 #if defined(OS_ANDROID)
   // Re-using the shell's main window on Android causes issues with networking
   // requests never succeeding. See http://crbug.com/277652.
   DiscardMainWindow();
@@ skipping 589 matching lines @@
   } else {
     printer_->AddErrorMessage(base::StringPrintf(
         "FAIL: Unexpected sendBluetoothManualChooserEvent() event name '%s'.",
         event_name.c_str()));
     return;
   }
   bluetooth_chooser_factory_->SendEvent(event, argument);
 }
 
 }  // namespace content