Fix snapshotting on linux by creating a separate automation path for

Fix test snapshotting on linux by creating a separate automation path for
snapshots outside of the thumbnail generator.
BUG=69370, 66371, 63022
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107645

[kkania, 2011-10-18 01:11:48]

Motivation: current snapshot system for webdriver is broken on linux, doesn't
give exact results on all platforms
(http://code.google.com/p/selenium/issues/detail?id=2536), and has
flaky/disabled tests.

This CL moves automation snapshotting off of the ThumbnailGenerator and onto a
simple mechanism in AutomationRendererHelper. It snapshots the entire page by
scrolling the main frame, instead of resizing. The CL also deletes the
automation proxy ui tests in exchange for some render view tests.

[Paweł Hajdan Jr., 2011-10-18 10:21:35]

Thank you for work on this, just some more work is needed.

http://codereview.chromium.org/8294030/diff/2001/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/8294030/diff/2001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.cc:1911: overall_success
= bytes_written == static_cast<int>(png_data.size());
nit: Parentheses around bytes_written == ... please.

http://codereview.chromium.org/8294030/diff/2001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.cc:1912: if (!success)
nit: Did you mean overall_success here?

http://codereview.chromium.org/8294030/diff/2001/chrome/browser/automation/au...
File chrome/browser/automation/automation_tab_helper.h (right):

http://codereview.chromium.org/8294030/diff/2001/chrome/browser/automation/au...
chrome/browser/automation/automation_tab_helper.h:21: class Size;
nit: Is this needed?

http://codereview.chromium.org/8294030/diff/2001/chrome/renderer/automation/a...
File chrome/renderer/automation/automation_renderer_helper.cc (right):

http://codereview.chromium.org/8294030/diff/2001/chrome/renderer/automation/a...
chrome/renderer/automation/automation_renderer_helper.cc:41: // We don't want to
change the widget's layout by resizing. Just scroll
Please get additional reviewer for this WebKit stuff.

http://codereview.chromium.org/8294030/diff/2001/chrome/renderer/automation/a...
chrome/renderer/automation/automation_renderer_helper.cc:124:
IPC_BEGIN_MESSAGE_MAP(AutomationRendererHelper, message)
Please use MAP_EX to avoid a DCHECK on malformed messages. Follow examples from
other automation code.

http://codereview.chromium.org/8294030/diff/2001/chrome/renderer/automation/a...
File chrome/renderer/automation/automation_renderer_helper_browsertest.cc
(right):

http://codereview.chromium.org/8294030/diff/2001/chrome/renderer/automation/a...
chrome/renderer/automation/automation_renderer_helper_browsertest.cc:32:
file_util::WriteFile(FilePath("snapshot" + reference_md5 + ".png"),
Check return value.

http://codereview.chromium.org/8294030/diff/2001/chrome/test/automation/autom...
File chrome/test/automation/automation_proxy_uitest.cc (left):

http://codereview.chromium.org/8294030/diff/2001/chrome/test/automation/autom...
chrome/test/automation/automation_proxy_uitest.cc:1516: class
AutomationProxySnapshotTest : public UITest {
The native testing environment for that code are UITest. The browser tests you
added do not fully exercise the implementation. Why are you removing those UI
tests?

[kkania, 2011-10-18 18:30:19]

jamesr: chrome/renderer/automation/*
phajdan-jr: all else

http://codereview.chromium.org/8294030/diff/2001/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/8294030/diff/2001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.cc:1911: overall_success
= bytes_written == static_cast<int>(png_data.size());
On 2011/10/18 10:21:35, Paweł Hajdan Jr. wrote:
> nit: Parentheses around bytes_written == ... please.

Done.

http://codereview.chromium.org/8294030/diff/2001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.cc:1912: if (!success)
On 2011/10/18 10:21:35, Paweł Hajdan Jr. wrote:
> nit: Did you mean overall_success here?

Done.

http://codereview.chromium.org/8294030/diff/2001/chrome/browser/automation/au...
File chrome/browser/automation/automation_tab_helper.h (right):

http://codereview.chromium.org/8294030/diff/2001/chrome/browser/automation/au...
chrome/browser/automation/automation_tab_helper.h:21: class Size;
On 2011/10/18 10:21:35, Paweł Hajdan Jr. wrote:
> nit: Is this needed?

Done.

http://codereview.chromium.org/8294030/diff/2001/chrome/renderer/automation/a...
File chrome/renderer/automation/automation_renderer_helper.cc (right):

http://codereview.chromium.org/8294030/diff/2001/chrome/renderer/automation/a...
chrome/renderer/automation/automation_renderer_helper.cc:41: // We don't want to
change the widget's layout by resizing. Just scroll
On 2011/10/18 10:21:35, Paweł Hajdan Jr. wrote:
> Please get additional reviewer for this WebKit stuff.

Done.

http://codereview.chromium.org/8294030/diff/2001/chrome/renderer/automation/a...
chrome/renderer/automation/automation_renderer_helper.cc:124:
IPC_BEGIN_MESSAGE_MAP(AutomationRendererHelper, message)
On 2011/10/18 10:21:35, Paweł Hajdan Jr. wrote:
> Please use MAP_EX to avoid a DCHECK on malformed messages. Follow examples
from
> other automation code.

Done.

http://codereview.chromium.org/8294030/diff/2001/chrome/renderer/automation/a...
File chrome/renderer/automation/automation_renderer_helper_browsertest.cc
(right):

http://codereview.chromium.org/8294030/diff/2001/chrome/renderer/automation/a...
chrome/renderer/automation/automation_renderer_helper_browsertest.cc:32:
file_util::WriteFile(FilePath("snapshot" + reference_md5 + ".png"),
On 2011/10/18 10:21:35, Paweł Hajdan Jr. wrote:
> Check return value.

Done.

http://codereview.chromium.org/8294030/diff/2001/chrome/test/automation/autom...
File chrome/test/automation/automation_proxy_uitest.cc (left):

http://codereview.chromium.org/8294030/diff/2001/chrome/test/automation/autom...
chrome/test/automation/automation_proxy_uitest.cc:1516: class
AutomationProxySnapshotTest : public UITest {
I was a bit too hasty to remove all these. I do think that it would be best to
push tests as low as possible though. I'll reduce the amount of tests here in
exchange for tests on the browser side.

[jamesr, 2011-10-18 19:03:57]

What sort of content is this intended to run on?

[kkania, 2011-10-18 19:13:46]

On 2011/10/18 19:03:57, jamesr wrote:
> What sort of content is this intended to run on?

It's supposed to work with almost any webpage (not pdf or plugin stuff). On
trunk it seems to have a few problems when gpu accelerated content is present.

[Paweł Hajdan Jr., 2011-10-19 09:03:33]

LGTM with a nit.

http://codereview.chromium.org/8294030/diff/12003/chrome/browser/automation/a...
File chrome/browser/automation/automation_tab_helper.cc (right):

http://codereview.chromium.org/8294030/diff/12003/chrome/browser/automation/a...
chrome/browser/automation/automation_tab_helper.cc:14: 
nit: No empty line here.

[jamesr, 2011-10-20 23:56:40]

Sorry for the delay - I don't think scrolling around the view will really work. 
Have you considered what will happen when there is a position:fixed element in
the page?

[jamesr, 2011-10-20 23:57:57]

On 2011/10/20 23:56:40, jamesr wrote:
> Sorry for the delay - I don't think scrolling around the view will really
work. 
> Have you considered what will happen when there is a position:fixed element in
> the page?

Also how are scrollbars supposed to be rendered?

This might work for a limited set of content but not for "almost any webpage".

[kkania, 2011-10-24 18:17:19]

Switched to using fixed layout before resizing to fit contentsSize.

[kkania, 2011-10-26 20:29:13]

On 2011/10/24 18:17:19, kkania wrote:
> Switched to using fixed layout before resizing to fit contentsSize.

jamesr, ping?

[jamesr, 2011-10-26 20:32:50]

renderer lgtm

You should send tryjobs before landing, but to do that I believe you'll have to
land the binaries (like pngs) separately.

[commit-bot: I haz the power, 2011-10-27 21:35:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkania@chromium.org/8294030/23001

[commit-bot: I haz the power, 2011-10-27 22:50:37]

Change committed as 107645