Taking screenshots while running a test

chrome/browser/chromeos/login/screenshot_tester.h

Index: chrome/browser/chromeos/login/screenshot_tester.h
diff --git a/chrome/browser/chromeos/login/screenshot_tester.h b/chrome/browser/chromeos/login/screenshot_tester.h
new file mode 100644
index 0000000000000000000000000000000000000000..3e5a17d65309a92a95c00d00a2ef751acdc00855
--- /dev/null
+++ b/chrome/browser/chromeos/login/screenshot_tester.h
@@ -0,0 +1,67 @@
+// 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_LOGIN_SCREENSHOT_TESTER_H_
+#define CHROME_BROWSER_CHROMEOS_LOGIN_SCREENSHOT_TESTER_H_
+
+#include "ash/shell.h"

[ygorshenin1, 2014/07/21 09:48:23]

Could you please remove inclusions of modules which are not used in the header
file?

[Lisa Ignatyeva, 2014/07/21 13:41:03]

Done.

+#include "base/base_export.h"
+#include "base/bind_internal.h"
+#include "base/memory/weak_ptr.h"
+#include "base/prefs/pref_service.h"
+#include "base/run_loop.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/chromeos/login/login_manager_test.h"
+#include "chrome/browser/chromeos/login/startup_utils.h"
+#include "chrome/browser/chromeos/login/test/oobe_screen_waiter.h"
+#include "chrome/browser/ui/ash/screenshot_taker.h"
+#include "chrome/common/pref_names.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "ui/gfx/image/image.h"
+#include "ui/snapshot/snapshot.h"
+
+typedef scoped_refptr<base::RefCountedBytes> PNGFile;

[ygorshenin1, 2014/07/21 09:48:23]

Please, don't use typedef's in header files. You can move new type definition to
the class scope.

[Lisa Ignatyeva, 2014/07/21 13:41:04]

Done.

+
+// Key to turn on testing with screenshots
+const char kEnableTestScreenshots[] = "enable-test-screenshots";
+// Key to specify directory where the screenshots will be saved. Only
+// works if enable-test-screenshots is on.
+const char kScreenshotDest[] = "screenshot-dest";

[ygorshenin1, 2014/07/21 09:48:23]

How about to rename the switch to "test-screenshots-dir"?

[Denis Kuznetsov (DE-MUC), 2014/07/21 12:13:17]

I'd vote for including "destination" to switch name, as there can be also a
"source" directory with golden images.

[Lisa Ignatyeva, 2014/07/21 13:41:03]

Done.

+const char kUpdateGoldenScreenshots[] = "update-golden-screenshots";

[ygorshenin1, 2014/07/21 09:48:23]

How about to move all switches to chromeos/chromeos_switches.h?

[Denis Kuznetsov (DE-MUC), 2014/07/21 12:13:17]

Acknowledged.

[Lisa Ignatyeva, 2014/07/21 13:41:03]

Done.

[Lisa Ignatyeva, 2014/07/21 13:41:04]

Done.

+
+// A class that allows taking, saving and comparing screnshots while
+// running tests.
+class ScreenshotTester {
+ public:
+  ScreenshotTester();
+  virtual ~ScreenshotTester();
+
+  // Returns true if the screenshots should be taken and will be taken,
+  // false otherwise. Also gets all the information from the command line
+  // swithes.
+  bool TryInitialize();
+
+  // Does all the work that has been stated through switches:
+  // updates golden screenshot or takes a new screenshot and compares it
+  // with the golden one (this part is not implemented yet).
+  void Run(const std::string& file_name);
+
+  base::WeakPtrFactory<ScreenshotTester> weak_factory_;

[ygorshenin1, 2014/07/21 09:48:23]

Weak factory should be declared last in the class declaration. Otherwise it
would be possible to pass weak pointer to some field in the class, which is
declared after the factory, so it will be destructed after the factory. Thus,
weak pointer will be invalidated too early.

[Denis Kuznetsov (DE-MUC), 2014/07/21 12:13:17]

Also, WeakFactory should be private.

[Lisa Ignatyeva, 2014/07/21 13:41:04]

Done.

[Lisa Ignatyeva, 2014/07/21 13:41:04]

Done.

[Lisa Ignatyeva, 2014/07/21 13:41:04]

Done.

+
+ private:
+  bool update_golden_screenshot_;

[ygorshenin1, 2014/07/21 09:48:23]

Please, reorder fields and methods according to
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar....

[Lisa Ignatyeva, 2014/07/21 13:41:03]

Done.

+  void TakeScreenshot();
+  // Saves |png_data| as a new golden screenshot for this test.

[ygorshenin1, 2014/07/21 09:48:23]

Could you please add blank lines before comments?

[Lisa Ignatyeva, 2014/07/21 13:41:04]

Done.

+  void UpdateGoldenScreenshot(PNGFile png_data);
+  // Saves |png_data" as a current screenshot.
+  void ReturnScreenshot(PNGFile png_data);
+  base::FilePath screenshot_dest_;
+  base::FilePath golden_screenshot_path;

[ygorshenin1, 2014/07/21 09:48:23]

Class fields's names must be with an underscore at the end.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

[Lisa Ignatyeva, 2014/07/21 13:41:04]

Done.

+  base::RunLoop run_loop_;
+  base::Closure run_loop_quitter_;
+  PNGFile screenshot;

[ygorshenin1, 2014/07/21 09:48:23]

Could you please rename "screenshot" to "screenshot_"?

[ygorshenin1, 2014/07/21 09:48:23]

Add inclusion of "base/macros.h" and add
DISALLOW_COPY_AND_ASSIGN(ScreenshotTester) at the end of the class declaration.

[Lisa Ignatyeva, 2014/07/21 13:41:04]

Done.

[Lisa Ignatyeva, 2014/07/21 13:41:04]

Done.

+};

[ygorshenin1, 2014/07/21 09:48:23]

nit: insert a blank line after the class declaration, please.

[Lisa Ignatyeva, 2014/07/21 13:41:04]

Done.

+#endif  // CHROME_BROWSER_CHROMEOS_LOGIN_SCREENSHOT_TESTER_H_