Allow macros in parametrized browser tests (IN_PROCESS_BROWSER_TEST_P).

Allow macros in parametrized browser tests (IN_PROCESS_BROWSER_TEST_P).

Adding one additional #define layer correctly expands the macro arguments. This
allows the MAYBE_ prefix trick to conditionally disable tests on some platforms.

Then, with this newly-acquired ability, fix encrypted_media_browsertest.cc to use the right style for disabling tests.

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

[vabr (Chromium), 2013-02-05 16:31:43]

Nice! Thanks, LGTM.

[Bernhard Bauer, 2013-02-05 18:22:17]

Paweł, Shadi: Please review.

[shadi, 2013-02-05 18:55:18]

Why are we changing encrypted media tests with this CL? 

For allowing _Maybe in parametrized tests see
https://code.google.com/p/googletest/issues/detail?id=389, this is definitely
needed but let's not relate it to encrypted media tests.

[vabr (Chromium), 2013-02-05 19:01:37]

On 2013/02/05 18:55:18, shadi wrote:
> Why are we changing encrypted media tests with this CL? 
> 
> For allowing _Maybe in parametrized tests see
> https://code.google.com/p/googletest/issues/detail?id=389, this is definitely
> needed but let's not relate it to encrypted media tests.

That's my fault, that I did not wait for Bernhard's fix and put the disabling
in. Given the nice progress on
https://code.google.com/p/chromium/issues/detail?id=174294, we could even let
the clumsy disabling of mine there, as it would be soon removed after the issue
with WebKit is solved.
WDYT, Bernhard?

[Bernhard Bauer, 2013-02-05 19:07:19]

On 2013/02/05 18:55:18, shadi wrote:
> Why are we changing encrypted media tests with this CL? 
> 
> For allowing _Maybe in parametrized tests see
> https://code.google.com/p/googletest/issues/detail?id=389, this is definitely
> needed but let's not relate it to encrypted media tests.

Well, the encrypted media tests were using the wrong disabling style, so I fixed
them. I'm not changing anything about on which platforms they are disabled, just
the syntax for it. I don't know whether that currently applies on trunk though.
If the tests have become re-enabled in the mean time, I'll remove the
encrypted_media_browsertest.cc changes, otherwise I'd like to keep them in, as
MAYBE_ is the preferred style.

[shadi, 2013-02-05 19:13:20]

On 2013/02/05 19:07:19, Bernhard Bauer wrote:
> On 2013/02/05 18:55:18, shadi wrote:
> > Why are we changing encrypted media tests with this CL? 
> > 
> > For allowing _Maybe in parametrized tests see
> > https://code.google.com/p/googletest/issues/detail?id=389, this is
definitely
> > needed but let's not relate it to encrypted media tests.
> 
> Well, the encrypted media tests were using the wrong disabling style, so I
fixed
> them. I'm not changing anything about on which platforms they are disabled,
just
> the syntax for it. I don't know whether that currently applies on trunk
though.
> If the tests have become re-enabled in the mean time, I'll remove the
> encrypted_media_browsertest.cc changes, otherwise I'd like to keep them in, as
> MAYBE_ is the preferred style.

Alright,  this can also give an example of why we are doing this change to the
macros. Plus the tests are not reenabled afaik.

LGTM.

[Paweł Hajdan Jr., 2013-02-06 08:29:57]

LGTM

[commit-bot: I haz the power, 2013-02-06 09:24:24]

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

[commit-bot: I haz the power, 2013-02-06 09:24:25]

Failed to apply patch for content/browser/media/encrypted_media_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/media/encrypted_media_browsertest.cc
  Hunk #1 FAILED at 126.
  Hunk #2 FAILED at 139.
  Hunk #3 FAILED at 153.
  Hunk #4 FAILED at 166.
  Hunk #5 FAILED at 179.
  5 out of 5 hunks FAILED -- saving rejects to file
content/browser/media/encrypted_media_browsertest.cc.rej

Patch:       content/browser/media/encrypted_media_browsertest.cc
Index: content/browser/media/encrypted_media_browsertest.cc
diff --git a/content/browser/media/encrypted_media_browsertest.cc
b/content/browser/media/encrypted_media_browsertest.cc
index
77e15a0c72cb092d2d9413e07bd2f20aa6cfe731..f9055710e000e0e38d0c044b249a33a1c79d5432
100644
--- a/content/browser/media/encrypted_media_browsertest.cc
+++ b/content/browser/media/encrypted_media_browsertest.cc
@@ -126,12 +126,11 @@ IN_PROC_BROWSER_TEST_F(EncryptedMediaTest,
InvalidKeySystem) {
 }
 
 #if defined(OS_LINUX) && !defined(NDEBUG)
