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

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: chrome/renderer/chrome_content_renderer_client_unittest.cc

Issue 2228623002: Adding Android specific behavior when overriding YouTube Flash embeds. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@metrics_v2
Patch Set: Addressed comments Created 4 years, 4 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
« no previous file with comments | « chrome/renderer/chrome_content_renderer_client.cc ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
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 f7df73e83a0eea3a4c96f6695c5f4c479cf6aa77..211187bb4a22ec9fd666038a643cde2eeb00e037 100644
--- a/chrome/renderer/chrome_content_renderer_client_unittest.cc
+++ b/chrome/renderer/chrome_content_renderer_client_unittest.cc
@@ -411,7 +411,8 @@ TEST_F(ChromeContentRendererClientTest, ShouldSuppressErrorPage) {
std::vector<GURL>(), GURL::EmptyGURL());
}
-TEST_F(ChromeContentRendererClientTest, RewriteYouTubeFlashEmbed) {
+// These are tests that are common for both Android and desktop browsers.
+TEST_F(ChromeContentRendererClientTest, RewriteEmbedCommon) {
struct TestData {
std::string original;
std::string expected;
@@ -429,14 +430,10 @@ TEST_F(ChromeContentRendererClientTest, RewriteYouTubeFlashEmbed) {
{"http://www.youtube.com/embed/deadbeef", ""},
// URL isn't using Flash, no www
{"http://youtube.com/embed/deadbeef", ""},
- // URL isn't using Flash, has JS API enabled
- {"http://www.youtube.com/embed/deadbeef?enablejsapi=1", ""},
// URL isn't using Flash, invalid parameter construct
{"http://www.youtube.com/embed/deadbeef&start=4", ""},
// URL is using Flash, no www
{"http://youtube.com/v/deadbeef", "http://youtube.com/embed/deadbeef"},
- // URL is using Flash, has JS API enabled
- {"http://www.youtube.com/v/deadbeef?enablejsapi=1", ""},
// URL is using Flash, is valid, https
{"https://www.youtube.com/v/deadbeef",
"https://www.youtube.com/embed/deadbeef"},
@@ -478,8 +475,6 @@ TEST_F(ChromeContentRendererClientTest, RewriteYouTubeFlashEmbed) {
"http://www.youtube-nocookie.com/embed/123/"},
// youtube-nocookie.com, isn't using flash
{"http://www.youtube-nocookie.com/embed/123/", ""},
- // youtube-nocookie.com, has JS API enabled
- {"http://www.youtube-nocookie.com/v/123?enablejsapi=1", ""},
// youtube-nocookie.com, has one parameter
{"http://www.youtube-nocookie.com/v/123?start=foo",
"http://www.youtube-nocookie.com/embed/123?start=foo"},
@@ -505,6 +500,63 @@ TEST_F(ChromeContentRendererClientTest, RewriteYouTubeFlashEmbed) {
client.OverrideFlashEmbedWithHTML(GURL(data.original)));
}
+#if defined(OS_ANDROID)
+TEST_F(ChromeContentRendererClientTest, RewriteEmbedAndroid) {
+ struct TestData {
+ std::string original;
+ std::string expected;
+ } test_data[] = {
+ // URL isn't using Flash, has JS API enabled
+ {"http://www.youtube.com/embed/deadbeef?enablejsapi=1", ""},
+ // URL is using Flash, has JS API enabled
+ {"http://www.youtube.com/v/deadbeef?enablejsapi=1",
+ "http://www.youtube.com/embed/deadbeef?enablejsapi=1"},
+ // youtube-nocookie.com, has JS API enabled
+ {"http://www.youtube-nocookie.com/v/123?enablejsapi=1",
+ "http://www.youtube-nocookie.com/embed/123?enablejsapi=1"},
+ // URL is using Flash, has JS API enabled, invalid parameter construct
+ {"http://www.youtube.com/v/deadbeef&enablejsapi=1",
+ "http://www.youtube.com/embed/deadbeef?enablejsapi=1"},
+ // URL is using Flash, has JS API enabled, invalid parameter construct,
+ // has multiple parameters
+ {"http://www.youtube.com/v/deadbeef&start=4&enablejsapi=1",
+ "http://www.youtube.com/embed/deadbeef?start=4&enablejsapi=1"},
+ };
+
+ ChromeContentRendererClient client;
+
+ for (const auto& data : test_data)
+ EXPECT_EQ(GURL(data.expected),
+ client.OverrideFlashEmbedWithHTML(GURL(data.original)));
mlamouri (slow - plz ping) 2016/08/09 11:12:38 style: this requires { }
kdsilva 2016/08/09 12:43:55 Done.
+}
+
mlamouri (slow - plz ping) 2016/08/09 11:12:39 nit: you could remove this empty line.
kdsilva 2016/08/09 12:43:55 Done.
+#else
+TEST_F(ChromeContentRendererClientTest, RewriteEmbedDesktop) {
+ struct TestData {
+ std::string original;
+ std::string expected;
+ } test_data[] = {
+ // URL isn't using Flash, has JS API enabled
+ {"http://www.youtube.com/embed/deadbeef?enablejsapi=1", ""},
+ // URL is using Flash, has JS API enabled
+ {"http://www.youtube.com/v/deadbeef?enablejsapi=1", ""},
+ // youtube-nocookie.com, has JS API enabled
+ {"http://www.youtube-nocookie.com/v/123?enablejsapi=1", ""},
+ // URL is using Flash, has JS API enabled, invalid parameter construct
+ {"http://www.youtube.com/v/deadbeef&enablejsapi=1", ""},
+ // URL is using Flash, has JS API enabled, invalid parameter construct,
+ // has multiple parameters
+ {"http://www.youtube.com/v/deadbeef&start=4&enablejsapi=1", ""},
+ };
+
+ ChromeContentRendererClient client;
+
+ for (const auto& data : test_data)
+ EXPECT_EQ(GURL(data.expected),
+ client.OverrideFlashEmbedWithHTML(GURL(data.original)));
mlamouri (slow - plz ping) 2016/08/09 11:12:38 style: this requires { }
kdsilva 2016/08/09 12:43:55 Done.
+}
+#endif
+
class ChromeContentRendererClientMetricsTest : public testing::Test {
public:
ChromeContentRendererClientMetricsTest() = default;
@@ -621,7 +673,38 @@ TEST_F(ChromeContentRendererClientMetricsTest, RewriteEmbedSuccessRewrite) {
EXPECT_EQ(total_count, samples->TotalCount());
}
-TEST_F(ChromeContentRendererClientMetricsTest, RewriteEmbedFailureJSAPI) {
+#if defined(OS_ANDROID)
+TEST_F(ChromeContentRendererClientMetricsTest,
+ RewriteEmbedFailureJSAPIAndroid) {
+ ChromeContentRendererClient client;
+
+ std::unique_ptr<base::HistogramSamples> samples = GetHistogramSamples();
+ auto total_count = 0;
+ EXPECT_EQ(total_count, samples->TotalCount());
+
+ const std::string test_data[] = {
+ // Valid parameter construct, one parameter
+ "http://www.youtube.com/v/deadbeef?enablejsapi=1",
+ // Valid parameter construct, has multiple parameters
+ "http://www.youtube.com/v/deadbeef?enablejsapi=1&foo=2",
+ // Invalid parameter construct, one parameter
+ "http://www.youtube.com/v/deadbeef&enablejsapi=1",
+ // Invalid parameter construct, has multiple parameters
+ "http://www.youtube.com/v/deadbeef&enablejsapi=1&foo=2"};
+
+ for (const auto& data : test_data) {
+ ++total_count;
+ GURL gurl = GURL(data);
+ OverrideFlashEmbed(gurl);
+ samples = GetHistogramSamples();
+ EXPECT_EQ(total_count, samples->GetCount(internal::SUCCESS_ENABLEJSAPI));
+ EXPECT_EQ(total_count, samples->TotalCount());
+ }
+}
+
mlamouri (slow - plz ping) 2016/08/09 11:12:38 ditto
kdsilva 2016/08/09 12:43:55 Done.
+#else
+TEST_F(ChromeContentRendererClientMetricsTest,
+ RewriteEmbedFailureJSAPIDesktop) {
ChromeContentRendererClient client;
std::unique_ptr<base::HistogramSamples> samples = GetHistogramSamples();
@@ -629,6 +712,8 @@ TEST_F(ChromeContentRendererClientMetricsTest, RewriteEmbedFailureJSAPI) {
EXPECT_EQ(total_count, samples->TotalCount());
const std::string test_data[] = {
+ // Valid parameter construct, one parameter
+ "http://www.youtube.com/v/deadbeef?enablejsapi=1",
// Invalid parameter construct, one parameter
"http://www.youtube.com/v/deadbeef&enablejsapi=1",
// Invalid parameter construct, has multiple parameters
@@ -643,3 +728,5 @@ TEST_F(ChromeContentRendererClientMetricsTest, RewriteEmbedFailureJSAPI) {
EXPECT_EQ(total_count, samples->TotalCount());
}
}
+
mlamouri (slow - plz ping) 2016/08/09 11:12:38 nit: remove this empty line
kdsilva 2016/08/09 12:43:55 Done.
+#endif
« no previous file with comments | « chrome/renderer/chrome_content_renderer_client.cc ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698