Generalizing architecture for screenshot testing

Moving parts connected to screenshot testing from login_ui_browsertest.cc to separate files. Uses BrowserTestBase::Admixture added in https://codereview.chromium.org/468493003/.

BUG=395653
Committed: https://crrev.com/e9d0473ab031c24ba51633a61ea1539d1827f74f
Cr-Commit-Position: refs/heads/master@{#295964}

[Nikita (slow), 2014-08-06 10:12:42]

Pavel, please review.

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

https://codereview.chromium.org/441263002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_ui_browsertest.cc (right):

https://codereview.chromium.org/441263002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_ui_browsertest.cc:94:
RunScreenshotTesting("LoginUITest-LoginUIVisible");
Note for future improvements: we can form test name dynamically, see
https://code.google.com/p/googletest/wiki/AdvancedGuide#Getting_the_Current_T...

https://codereview.chromium.org/441263002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screenshot_test.cc (right):

https://codereview.chromium.org/441263002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_test.cc:18: }
Add a new line.

https://codereview.chromium.org/441263002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_test.cc:38: void
ScreenshotTest::Observe(int type,
Why do we need this method?

https://codereview.chromium.org/441263002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screenshot_test.h (right):

https://codereview.chromium.org/441263002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_test.h:23: class ScreenshotTest :
public LoginManagerTest,
Can't we derieve LoginManagerTest from ScreenshotTest instead?

https://codereview.chromium.org/441263002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_test.h:84: ScreenshotTester
screenshot_tester;
screenshot_tester_

https://codereview.chromium.org/441263002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screenshot_tester.cc (left):

https://codereview.chromium.org/441263002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_tester.cc:62: CHECK(false) << "Invalid
mode for screenshot testing: " << mode;
Use "LOG_ASSERT(condition) << message" instead of "if (!condition) CHECK(false)
<< message"

https://codereview.chromium.org/441263002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screenshot_tester.cc (right):

https://codereview.chromium.org/441263002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_tester.cc:39:
ScreenshotTester::ScreenshotTester() : test_mode_(false), weak_factory_(this) {
test_mode_ should be enum.

https://codereview.chromium.org/441263002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_tester.cc:45: bool
ScreenshotTester::TryInitialize() {
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.

[Lisa Ignatyeva, 2014-08-19 11:51:23]

I've completely rewritten the architecture. Now screenshot testing part is not a
test class any more, but a BrowserTestBase::Admixture introduced in 
https://codereview.chromium.org/468493003/ (not committed yet).

[dzhioev (left Google), 2014-08-25 17:01:19]

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_ui_browsertest.cc (right):

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_ui_browsertest.cc:29: }
Either add new line here or remove empty line above.

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_ui_browsertest.cc:87:
ScreenshotComparingAdmixtureForLoginUIVisible* screenshot_comparing;
screenshot_comparing_;

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screenshot_comparing_admixture.cc (right):

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_comparing_admixture.cc:16:
ScreenshotComparingAdmixture::ScreenshotComparingAdmixture() : registrar_() {
No need for |registrar_| explicit initialization.

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screenshot_comparing_admixture.h (right):

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_comparing_admixture.h:24: public
content::NotificationObserver {
Fix indentation.

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_comparing_admixture.h:42: const
content::NotificationDetails& details) OVERRIDE = 0;
I think this method is not needed in class interface.

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_comparing_admixture.h:60: // virtual
void RunTestOnMainThread() OVERRIDE;
Delete?

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_comparing_admixture.h:81: // virtual
void TestBody() OVERRIDE;
Delete?

[Lisa Ignatyeva, 2014-08-25 17:46:29]

Patchset #4 (id:60001) has been deleted

[Lisa Ignatyeva, 2014-08-25 17:57:54]

Most of the animation handling is not part of ScreenshotComparingAdmixture
anymore, it now handles only specific compositor-related animation problems.

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_ui_browsertest.cc (right):

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_ui_browsertest.cc:29: }
On 2014/08/25 17:01:19, dzhioev wrote:
> Either add new line here or remove empty line above.

Done.

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_ui_browsertest.cc:87:
ScreenshotComparingAdmixtureForLoginUIVisible* screenshot_comparing;
On 2014/08/25 17:01:19, dzhioev wrote:
> screenshot_comparing_;

Done.

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screenshot_comparing_admixture.cc (right):

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_comparing_admixture.cc:16:
ScreenshotComparingAdmixture::ScreenshotComparingAdmixture() : registrar_() {
On 2014/08/25 17:01:19, dzhioev wrote:
> No need for |registrar_| explicit initialization.

Done.

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screenshot_comparing_admixture.h (right):

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_comparing_admixture.h:24: public
content::NotificationObserver {
On 2014/08/25 17:01:19, dzhioev wrote:
> Fix indentation.

Done.

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_comparing_admixture.h:42: const
content::NotificationDetails& details) OVERRIDE = 0;
On 2014/08/25 17:01:19, dzhioev wrote:
> I think this method is not needed in class interface.

Done.

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_comparing_admixture.h:60: // virtual
void RunTestOnMainThread() OVERRIDE;
On 2014/08/25 17:01:19, dzhioev wrote:
> Delete?

Done.

https://codereview.chromium.org/441263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screenshot_comparing_admixture.h:81: // virtual
void TestBody() OVERRIDE;
On 2014/08/25 17:01:19, dzhioev wrote:
> Delete?

Done.

[Lisa Ignatyeva, 2014-09-17 12:29:12]

I've rewritten it again, now using stuff from this
https://codereview.chromium.org/574703002/ CL

[dzhioev (left Google), 2014-09-18 04:18:25]

Much better! Thanks.
LGTM with nit

https://codereview.chromium.org/441263002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screenshot_testing_mixin.cc (right):

https://codereview.chromium.org/441263002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screenshot_testing_mixin.cc:23: VLOG(0) <<
"setting up!";
nit: Please remove this log message. Or make it more informative and strict =)

[Lisa Ignatyeva, 2014-09-18 15:53:46]

Patchset #7 (id:140001) has been deleted

[Lisa Ignatyeva, 2014-09-22 12:48:02]

The CQ bit was checked by elizavetai@chromium.org

[Lisa Ignatyeva, 2014-09-22 12:48:06]

The CQ bit was unchecked by elizavetai@chromium.org

[Lisa Ignatyeva, 2014-09-22 12:50:28]

The CQ bit was checked by elizavetai@chromium.org

[Lisa Ignatyeva, 2014-09-22 12:50:59]

The CQ bit was unchecked by elizavetai@chromium.org

[Lisa Ignatyeva, 2014-09-22 12:51:07]

Patchset #8 (id:180001) has been deleted

[Lisa Ignatyeva, 2014-09-22 12:51:11]

The CQ bit was checked by elizavetai@chromium.org

[commit-bot: I haz the power, 2014-09-22 12:51:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/441263002/160001

[commit-bot: I haz the power, 2014-09-22 14:00:09]

Committed patchset #7 (id:160001) as dbaf89089d1a32f44d5e7b1bfb823f6f599bce87

[commit-bot: I haz the power, 2014-09-22 14:00:51]

Patchset 7 (id:??) landed as
https://crrev.com/e9d0473ab031c24ba51633a61ea1539d1827f74f
Cr-Commit-Position: refs/heads/master@{#295964}