-// http://crbug.com/174294; not using MAYBE_ because IN_PROC_BROWSER_TEST_P
does
-// not expand the test name macro correctly, TODO(vabr, bauerb)
-IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, DISABLED_BasicPlayback_AudioOnly) {
+#define MAYBE_BasicPlayback_AudioOnly DISABLED_BasicPlayback_AudioOnly
 #else
-IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, BasicPlayback_AudioOnly) {
+#define MAYBE_BasicPlayback_AudioOnly BasicPlayback_AudioOnly
 #endif
+IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, MAYBE_BasicPlayback_AudioOnly) {
   const string16 kExpected = ASCIIToUTF16("ENDED");
   ASSERT_NO_FATAL_FAILURE(
       TestSimplePlayback("bear-a-enc_a.webm", kWebMAudioOnly,
@@ -139,13 +138,13 @@ IN_PROC_BROWSER_TEST_P(EncryptedMediaTest,
BasicPlayback_AudioOnly) {
 }
 
 #if defined(OS_LINUX) && !defined(NDEBUG)
-// http://crbug.com/174294; not using MAYBE_ because IN_PROC_BROWSER_TEST_P
does
-// not expand the test name macro correctly, TODO(vabr, bauerb)
-IN_PROC_BROWSER_TEST_P(EncryptedMediaTest,
-                       DISABLED_BasicPlayback_AudioClearVideo) {
+#define MAYBE_BasicPlayback_AudioClearVideo\
+  DISABLED_BasicPlayback_AudioClearVideo
 #else
-IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, BasicPlayback_AudioClearVideo) {
+#define MAYBE_BasicPlayback_AudioClearVideo BasicPlayback_AudioClearVideo
 #endif
+IN_PROC_BROWSER_TEST_P(EncryptedMediaTest,
+                       MAYBE_BasicPlayback_AudioClearVideo) {
   const string16 kExpected = ASCIIToUTF16("ENDED");
   ASSERT_NO_FATAL_FAILURE(
       TestSimplePlayback("bear-320x240-av-enc_a.webm", kWebMAudioVideo,
@@ -153,12 +152,11 @@ IN_PROC_BROWSER_TEST_P(EncryptedMediaTest,
BasicPlayback_AudioClearVideo) {
 }
 
 #if defined(OS_LINUX) && !defined(NDEBUG)
-// http://crbug.com/174294; not using MAYBE_ because IN_PROC_BROWSER_TEST_P
does
-// not expand the test name macro correctly, TODO(vabr, bauerb)
-IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, DISABLED_BasicPlayback_VideoAudio) {
+#define MAYBE_BasicPlayback_VideoAudio DISABLED_BasicPlayback_VideoAudio
 #else
-IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, BasicPlayback_VideoAudio) {
+#define MAYBE_BasicPlayback_VideoAudio BasicPlayback_VideoAudio
 #endif
+IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, MAYBE_BasicPlayback_VideoAudio) {
   const string16 kExpected = ASCIIToUTF16("ENDED");
   ASSERT_NO_FATAL_FAILURE(
       TestSimplePlayback("bear-320x240-av-enc_av.webm", kWebMAudioVideo,
@@ -166,12 +164,11 @@ IN_PROC_BROWSER_TEST_P(EncryptedMediaTest,
BasicPlayback_VideoAudio) {
 }
 
 #if defined(OS_LINUX) && !defined(NDEBUG)
-// http://crbug.com/174294; not using MAYBE_ because IN_PROC_BROWSER_TEST_P
does
-// not expand the test name macro correctly, TODO(vabr, bauerb)
-IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, DISABLED_BasicPlayback_VideoOnly) {
+#define MAYBE_BasicPlayback_VideoOnly DISABLED_BasicPlayback_VideoOnly
 #else
-IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, BasicPlayback_VideoOnly) {
+#define MAYBE_BasicPlayback_VideoOnly BasicPlayback_VideoOnly
 #endif
+IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, MAYBE_BasicPlayback_VideoOnly) {
   const string16 kExpected = ASCIIToUTF16("ENDED");
   ASSERT_NO_FATAL_FAILURE(
       TestSimplePlayback("bear-320x240-v-enc_v.webm", kWebMVideoOnly,
@@ -179,13 +176,13 @@ IN_PROC_BROWSER_TEST_P(EncryptedMediaTest,
BasicPlayback_VideoOnly) {
 }
 
 #if defined(OS_LINUX) && !defined(NDEBUG)
-// http://crbug.com/174294; not using MAYBE_ because IN_PROC_BROWSER_TEST_P
does
-// not expand the test name macro correctly, TODO(vabr, bauerb)
-IN_PROC_BROWSER_TEST_P(EncryptedMediaTest,
-                       DISABLED_BasicPlayback_VideoClearAudio) {
+#define MAYBE_BasicPlayback_VideoClearAudio\
+  DISABLED_BasicPlayback_VideoClearAudio
 #else
-IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, BasicPlayback_VideoClearAudio) {
+#define MAYBE_BasicPlayback_VideoClearAudio BasicPlayback_VideoClearAudio
 #endif
+IN_PROC_BROWSER_TEST_P(EncryptedMediaTest,
+                       MAYBE_BasicPlayback_VideoClearAudio) {
   const string16 kExpected = ASCIIToUTF16("ENDED");
   ASSERT_NO_FATAL_FAILURE(
       TestSimplePlayback("bear-320x240-av-enc_v.webm", kWebMAudioVideo,

[commit-bot: I haz the power, 2013-02-06 09:45:43]

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

[commit-bot: I haz the power, 2013-02-06 15:46:37]

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

[commit-bot: I haz the power, 2013-02-06 16:05:54]

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

[commit-bot: I haz the power, 2013-02-06 16:29:28]

Change committed as 180984