Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(10690)

Unified Diff: chrome/renderer/chrome_content_renderer_client_unittest.cc

Issue 2154233003: Rewrite YouTube Flash embeds (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Clean up Created 4 years, 5 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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..cb174ed4a85e4c1d5dd5f1da4a57870712004c64 100644
--- a/chrome/renderer/chrome_content_renderer_client_unittest.cc
+++ b/chrome/renderer/chrome_content_renderer_client_unittest.cc
@@ -408,3 +408,63 @@ TEST_F(ChromeContentRendererClientTest, ShouldSuppressErrorPage) {
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 RewriteYouTubeFla
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
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 spa
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
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
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 U
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 li
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 {
kdsilva 2016/07/28 15:17:17 Done.
+}

Powered by Google App Engine
This is Rietveld 408576698