chrome/browser/chromeos/login/screenshot_tester.h - Issue 405093003: Taking screenshots while running a test

Side by Side Diff: chrome/browser/chromeos/login/screenshot_tester.h

Issue 405093003: Taking screenshots while running a test (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Created 6 years, 5 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« chrome/browser/chromeos/login/login_ui_browsertest.cc ('K') | « chrome/browser/chromeos/login/login_ui_browsertest.cc ('k') | chrome/browser/chromeos/login/screenshot_tester.cc » ('j') | chrome/browser/chromeos/login/screenshot_tester.cc » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 // Copyright 2014 The Chromium Authors. All rights reserved.

	2 // Use of this source code is governed by a BSD-style license that can be

	3 // found in the LICENSE file.

	4

	5 #ifndef CHROME_BROWSER_CHROMEOS_LOGIN_SCREENSHOT_TESTER_H_

	6 #define CHROME_BROWSER_CHROMEOS_LOGIN_SCREENSHOT_TESTER_H_

	7

	8 #include "ash/shell.h"
	ygorshenin1 2014/07/21 09:48:23 Could you please remove inclusions of modules whic Could you please remove inclusions of modules which are not used in the header file? Lisa Ignatyeva 2014/07/21 13:41:03 Done. Show quoted text On 2014/07/21 09:48:23, ygorshenin1 wrote: > Could you please remove inclusions of modules which are not used in the header > file? Done.
	9 #include "base/base_export.h"

	10 #include "base/bind_internal.h"

	11 #include "base/memory/weak_ptr.h"

	12 #include "base/prefs/pref_service.h"

	13 #include "base/run_loop.h"

	14 #include "chrome/browser/browser_process.h"

	15 #include "chrome/browser/chromeos/login/login_manager_test.h"

	16 #include "chrome/browser/chromeos/login/startup_utils.h"

	17 #include "chrome/browser/chromeos/login/test/oobe_screen_waiter.h"

	18 #include "chrome/browser/ui/ash/screenshot_taker.h"

	19 #include "chrome/common/pref_names.h"

	20 #include "content/public/browser/browser_thread.h"

	21 #include "content/public/browser/notification_observer.h"

	22 #include "content/public/browser/notification_registrar.h"

	23 #include "ui/gfx/image/image.h"

	24 #include "ui/snapshot/snapshot.h"

	25

	26 typedef scoped_refptr<base::RefCountedBytes> PNGFile;
	ygorshenin1 2014/07/21 09:48:23 Please, don't use typedef's in header files. You c 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. Show quoted text On 2014/07/21 09:48:23, ygorshenin1 wrote: > Please, don't use typedef's in header files. You can move new type definition to > the class scope. Done.
	27

	28 // Key to turn on testing with screenshots

	29 const char kEnableTestScreenshots[] = "enable-test-screenshots";

	30 // Key to specify directory where the screenshots will be saved. Only

	31 // works if enable-test-screenshots is on.

	32 const char kScreenshotDest[] = "screenshot-dest";
	ygorshenin1 2014/07/21 09:48:23 How about to rename the switch to "test-screenshot 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 nam Show quoted text On 2014/07/21 09:48:23, ygorshenin1 wrote: > How about to rename the switch to "test-screenshots-dir"? 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. Show quoted text On 2014/07/21 09:48:23, ygorshenin1 wrote: > How about to rename the switch to "test-screenshots-dir"? Done.
	33 const char kUpdateGoldenScreenshots[] = "update-golden-screenshots";
	ygorshenin1 2014/07/21 09:48:23 How about to move all switches to chromeos/chromeo How about to move all switches to chromeos/chromeos_switches.h? Denis Kuznetsov (DE-MUC) 2014/07/21 12:13:17 Acknowledged. Show quoted text On 2014/07/21 09:48:23, ygorshenin1 wrote: > How about to move all switches to chromeos/chromeos_switches.h? Acknowledged. Lisa Ignatyeva 2014/07/21 13:41:03 Done. Show quoted text On 2014/07/21 09:48:23, ygorshenin1 wrote: > How about to move all switches to chromeos/chromeos_switches.h? Done. Lisa Ignatyeva 2014/07/21 13:41:04 Done. Show quoted text On 2014/07/21 12:13:17, Denis Kuznetsov wrote: > On 2014/07/21 09:48:23, ygorshenin1 wrote: > > How about to move all switches to chromeos/chromeos_switches.h? > > Acknowledged. Done.
	34

	35 // A class that allows taking, saving and comparing screnshots while

	36 // running tests.

	37 class ScreenshotTester {

	38 public:

	39 ScreenshotTester();

	40 virtual ~ScreenshotTester();

	41

	42 // Returns true if the screenshots should be taken and will be taken,

	43 // false otherwise. Also gets all the information from the command line

	44 // swithes.

	45 bool TryInitialize();

	46

	47 // Does all the work that has been stated through switches:

	48 // updates golden screenshot or takes a new screenshot and compares it

	49 // with the golden one (this part is not implemented yet).

	50 void Run(const std::string& file_name);

	51

	52 base::WeakPtrFactory<ScreenshotTester> weak_factory_;
	ygorshenin1 2014/07/21 09:48:23 Weak factory should be declared last in the class 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. Show quoted text On 2014/07/21 09:48:23, ygorshenin1 wrote: > 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. Also, WeakFactory should be private. Lisa Ignatyeva 2014/07/21 13:41:04 Done. Show quoted text On 2014/07/21 12:13:17, Denis Kuznetsov wrote: > On 2014/07/21 09:48:23, ygorshenin1 wrote: > > 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. > > Also, WeakFactory should be private. Done. Lisa Ignatyeva 2014/07/21 13:41:04 Done. Show quoted text On 2014/07/21 09:48:23, ygorshenin1 wrote: > 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. Done. Lisa Ignatyeva 2014/07/21 13:41:04 Done. Show quoted text On 2014/07/21 09:48:23, ygorshenin1 wrote: > 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. Done.
	53

	54 private:

	55 bool update_golden_screenshot_;
	ygorshenin1 2014/07/21 09:48:23 Please, reorder fields and methods according to ht 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. Show quoted text On 2014/07/21 09:48:23, ygorshenin1 wrote: > Please, reorder fields and methods according to > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar.... Done.
	56 void TakeScreenshot();

	57 // 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? Could you please add blank lines before comments? Lisa Ignatyeva 2014/07/21 13:41:04 Done. Show quoted text On 2014/07/21 09:48:23, ygorshenin1 wrote: > Could you please add blank lines before comments? Done.
	58 void UpdateGoldenScreenshot(PNGFile png_data);

	59 // Saves \|png_data" as a current screenshot.

	60 void ReturnScreenshot(PNGFile png_data);

	61 base::FilePath screenshot_dest_;

	62 base::FilePath golden_screenshot_path;
	ygorshenin1 2014/07/21 09:48:23 Class fields's names must be with an underscore at 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. Show quoted text On 2014/07/21 09:48:23, ygorshenin1 wrote: > Class fields's names must be with an underscore at the end. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab... Done.
	63 base::RunLoop run_loop_;

	64 base::Closure run_loop_quitter_;

	65 PNGFile screenshot;
	ygorshenin1 2014/07/21 09:48:23 Could you please rename "screenshot" to "screensho Could you please rename "screenshot" to "screenshot_"? ygorshenin1 2014/07/21 09:48:23 Add inclusion of "base/macros.h" and add DISALLOW_ 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. Show quoted text On 2014/07/21 09:48:23, ygorshenin1 wrote: > Could you please rename "screenshot" to "screenshot_"? Done. Lisa Ignatyeva 2014/07/21 13:41:04 Done. Show quoted text On 2014/07/21 09:48:23, ygorshenin1 wrote: > Add inclusion of "base/macros.h" and add > DISALLOW_COPY_AND_ASSIGN(ScreenshotTester) at the end of the class declaration. Done.
	66 };
	ygorshenin1 2014/07/21 09:48:23 nit: insert a blank line after the class declarati nit: insert a blank line after the class declaration, please. Lisa Ignatyeva 2014/07/21 13:41:04 Done. Show quoted text On 2014/07/21 09:48:23, ygorshenin1 wrote: > nit: insert a blank line after the class declaration, please. Done.
	67 #endif // CHROME_BROWSER_CHROMEOS_LOGIN_SCREENSHOT_TESTER_H_

OLD	NEW