Rewrite YouTube Flash embeds

chrome/renderer/chrome_content_renderer_client_unittest.cc

Index: chrome/renderer/chrome_content_renderer_client_unittest.cc
diff --git a/chrome/renderer/chrome_content_renderer_client_unittest.cc b/chrome/renderer/chrome_content_renderer_client_unittest.cc
index 898916fbe7657a8be1d6a079058986f32b62f16a..eb298e5d176948cfa9309ed46ebb408083047dea 100644
--- a/chrome/renderer/chrome_content_renderer_client_unittest.cc
+++ b/chrome/renderer/chrome_content_renderer_client_unittest.cc
@@ -408,3 +408,80 @@ TEST_F(ChromeContentRendererClientTest, ShouldSuppressErrorPage) {
   SearchBouncer::GetInstance()->OnSetSearchURLs(
       std::vector<GURL>(), GURL::EmptyGURL());
 }
+
+TEST_F(ChromeContentRendererClientTest, RewriteYouTubeFlashEmbedTest) {
+  ChromeContentRendererClient client;
+
+  std::string notOverriden;
+  std::string expected;
+
+  std::string original = "";
+  EXPECT_EQ(notOverriden, client.OverrideFlashEmbedWithHTML(original));
+
+  original = "youtube.com";
+  EXPECT_EQ(notOverriden, client.OverrideFlashEmbedWithHTML(original));

[mlamouri (slow - plz ping), 2016/07/21 15:55:07]

I think always defining "expected" will help to read because one can quickly
look at "original" and "expected". It adds some cognitive overhead to check the
EXPECT_EQ() to check what's in there.

[kdsilva, 2016/07/22 13:55:58]

Acknowledged.

+
+  original = "http://www.youtube.com";
+  EXPECT_EQ(notOverriden, client.OverrideFlashEmbedWithHTML(original));
+
+  original = "https://www.youtube.com";
+  EXPECT_EQ(notOverriden, client.OverrideFlashEmbedWithHTML(original));
+
+  original = "https://www.foo.youtube.com";

[mlamouri (slow - plz ping), 2016/07/21 15:55:07]

You are testing "www.youtube.com", "youtube.com", "https://...",
"www.foo.youtube.com" on URLs that are not rewritten (they don't match "/v/"
requirement). You should do the same on the ones that are matching the
requirements IMO.

[kdsilva, 2016/07/22 13:55:58]

Done. Although something like "youtube.com/v/VIDEO_ID" wouldn't be considered
valid since we assume we're receiving a complete URL.

+  EXPECT_EQ(notOverriden, client.OverrideFlashEmbedWithHTML(original));
+
+  // Non-YouTube domains shouldn't be modified
+  original = "http://www.plus.google.com";
+  EXPECT_EQ(notOverriden, client.OverrideFlashEmbedWithHTML(original));
+
+  // URL isn't using Flash
+  original = "http://www.youtube.com/embed/cW44BpXpjYw";
+  EXPECT_EQ(notOverriden, client.OverrideFlashEmbedWithHTML(original));
+
+  // URL isn't using Flash, no www
+  original = "youtube.com/embed/cW44BpXpjYw";
+  EXPECT_EQ(notOverriden, client.OverrideFlashEmbedWithHTML(original));
+
+  // URL isn't using Flash, has JS API enabled
+  original = "http://www.youtube.com/embed/cW44BpXpjYw?enablejsapi=1";
+  EXPECT_EQ(notOverriden, client.OverrideFlashEmbedWithHTML(original));
+
+  // URL isn't using Flash, is invalid
+  original = "http://www.youtube.com/embed/cW44BpXpjYw&start=4";
+  EXPECT_EQ(notOverriden, client.OverrideFlashEmbedWithHTML(original));
+
+  // URL is using Flash, has JS API enabled
+  original = "http://www.youtube.com/v/cW44BpXpjYw?enablejsapi=1";
+  EXPECT_EQ(notOverriden, client.OverrideFlashEmbedWithHTML(original));
+
+  // URL is using Flash, is valid
+  original = "https://www.youtube.com/v/cW44BpXpjYw";
+  expected = "https://www.youtube.com/embed/cW44BpXpjYw";
+  EXPECT_EQ(expected, client.OverrideFlashEmbedWithHTML(original));
+
+  // URL is using Flash, is valid, has one parameter
+  original = "http://www.youtube.com/v/cW44BpXpjYw?start=4";
+  expected = "http://www.youtube.com/embed/cW44BpXpjYw?start=4";
+  EXPECT_EQ(expected, client.OverrideFlashEmbedWithHTML(original));
+
+  // URL is using Flash, is valid, has multiple parameters
+  original = "http://www.youtube.com/v/cW44BpXpjYw?start=4&fs=1";
+  expected = "http://www.youtube.com/embed/cW44BpXpjYw?start=4&fs=1";
+  EXPECT_EQ(expected, client.OverrideFlashEmbedWithHTML(original));
+
+  // URL is using Flash, is invalid, has one parameter
+  original = "http://www.youtube.com/v/cW44BpXpjYw&start=4";
+  expected = "http://www.youtube.com/embed/cW44BpXpjYw?start=4";
+  EXPECT_EQ(expected, client.OverrideFlashEmbedWithHTML(original));
+
+  // URL is using Flash, is invalid, has one parameter
+  original = "http://www.youtube.com/v/cW44BpXpjYw&start=4&fs=1?foo=bar";
+  expected = "http://www.youtube.com/embed/cW44BpXpjYw?start=4&fs=1&foo=bar";
+  EXPECT_EQ(expected, client.OverrideFlashEmbedWithHTML(original));
+
+  // URL is using Flash, is invalid, has multiple parameters
+  original = "http://www.youtube.com/v/cW44BpXpjYw&start=4&fs=1";
+  expected = "http://www.youtube.com/embed/cW44BpXpjYw?start=4&fs=1";
+  EXPECT_EQ(expected, client.OverrideFlashEmbedWithHTML(original));
+}

[mlamouri (slow - plz ping), 2016/07/21 15:55:07]

Something you can do to improve readability is:
```
struct TestData {
  std::string original;
  std::string expected;
};

std::vector<TestData> test_data = {
  { "https://youtube.com", "" }, // original / exppected
};

for (auto data : test_data)
  EXPECT_EQ(data.expected, client.OverrideFlashEmbedWithHTML(data.original);
```

[kdsilva, 2016/07/22 13:55:57]

Done.

+