Bug 1258053 - Convert YouTube Flash embed URLs that contain params in the path components. r=kmachulis a=lizzard
authorMasatoshi Kimura <VYV03354@nifty.ne.jp>
Thu, 07 Jul 2016 01:17:40 +0900
changeset 339852 bf67cbd8360a4249d6bfa0d2c256ec4291e10fc3
parent 339851 8d664d53f812c239fd6028e4eb32a7ad692a5436
child 339853 a1df906402356101da3b147683aaeb7f76b28bc7
push id6249
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 13:59:36 +0000
treeherdermozilla-beta@bad9d4f5bf7e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmachulis, lizzard
bugs1258053
milestone49.0a2
Bug 1258053 - Convert YouTube Flash embed URLs that contain params in the path components. r=kmachulis a=lizzard
dom/base/nsObjectLoadingContent.cpp
dom/base/test/mochitest.ini
dom/base/test/test_bug1240471.html
dom/base/test/test_youtube_flash_embed.html
--- a/dom/base/nsObjectLoadingContent.cpp
+++ b/dom/base/nsObjectLoadingContent.cpp
@@ -1527,52 +1527,49 @@ nsObjectLoadingContent::MaybeRewriteYout
   // See if requester is planning on using the JS API.
   nsAutoCString uri;
   aURI->GetSpec(uri);
   if (uri.Find("enablejsapi=1", true, 0, -1) != kNotFound) {
     Telemetry::Accumulate(Telemetry::YOUTUBE_NONREWRITABLE_EMBED_SEEN, 1);
     return;
   }
 
-  // Some youtube urls have invalid query strings attached, e.g.
+  // Some YouTube urls have parameters in path components, e.g.
   // http://youtube.com/embed/7LcUOEP7Brc&start=35. These URLs work with flash,
   // but break iframe/object embedding. If this situation occurs with rewritten
-  // URLs, and the user has flash installed, just use flash. If the user does
-  // not have flash installed or activated, chop off the query in order to make
-  // the video load correctly as an iframe. In either case, warn about it in the
+  // URLs, convert the parameters to query in order to make the video load
+  // correctly as an iframe. In either case, warn about it in the
   // developer console.
   int32_t ampIndex = uri.FindChar('&', 0);
-  bool trimQuery = false;
+  bool replaceQuery = false;
   if (ampIndex != -1) {
     int32_t qmIndex = uri.FindChar('?', 0);
     if (qmIndex == -1 ||
         qmIndex > ampIndex) {
-      if (!nsContentUtils::IsSWFPlayerEnabled()) {
-        trimQuery = true;
-      } else {
-        // Flash is enabled, just use it in this case.
-        return;
-      }
+      replaceQuery = true;
     }
   }
 
   // If we've made it this far, we've got a rewritable embed. Log it in
   // telemetry.
   Telemetry::Accumulate(Telemetry::YOUTUBE_REWRITABLE_EMBED_SEEN, 1);
 
   // If we're pref'd off, return after telemetry has been logged.
   if (!Preferences::GetBool(kPrefYoutubeRewrite)) {
     return;
   }
 
   nsAutoString utf16OldURI = NS_ConvertUTF8toUTF16(uri);
