Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
405093003:
Taking screenshots while running a test (Closed)
|
Created:
6 years, 5 months ago by Lisa Ignatyeva Modified:
6 years, 5 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionTaking screenshots while running a test
Adds a class ScreenshotTester, which now adds an opportunity to take a screenshot while running a test and then
save it to a directory specified by some switch. it is now fully included in LoginUIVisible test.
Taking screenshots also requires waiting until all the animation finishes loading. To handle with it, a class AnimationDelayHandler
is implemented (in LoginUIVisible).
To turn taking screenshots on, switch --enable-test-screenshots should be used. To specify the directory where all the screenshots will be
stored, use --screenshot-dest=value, where value is the full path to the directory.
It is planned that working with this class will have two modes: taking a new screenshot and saving it, and taking a screenshot and comparing
it with the previously stored one. Now only the first mode is implemented, and running it also requires a switch --update-golden-screenshots.
Pixel output is usually off in the tests, so before taking screenshots switches --enable-pixel-output-in-tests and --ui-enable-impl-side-painting should be also used.
BUG=395653
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284504
Patch Set 1 #
Total comments: 53
Patch Set 2 : Lots of style bugs fixed; switches restuctured, renamed and moved #Patch Set 3 : Switches restructured again #
Total comments: 8
Patch Set 4 : added comments for ScreenshotTester fields & fixed some minor style bugs #
Total comments: 2
Patch Set 5 : added namespace chromeos & removed golden_screenshot_path_ field from ScreenshotTester #
Total comments: 6
Patch Set 6 : minor fixes #Patch Set 7 : last DCHECK replaced with DCHECK_CURRENTLY_ON #
Messages
Total messages: 23 (0 generated)
First bunch of comments. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/login_ui_browsertest.cc (right): https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/login_ui_browsertest.cc:67: bool waiter_loop_is_on_, login_or_lock_webui_visible_; nit: declare "login_or_lock_webui_visible_" on a separate line. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/login_ui_browsertest.cc:142: AnimationDelayHandler animation_delay_handler; Move these fields to the protected part of the LoginUITest. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/login_ui_browsertest.cc:152: if (!enable_test_screenshots_) { nit: replace "if (!enable_test_screenshots_)" by "else". https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/login_ui_browsertest.cc:153: LOG(ERROR) << "Screenshots will not be taken"; Probably it's not an ERROR but a WARNING? https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screenshot_tester.cc (right): https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.cc:34: if (!command_line.HasSwitch(kEnableTestScreenshots)) { nit: curly braces are not needed here. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.cc:37: bool screenshot_dest_enabled_ = command_line.HasSwitch(kScreenshotDest); Local variables must be declared without underscore at the end. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.cc:47: if (screenshot_dest_enabled_) { As you're always use both flags: "--enable-test-screenshots" and "--screenshot-dest", why not to leave just "screenshot-dest" switch? https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.cc:104: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); nit: move DCHECK to the beginning of the method. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.cc:106: PNGFile screenshot; This variable is unused. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screenshot_tester.h (right): https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:8: #include "ash/shell.h" Could you please remove inclusions of modules which are not used in the header file? https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:26: typedef scoped_refptr<base::RefCountedBytes> PNGFile; Please, don't use typedef's in header files. You can move new type definition to the class scope. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:32: const char kScreenshotDest[] = "screenshot-dest"; How about to rename the switch to "test-screenshots-dir"? https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:33: const char kUpdateGoldenScreenshots[] = "update-golden-screenshots"; How about to move all switches to chromeos/chromeos_switches.h? https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:52: base::WeakPtrFactory<ScreenshotTester> weak_factory_; 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. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:55: bool update_golden_screenshot_; Please, reorder fields and methods according to http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar.... https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:57: // Saves |png_data| as a new golden screenshot for this test. Could you please add blank lines before comments? https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:62: base::FilePath golden_screenshot_path; Class fields's names must be with an underscore at the end. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab... https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:65: PNGFile screenshot; Add inclusion of "base/macros.h" and add DISALLOW_COPY_AND_ASSIGN(ScreenshotTester) at the end of the class declaration. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:65: PNGFile screenshot; Could you please rename "screenshot" to "screenshot_"? https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:66: }; nit: insert a blank line after the class declaration, please. https://codereview.chromium.org/405093003/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (left): https://codereview.chromium.org/405093003/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1118: nit: could you please restore back a blank line?
https://chromiumcodereview.appspot.com/405093003/diff/1/chrome/browser/chrome... File chrome/browser/chromeos/login/screenshot_tester.cc (right): https://chromiumcodereview.appspot.com/405093003/diff/1/chrome/browser/chrome... chrome/browser/chromeos/login/screenshot_tester.cc:47: if (screenshot_dest_enabled_) { On 2014/07/21 09:48:22, ygorshenin1 wrote: > As you're always use both flags: "--enable-test-screenshots" and > "--screenshot-dest", why not to leave just "screenshot-dest" switch? Later there will be a stage when we compare screenshots with sample screenshots, not dumping them. https://chromiumcodereview.appspot.com/405093003/diff/1/chrome/browser/chrome... chrome/browser/chromeos/login/screenshot_tester.cc:104: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2014/07/21 09:48:23, ygorshenin1 wrote: > nit: move DCHECK to the beginning of the method. Acknowledged. https://chromiumcodereview.appspot.com/405093003/diff/1/chrome/browser/chrome... File chrome/browser/chromeos/login/screenshot_tester.h (right): https://chromiumcodereview.appspot.com/405093003/diff/1/chrome/browser/chrome... chrome/browser/chromeos/login/screenshot_tester.h:32: const char kScreenshotDest[] = "screenshot-dest"; 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. https://chromiumcodereview.appspot.com/405093003/diff/1/chrome/browser/chrome... chrome/browser/chromeos/login/screenshot_tester.h:33: const char kUpdateGoldenScreenshots[] = "update-golden-screenshots"; On 2014/07/21 09:48:23, ygorshenin1 wrote: > How about to move all switches to chromeos/chromeos_switches.h? Acknowledged. https://chromiumcodereview.appspot.com/405093003/diff/1/chrome/browser/chrome... chrome/browser/chromeos/login/screenshot_tester.h:52: base::WeakPtrFactory<ScreenshotTester> weak_factory_; 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.
The CQ bit was checked by elizavetai@chromium.org
The CQ bit was unchecked by elizavetai@chromium.org
The CQ bit was checked by elizavetai@chromium.org
The CQ bit was unchecked by elizavetai@chromium.org
https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/login_ui_browsertest.cc (right): https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/login_ui_browsertest.cc:67: bool waiter_loop_is_on_, login_or_lock_webui_visible_; On 2014/07/21 09:48:22, ygorshenin1 wrote: > nit: declare "login_or_lock_webui_visible_" on a separate line. Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/login_ui_browsertest.cc:142: AnimationDelayHandler animation_delay_handler; On 2014/07/21 09:48:22, ygorshenin1 wrote: > Move these fields to the protected part of the LoginUITest. Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/login_ui_browsertest.cc:152: if (!enable_test_screenshots_) { On 2014/07/21 09:48:22, ygorshenin1 wrote: > nit: replace "if (!enable_test_screenshots_)" by "else". Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/login_ui_browsertest.cc:153: LOG(ERROR) << "Screenshots will not be taken"; On 2014/07/21 09:48:22, ygorshenin1 wrote: > Probably it's not an ERROR but a WARNING? Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screenshot_tester.cc (right): https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.cc:34: if (!command_line.HasSwitch(kEnableTestScreenshots)) { On 2014/07/21 09:48:22, ygorshenin1 wrote: > nit: curly braces are not needed here. Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.cc:37: bool screenshot_dest_enabled_ = command_line.HasSwitch(kScreenshotDest); On 2014/07/21 09:48:22, ygorshenin1 wrote: > Local variables must be declared without underscore at the end. Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.cc:47: if (screenshot_dest_enabled_) { On 2014/07/21 09:48:22, ygorshenin1 wrote: > As you're always use both flags: "--enable-test-screenshots" and > "--screenshot-dest", why not to leave just "screenshot-dest" switch? Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.cc:47: if (screenshot_dest_enabled_) { On 2014/07/21 12:13:16, Denis Kuznetsov wrote: > On 2014/07/21 09:48:22, ygorshenin1 wrote: > > As you're always use both flags: "--enable-test-screenshots" and > > "--screenshot-dest", why not to leave just "screenshot-dest" switch? > Later there will be a stage when we compare screenshots with sample screenshots, > not dumping them. Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.cc:104: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2014/07/21 09:48:23, ygorshenin1 wrote: > nit: move DCHECK to the beginning of the method. Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.cc:104: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2014/07/21 12:13:16, Denis Kuznetsov wrote: > On 2014/07/21 09:48:23, ygorshenin1 wrote: > > nit: move DCHECK to the beginning of the method. > > Acknowledged. Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.cc:106: PNGFile screenshot; On 2014/07/21 09:48:22, ygorshenin1 wrote: > This variable is unused. Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screenshot_tester.h (right): https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:8: #include "ash/shell.h" 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. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:26: typedef scoped_refptr<base::RefCountedBytes> PNGFile; 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. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:32: const char kScreenshotDest[] = "screenshot-dest"; On 2014/07/21 09:48:23, ygorshenin1 wrote: > How about to rename the switch to "test-screenshots-dir"? Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:33: const char kUpdateGoldenScreenshots[] = "update-golden-screenshots"; On 2014/07/21 09:48:23, ygorshenin1 wrote: > How about to move all switches to chromeos/chromeos_switches.h? Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:33: const char kUpdateGoldenScreenshots[] = "update-golden-screenshots"; 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. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:52: base::WeakPtrFactory<ScreenshotTester> weak_factory_; 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. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:52: base::WeakPtrFactory<ScreenshotTester> weak_factory_; 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. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:52: base::WeakPtrFactory<ScreenshotTester> weak_factory_; 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. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:55: bool update_golden_screenshot_; 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. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:57: // Saves |png_data| as a new golden screenshot for this test. On 2014/07/21 09:48:23, ygorshenin1 wrote: > Could you please add blank lines before comments? Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:62: base::FilePath golden_screenshot_path; 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. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:65: PNGFile screenshot; On 2014/07/21 09:48:23, ygorshenin1 wrote: > Could you please rename "screenshot" to "screenshot_"? Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:65: PNGFile screenshot; 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. https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screenshot_tester.h:66: }; On 2014/07/21 09:48:23, ygorshenin1 wrote: > nit: insert a blank line after the class declaration, please. Done. https://codereview.chromium.org/405093003/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (left): https://codereview.chromium.org/405093003/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1118: On 2014/07/21 09:48:23, ygorshenin1 wrote: > nit: could you please restore back a blank line? Done.
https://chromiumcodereview.appspot.com/405093003/diff/40001/chrome/browser/ch... File chrome/browser/chromeos/login/screenshot_tester.h (right): https://chromiumcodereview.appspot.com/405093003/diff/40001/chrome/browser/ch... chrome/browser/chromeos/login/screenshot_tester.h:20: typedef scoped_refptr<base::RefCountedBytes> PNGFile; This is used only in private section. Move typedef to private section as well. https://chromiumcodereview.appspot.com/405093003/diff/40001/chrome/browser/ch... chrome/browser/chromeos/login/screenshot_tester.h:40: // Saves |png_data" as a current screenshot. nit: fix typo https://chromiumcodereview.appspot.com/405093003/diff/40001/chrome/browser/ch... chrome/browser/chromeos/login/screenshot_tester.h:42: base::FilePath screenshot_dest_; Extra line between methods and fields. Some comments for fields are welcome. https://chromiumcodereview.appspot.com/405093003/diff/40001/chrome/browser/ch... chrome/browser/chromeos/login/screenshot_tester.h:47: base::WeakPtrFactory<ScreenshotTester> weak_factory_; Move factory down - it should be last field, as Yuri noted.
https://codereview.chromium.org/405093003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/screenshot_tester.h (right): https://codereview.chromium.org/405093003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.h:20: typedef scoped_refptr<base::RefCountedBytes> PNGFile; On 2014/07/21 13:49:43, Denis Kuznetsov wrote: > This is used only in private section. Move typedef to private section as well. Done. https://codereview.chromium.org/405093003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.h:40: // Saves |png_data" as a current screenshot. On 2014/07/21 13:49:43, Denis Kuznetsov wrote: > nit: fix typo Done. https://codereview.chromium.org/405093003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.h:42: base::FilePath screenshot_dest_; On 2014/07/21 13:49:43, Denis Kuznetsov wrote: > Extra line between methods and fields. > Some comments for fields are welcome. Done. https://codereview.chromium.org/405093003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.h:47: base::WeakPtrFactory<ScreenshotTester> weak_factory_; On 2014/07/21 13:49:43, Denis Kuznetsov wrote: > Move factory down - it should be last field, as Yuri noted. Done.
https://chromiumcodereview.appspot.com/405093003/diff/60001/chrome/browser/ch... File chrome/browser/chromeos/login/screenshot_tester.h (right): https://chromiumcodereview.appspot.com/405093003/diff/60001/chrome/browser/ch... chrome/browser/chromeos/login/screenshot_tester.h:15: namespace chromeos here and in .cc file
Also removed an unnecessary golden_screenshot_path_ field from ScreenshotTester. https://codereview.chromium.org/405093003/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/screenshot_tester.h (right): https://codereview.chromium.org/405093003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.h:15: On 2014/07/21 14:25:33, Denis Kuznetsov wrote: > namespace chromeos here and in .cc file Done.
lgtm
The CQ bit was checked by elizavetai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/elizavetai@chromium.org/405093003/80001
The CQ bit was unchecked by elizavetai@chromium.org
Please create a tracking bug and reference it here as: BUG=id
https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/login_ui_browsertest.cc (right): https://codereview.chromium.org/405093003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/login_ui_browsertest.cc:67: bool waiter_loop_is_on_, login_or_lock_webui_visible_; Not done :) On 2014/07/21 13:41:02, Lisa Ignatyeva wrote: > On 2014/07/21 09:48:22, ygorshenin1 wrote: > > nit: declare "login_or_lock_webui_visible_" on a separate line. > > Done. https://codereview.chromium.org/405093003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/login_ui_browsertest.cc (right): https://codereview.chromium.org/405093003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/login_ui_browsertest.cc:148: if (enable_test_screenshots_) { nit: you could get rid of curly braces in both branches. https://codereview.chromium.org/405093003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/screenshot_tester.cc (right): https://codereview.chromium.org/405093003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:90: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); nit: it's possible to simplify the check: DCHECK_CURRENTLY_ON(content::BrowserThread::UI). https://codereview.chromium.org/405093003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/screenshot_tester.h (right): https://codereview.chromium.org/405093003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.h:36: typedef scoped_refptr<base::RefCountedBytes> PNGFile; nit: add an empty line after type declaration + add a comment to the TakeScreenshot() method.
https://codereview.chromium.org/405093003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/login_ui_browsertest.cc (right): https://codereview.chromium.org/405093003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/login_ui_browsertest.cc:148: if (enable_test_screenshots_) { On 2014/07/21 16:38:43, ygorshenin1 wrote: > nit: you could get rid of curly braces in both branches. I'd prefer keeping them both. As I understand, it's OK according to the code style (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...) https://codereview.chromium.org/405093003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/screenshot_tester.cc (right): https://codereview.chromium.org/405093003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:90: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2014/07/21 16:38:43, ygorshenin1 wrote: > nit: it's possible to simplify the check: > DCHECK_CURRENTLY_ON(content::BrowserThread::UI). Done. https://codereview.chromium.org/405093003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/screenshot_tester.h (right): https://codereview.chromium.org/405093003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.h:36: typedef scoped_refptr<base::RefCountedBytes> PNGFile; On 2014/07/21 16:38:43, ygorshenin1 wrote: > nit: add an empty line after type declaration + add a comment to the > TakeScreenshot() method. Done.
lgtm
The CQ bit was checked by elizavetai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/elizavetai@chromium.org/405093003/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
Message was sent while issue was closed.
Change committed as 284504 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
