Generalizing architecture for screenshot testing

chrome/browser/chromeos/login/screenshot_tester.cc

 // Copyright 2014 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 "chrome/browser/chromeos/login/screenshot_tester.h"
 
 #include "ash/shell.h"
 #include "base/base_export.h"
 #include "base/bind_internal.h"
 #include "base/command_line.h"
@@ skipping 18 matching lines @@
 // Sets test mode for screenshot testing.
 const char kTestMode[] = "test";
 
 // Sets update mode for screenshot testing.
 const char kUpdateMode[] = "update";
 
 }  // namespace
 
 namespace chromeos {
 
 ScreenshotTester::ScreenshotTester() : test_mode_(false), weak_factory_(this) {

[dzhioev (left Google), 2014/08/07 11:23:56]

test_mode_ should be enum.

 }
 
 ScreenshotTester::~ScreenshotTester() {
 }
 
 bool ScreenshotTester::TryInitialize() {

[dzhioev (left Google), 2014/08/07 11:23:56]

I don't like that this class use command line as a source for construction
params. Can you please change this class in a way that it will receive all the
configuration params through constructor. Then you can create factory method
"ScreenshotTester* ScreenshotTester::FromCommandLine" that will analyze command
line switches and construct ScreenshotTester.

   CommandLine& command_line = *CommandLine::ForCurrentProcess();
   if (!command_line.HasSwitch(switches::kEnableScreenshotTestingWithMode))
     return false;
-  if (!command_line.HasSwitch(::switches::kEnablePixelOutputInTests) ||
-      !command_line.HasSwitch(::switches::kUIEnableImplSidePainting)) {
-    // TODO(elizavetai): make turning on --enable-pixel-output-in-tests
-    // and --ui-enable-impl-side-painting automatical.
-    LOG(ERROR) << "--enable-pixel-output-in-tests and "
-               << "--ui-enable-impl-side-painting are required to take "
-               << "screenshots";
-    return false;
-  }
-
   std::string mode = command_line.GetSwitchValueASCII(
       switches::kEnableScreenshotTestingWithMode);
   if (mode != kUpdateMode && mode != kTestMode) {
     CHECK(false) << "Invalid mode for screenshot testing: " << mode;

[dzhioev (left Google), 2014/08/07 11:23:56]

Use "LOG_ASSERT(condition) << message" instead of "if (!condition) CHECK(false)
<< message"

   }
 
   if (!command_line.HasSwitch(chromeos::switches::kGoldenScreenshotsDir)) {
     CHECK(false) << "No directory for golden screenshots specified";
   }
 
   golden_screenshots_dir_ =
       command_line.GetSwitchValuePath(switches::kGoldenScreenshotsDir);
 
   if (mode == kTestMode) {
@@ skipping 41 matching lines @@
                << screenshot_path.DirName().value();
     return false;
   }
   if (static_cast<size_t>(
           base::WriteFile(screenshot_path,
                           reinterpret_cast<char*>(&(png_data->data()[0])),
                           png_data->size())) != png_data->size()) {
     LOG(ERROR) << "Can't save screenshot " << file_name;
     return false;
   }
-  VLOG(0) << "Screenshot " << file_name << ".png saved successfully to "
-          << screenshot_dir.value();
+  VLOG(0) << "Screenshot " << test_name_ + "_" + file_name + ".png"
+          << " saved successfully to " << screenshot_dir.value();
   return true;
 }
 
 void ScreenshotTester::ReturnScreenshot(const PNGFile& screenshot,
                                         PNGFile png_data) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   screenshot->data() = png_data->data();
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE, run_loop_quitter_);
 }
@@ skipping 79 matching lines @@
     VLOG(0) << "Current screenshot and diff picture saved to "
             << artifacts_dir_.value();
   } else {
     VLOG(0) << "Current screenshot matches the golden screenshot. Screenshot "
                "testing passed.";
   }
   ASSERT_TRUE(screenshots_match);
 }
 
 }  // namespace chromeos