Add automated tests for PDFs.

Add automated tests for PDFs.


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

[sky, 2010-10-27 23:54:30]

http://codereview.chromium.org/2720002/diff/32001/33008
File chrome/test/plugin/pdf_browsertest.cc (right):

http://codereview.chromium.org/2720002/diff/32001/33008#newcode26
chrome/test/plugin/pdf_browsertest.cc:26: static const int kBrowserWidth = 1000;
If you need this size you should check the screen resolution in your test. I'm
not sure if the bots end up in some configurations that could result in smaller
windows.

http://codereview.chromium.org/2720002/diff/32001/33008#newcode38
chrome/test/plugin/pdf_browsertest.cc:38: void OnQuit() {
Is this used?

http://codereview.chromium.org/2720002/diff/32001/33008#newcode65
chrome/test/plugin/pdf_browsertest.cc:65:
browser()->GetSelectedTabContents()->render_view_host();
nit: 4 space indent.

http://codereview.chromium.org/2720002/diff/32001/33008#newcode66
chrome/test/plugin/pdf_browsertest.cc:66: NotificationRegistrar registrar;
Can you use ui_test_utils::WaitForNotification() here?

http://codereview.chromium.org/2720002/diff/32001/33008#newcode168
chrome/test/plugin/pdf_browsertest.cc:168: MessageLoopForUI::current()->Quit();
Do this first, that way if you assert the test won't time out.

http://codereview.chromium.org/2720002/diff/32001/33008#newcode172
chrome/test/plugin/pdf_browsertest.cc:172: bool have_plugin_;
How about some comment here?

http://codereview.chromium.org/2720002/diff/32001/33008#newcode186
chrome/test/plugin/pdf_browsertest.cc:186: WaitForResponse();
Wrap calls to WaitForResponse and VerifySnapshot in ASSERT_NO_FATAL_FAILURE.
That way if the function you're calling to ASSERTS, you won't execute anymore.

http://codereview.chromium.org/2720002/diff/32001/33008#newcode234
chrome/test/plugin/pdf_browsertest.cc:234: ASSERT_EQ(text, "adipiscing");
expected value should be first.

[Paweł Hajdan Jr., 2010-10-28 08:35:30]

Drive-by with some test comments. No need to wait for another review by me.

http://codereview.chromium.org/2720002/diff/32001/33008
File chrome/test/plugin/pdf_browsertest.cc (right):

http://codereview.chromium.org/2720002/diff/32001/33008#newcode90
chrome/test/plugin/pdf_browsertest.cc:90: ASSERT_TRUE(ui_test_utils::FindInPage(
nit: Could you rather use ASSERT_EQ(0, ...)?

http://codereview.chromium.org/2720002/diff/32001/33008#newcode111
chrome/test/plugin/pdf_browsertest.cc:111: ASSERT_TRUE(size ==
file_util::ReadFile(reference, data.get(), size));
nit: Same here.

http://codereview.chromium.org/2720002/diff/32001/33008#newcode216
chrome/test/plugin/pdf_browsertest.cc:216:
ASSERT_TRUE(ui_test_utils::FindInPage(
nit: and here.

[jam, 2010-10-28 18:43:41]

updated per comments

[sky, 2010-10-28 19:22:10]

LGTM