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) {

[mlamouri (slow - plz ping), 2016/07/28 13:05:13]

No need to have a Test suffix in RewriteYouTubeFlashEmbed.

[kdsilva, 2016/07/28 15:17:17]

Done.

+  struct TestData {
+    std::string original;
+    std::string expected;
+  } test_data[] = {
+    { "youtube.com", "" }, // original, expected

[mlamouri (slow - plz ping), 2016/07/28 13:05:13]

Can you move the "// original, expected" above and maybe have "// { original,
expected }"

[kdsilva, 2016/07/28 15:17:17]

Done.

+    { "www.youtube.com", "" },
+    { "http://www.youtube.com", "" },
+    { "https://www.youtube.com", "" },
+    { "http://www.foo.youtube.com", "" },
+    { "https://www.foo.youtube.com", "" },
+    // Non-YouTube domains shouldn't be modified
+    { "http://www.plus.google.com", "" },
+    // URL isn't using Flash
+    {"http://www.youtube.com/embed/cW44BpXpjYw", ""},

[mlamouri (slow - plz ping), 2016/07/28 13:05:13]

Starting from here, you seem to no longer have spaces after { and before }.

[kdsilva, 2016/07/28 15:17:17]

Done.

+    // URL isn't using Flash, no www
+    {"youtube.com/embed/cW44BpXpjYw", ""},

[mlamouri (slow - plz ping), 2016/07/28 13:05:13]

No "www" indeed but you forgot the "http://" which makes the URL entirely
invalid.

[kdsilva, 2016/07/28 15:17:17]

Done.

+    // URL isn't using Flash, has JS API enabled
+    {"http://www.youtube.com/embed/cW44BpXpjYw?enablejsapi=1"},

[mlamouri (slow - plz ping), 2016/07/28 13:05:13]

You forgot the expected value.

[kdsilva, 2016/07/28 15:17:17]

Done.

+    // URL isn't using Flash, is invalid
+    {"http://www.youtube.com/embed/cW44BpXpjYw&start=4", ""},
+    // URL is using Flash, has JS API enabled
+    {"http://www.youtube.com/v/cW44BpXpjYw?enablejsapi=1", ""},
+    // URL is using Flash, is valid, https
+    {"https://www.youtube.com/v/cW44BpXpjYw",
+      "https://www.youtube.com/embed/cW44BpXpjYw"},

[mlamouri (slow - plz ping), 2016/07/28 13:05:13]

style: here and below, you have one space too many. The two strings should be
aligned.

[kdsilva, 2016/07/28 15:17:17]

Done.

+     // URL is using Flash, is valid, http
+    {"http://www.youtube.com/v/cW44BpXpjYw",
+      "http://www.youtube.com/embed/cW44BpXpjYw"},
+    // URL is using Flash, is valid, not a complete URL, no www or protocol
+    {"youtube.com/v/cW44BpXpjYw", ""},
+    // URL is using Flash, is valid, not a complete URL,or protocol
+    {"www.youtube.com/v/cW44BpXpjYw", ""},
+    // URL is using Flash, valid
+    {"https://www.foo.youtube.com/v/cW44BpXpjYw",
+      "https://www.foo.youtube.com/embed/cW44BpXpjYw"},
+    // URL is using Flash, is valid, has one parameter
+    {"http://www.youtube.com/v/cW44BpXpjYw?start=4",
+      "http://www.youtube.com/embed/cW44BpXpjYw?start=4"},
+    // URL is using Flash, is valid, has multiple parameters
+    {"http://www.youtube.com/v/cW44BpXpjYw?start=4&fs=1",
+      "http://www.youtube.com/embed/cW44BpXpjYw?start=4&fs=1"},
+    // URL is using Flash, is invalid, has one parameter
+    {"http://www.youtube.com/v/cW44BpXpjYw&start=4",
+      "http://www.youtube.com/embed/cW44BpXpjYw?start=4"},
+    // URL is using Flash, is invalid, has multiple parameters
+    {"http://www.youtube.com/v/cW44BpXpjYw&start=4&fs=1?foo=bar",
+      "http://www.youtube.com/embed/cW44BpXpjYw?start=4&fs=1&foo=bar"},
+    // URL is using Flash, is invalid, has multiple parameters
+    {"http://www.youtube.com/v/cW44BpXpjYw&start=4&fs=1",
+      "http://www.youtube.com/embed/cW44BpXpjYw?start=4&fs=1"}

[mlamouri (slow - plz ping), 2016/07/28 13:05:13]

Can you have a unit test that succeeds where the URL is
"http://youtube.com/..."?

[kdsilva, 2016/07/28 15:17:17]

Done.

+  };

[mlamouri (slow - plz ping), 2016/07/28 13:05:13]

Can you add a test with the URL being something like:
- https://www.youtube.com/foo/v/ABCDEF (should fail)
- http://youtube.com/v/a/ (should succeed)
- http://youtube.com/v/123/ (should suceed)

[kdsilva, 2016/07/28 15:17:17]

Done.

+
+  ChromeContentRendererClient client;
+
+  for (auto data : test_data) {
+    EXPECT_EQ(data.expected, client.OverrideFlashEmbedWithHTML(data.original));
+  }

[mlamouri (slow - plz ping), 2016/07/28 13:05:13]

style: the coding style says that we should have { } for one-line statements.

[kdsilva, 2016/07/28 15:17:17]

Done.

+}