Compile pdf_browsertest.cc in non-official builds too.

Compile pdf_browsertest.cc in non-official builds too.

This prevents build breakages on internal buildbots.

The actual tests in the file are still disabled, because they depend on the PDF viewer.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243679

[commit-bot: I haz the power, 2014-01-07 15:24:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/120303004/1

[commit-bot: I haz the power, 2014-01-07 15:24:16]

Failed to apply patch for chrome/browser/ui/pdf/pdf_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/pdf/pdf_browsertest.cc
  Hunk #1 succeeded at 31 (offset 1 line).
  Hunk #2 FAILED at 132.
  Hunk #3 succeeded at 215 (offset 1 line).
  Hunk #4 succeeded at 239 (offset 1 line).
  Hunk #5 succeeded at 269 (offset 1 line).
  Hunk #6 succeeded at 307 (offset 1 line).
  Hunk #7 succeeded at 378 (offset 1 line).
  1 out of 7 hunks FAILED -- saving rejects to file
chrome/browser/ui/pdf/pdf_browsertest.cc.rej

Patch:       chrome/browser/ui/pdf/pdf_browsertest.cc
Index: chrome/browser/ui/pdf/pdf_browsertest.cc
diff --git a/chrome/browser/ui/pdf/pdf_browsertest.cc
b/chrome/browser/ui/pdf/pdf_browsertest.cc
index
70e48361eba458324e204ee30985ac80bd54f57e..512beee14e37ede078a0f7c4b686131f80fbf43d
100644
--- a/chrome/browser/ui/pdf/pdf_browsertest.cc
+++ b/chrome/browser/ui/pdf/pdf_browsertest.cc
@@ -30,6 +30,9 @@
 using content::NavigationController;
 using content::WebContents;
 
+// Note: All tests in here require the internal PDF plugin, so they're disabled
+// in non-official builds. We still compile them though, to prevent bitrot.
+
 namespace {
 
 // Include things like browser frame and scrollbar and make sure we're bigger
@@ -129,7 +132,7 @@ class PDFBrowserTest : public InProcessBrowserTest,
     base::FilePath reference = ui_test_utils::GetTestFilePath(
         GetPDFTestDir(),
         base::FilePath().AppendASCII(expected_filename_));
-    base::PlatformFileInfo info;
+    base::File::Info info;
     ASSERT_TRUE(base::GetFileInfo(reference, &info));
     int size = static_cast<size_t>(info.size);
     scoped_ptr<char[]> data(new char[size]);
@@ -211,7 +214,7 @@ class PDFBrowserTest : public InProcessBrowserTest,
   net::test_server::EmbeddedTestServer pdf_test_server_;
 };
 
-#if defined(OS_CHROMEOS)
+#if !defined(GOOGLE_CHROME_BUILD) || defined(OS_CHROMEOS)
 // TODO(sanjeevr): http://crbug.com/79837
 #define MAYBE_Basic DISABLED_Basic
 #else
@@ -235,7 +238,7 @@ IN_PROC_BROWSER_TEST_F(PDFBrowserTest, MAYBE_Basic) {
 #endif
 }
 
-#if defined(OS_CHROMEOS)
+#if !defined(GOOGLE_CHROME_BUILD) || defined(OS_CHROMEOS)
 // TODO(sanjeevr): http://crbug.com/79837
 #define MAYBE_Scroll DISABLED_Scroll
 #else
@@ -265,7 +268,7 @@ IN_PROC_BROWSER_TEST_F(PDFBrowserTest, MAYBE_Scroll) {
   ASSERT_GT(y_offset, 0);
 }
 
-#if defined(OS_CHROMEOS)
+#if !defined(GOOGLE_CHROME_BUILD) || defined(OS_CHROMEOS)
 // TODO(sanjeevr): http://crbug.com/79837
 #define MAYBE_FindAndCopy DISABLED_FindAndCopy
 #else
@@ -303,7 +306,12 @@ const int kLoadingNumberOfParts = 10;
 // This also loads all documents that used to crash, to ensure we don't have
 // regressions.
 // If it flakes, reopen http://crbug.com/74548.
-IN_PROC_BROWSER_TEST_P(PDFBrowserTest, Loading) {
+#if !defined(GOOGLE_CHROME_BUILD)
+#define MAYBE_Loading DISABLED_Loading
+#else
+#define MAYBE_Loading Loading
+#endif
+IN_PROC_BROWSER_TEST_P(PDFBrowserTest, MAYBE_Loading) {
   ASSERT_TRUE(pdf_test_server()->InitializeAndWaitUntilReady());
 
   NavigationController* controller =
@@ -369,7 +377,7 @@ INSTANTIATE_TEST_CASE_P(PDFTestFiles,
                         PDFBrowserTest,
                         testing::Range(0, kLoadingNumberOfParts));
 
-#if defined(GOOGLE_CHROME_BUILD) && defined(OS_MACOSX)
+#if !defined(GOOGLE_CHROME_BUILD) || defined(OS_MACOSX)
 // http://crbug.com/315160
 #define MAYBE_Action DISABLED_Action
 #else

[Bernhard Bauer, 2014-01-07 15:34:02]

Please review.

[Lei Zhang, 2014-01-07 19:58:36]

lgtm with a nit:

https://codereview.chromium.org/120303004/diff/110001/chrome/browser/ui/pdf/p...
File chrome/browser/ui/pdf/pdf_browsertest.cc (right):

https://codereview.chromium.org/120303004/diff/110001/chrome/browser/ui/pdf/p...
chrome/browser/ui/pdf/pdf_browsertest.cc:219: // TODO(sanjeevr):
http://crbug.com/79837
Would you mind changing the TODO to me since Sanjeev no longer works on
Chromium? Please make it obvious the TODO is for CrOS only. Ditto below.

[jam, 2014-01-07 20:39:39]

It's done like this so that the test doesn't run when the pdf plugin isn't
checked out.

Please don't change this. If we add run time checks instead, then they wouldn't
catch the case where on official builders the pdf plugin got lost for one reason
or another.

[Bernhard Bauer, 2014-01-07 22:05:13]

I don't understand. This shouldn't change the behavior for official builds:
they will still build and run the tests, and fail if the plugin isn't
there. The only difference is that the test is now built (but not run) on
non-official builds.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Bernhard Bauer, 2014-01-08 09:33:24]

Re-adding John, who got dropped for some reason.

https://codereview.chromium.org/120303004/diff/110001/chrome/browser/ui/pdf/p...
File chrome/browser/ui/pdf/pdf_browsertest.cc (right):

https://codereview.chromium.org/120303004/diff/110001/chrome/browser/ui/pdf/p...
chrome/browser/ui/pdf/pdf_browsertest.cc:219: // TODO(sanjeevr):
http://crbug.com/79837
On 2014/01/07 19:58:37, Lei Zhang wrote:
> Would you mind changing the TODO to me since Sanjeev no longer works on
> Chromium? Please make it obvious the TODO is for CrOS only. Ditto below.

Done.

[jam, 2014-01-08 19:32:18]

On 2014/01/07 22:05:13, Bernhard Bauer wrote:
> I don't understand. This shouldn't change the behavior for official builds:
> they will still build and run the tests, and fail if the plugin isn't
> there. The only difference is that the test is now built (but not run) on
> non-official builds.

sorry I missed the changes to the cc file, lgtm

[commit-bot: I haz the power, 2014-01-08 19:39:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/120303004/260001

[commit-bot: I haz the power, 2014-01-08 22:45:46]

Change committed as 243679