-  // If we need to trim the query off the URL, it means it's malformed, and an
-  // ampersand comes first. Use the index we found earlier.
-  if (trimQuery) {
-    uri.Truncate(ampIndex);
+  // If we need to convert the URL, it means an ampersand comes first.
+  // Use the index we found earlier.
+  if (replaceQuery) {
+    // Replace question marks with ampersands.
+    uri.ReplaceChar('?', '&');
+    // Replace the first ampersand with a question mark.
+    uri.SetCharAt('?', ampIndex);
   }
   // Switch out video access url formats, which should possibly allow HTML5
   // video loading.
   uri.ReplaceSubstring(NS_LITERAL_CSTRING("/v/"),
                        NS_LITERAL_CSTRING("/embed/"));
   nsAutoString utf16URI = NS_ConvertUTF8toUTF16(uri);
   nsresult rv = nsContentUtils::NewURIWithDocumentCharset(aOutURI,
                                                           utf16URI,
@@ -1580,17 +1577,17 @@ nsObjectLoadingContent::MaybeRewriteYout
                                                           aBaseURI);
   if (NS_FAILED(rv)) {
     return;
   }
   const char16_t* params[] = { utf16OldURI.get(), utf16URI.get() };
   const char* msgName;
   // If there's no query to rewrite, just notify in the developer console
   // that we're changing the embed.
-  if (!trimQuery) {
+  if (!replaceQuery) {
     msgName = "RewriteYoutubeEmbed";
   } else {
     msgName = "RewriteYoutubeEmbedInvalidQuery";
   }
   nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
                                   NS_LITERAL_CSTRING("Plugins"),
                                   thisContent->OwnerDoc(),
                                   nsContentUtils::eDOM_PROPERTIES,
--- a/dom/base/test/mochitest.ini
+++ b/dom/base/test/mochitest.ini
@@ -663,17 +663,16 @@ skip-if = buildapp == 'b2g' || toolkit =
 skip-if = buildapp == 'b2g'
 [test_bug1126851.html]
 skip-if = buildapp == 'b2g'
 [test_bug1163743.html]
 [test_bug1165501.html]
 [test_bug1187157.html]
 [test_bug1198095.html]
 [test_bug1238440.html]
-[test_bug1240471.html]
 [test_bug1250148.html]
 [test_bug1259588.html]
 [test_bug1263696.html]
 [test_caretPositionFromPoint.html]
 [test_change_policy.html]
 skip-if = buildapp == 'b2g' #no ssl support
 [test_classList.html]
 [test_clearTimeoutIntervalNoArg.html]
@@ -914,8 +913,9 @@ skip-if = buildapp == 'b2g' || (android_
 support-files = test_XHR_timeout.js
 [test_xhr_withCredentials.html]
 [test_XHRDocURI.html]
 [test_XHRResponseURL.html]
 [test_XHRSendData.html]
 [test_console_binding.html]
 [test_unknown_url_origin.html]
 [test_console_proto.html]
+[test_youtube_flash_embed.html]
rename from dom/base/test/test_bug1240471.html
rename to dom/base/test/test_youtube_flash_embed.html
--- a/dom/base/test/test_bug1240471.html
+++ b/dom/base/test/test_youtube_flash_embed.html
@@ -7,28 +7,27 @@
     <meta charset="utf-8">
     <title>Test for Bug 1240471</title>
     <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
     <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
     <script type="application/javascript">
      SimpleTest.waitForExplicitFinish();
      "use strict";
      function onLoad () {
-       let youtube_changed_url_noquery = "https://mochitest.youtube.com/embed/Xm5i5kbIXzc";
        let youtube_changed_url_query = "https://mochitest.youtube.com/embed/Xm5i5kbIXzc?start=10&end=20";
 
        function testEmbed(embed, expected_url) {
          ok (embed, "Embed node exists");
          embed = SpecialPowers.wrap(embed);
          is (embed.srcURI.spec, expected_url, "Should have src uri of " + expected_url);
        }
        info("Running youtube rewrite query test");
        testEmbed(document.getElementById("testembed-correct"), youtube_changed_url_query);
-       testEmbed(document.getElementById("testembed-wrong"), youtube_changed_url_noquery);
-       testEmbed(document.getElementById("testembed-whywouldyouevendothat"), youtube_changed_url_noquery);
+       testEmbed(document.getElementById("testembed-wrong"), youtube_changed_url_query);
+       testEmbed(document.getElementById("testembed-whywouldyouevendothat"), youtube_changed_url_query);
        SimpleTest.finish();
      }
     </script>
   </head>
   <body onload="onLoad()">
     <embed id="testembed-correct"
            src="https://mochitest.youtube.com/v/Xm5i5kbIXzc?start=10&end=20"
            type="application/x-shockwave-flash"