Rewrite YouTube Flash embeds

chrome/renderer/chrome_content_renderer_client_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/renderer/chrome_content_renderer_client.h"
 
 #include <stddef.h>
 
 #include <vector>
 
@@ skipping 390 matching lines @@
   ChromeContentRendererClient client;
   SearchBouncer::GetInstance()->OnSetSearchURLs(
       std::vector<GURL>(), GURL("http://example.com/n"));
   EXPECT_FALSE(client.ShouldSuppressErrorPage(nullptr,
                                               GURL("http://example.com")));
   EXPECT_TRUE(client.ShouldSuppressErrorPage(nullptr,
                                              GURL("http://example.com/n")));
   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.

+