Rewrite YouTube Flash embeds

chrome/renderer/chrome_content_renderer_client.cc

Index: chrome/renderer/chrome_content_renderer_client.cc
diff --git a/chrome/renderer/chrome_content_renderer_client.cc b/chrome/renderer/chrome_content_renderer_client.cc
index 68c5529d55ea8d86eeaf69d6b5891f783f3a670a..0392c4b6899ef3670f0db7818ceb3ceddee5a885 100644
--- a/chrome/renderer/chrome_content_renderer_client.cc
+++ b/chrome/renderer/chrome_content_renderer_client.cc
@@ -1390,3 +1390,45 @@ bool ChromeContentRendererClient::ShouldEnforceWebRTCRoutingPreferences() {
   return true;
 #endif
 }
+
+std::string ChromeContentRendererClient::OverrideFlashEmbedWithHTML(
+    const std::string& url) {
+

[mlamouri (slow - plz ping), 2016/07/21 15:55:07]

style: remove empty line

[kdsilva, 2016/07/22 13:55:57]

Done.

+  GURL gurl = GURL(url);

[mlamouri (slow - plz ping), 2016/07/21 15:55:07]

nit: I would call the `url` you get as param `str_url` because you actually only
use it once (to convert to GURL).

[kdsilva, 2016/07/22 13:55:57]

Done.

+

[mlamouri (slow - plz ping), 2016/07/21 15:55:07]

nit: you could add `DCHECK(url.isValid())` so it guarantees that no one tries to
send garbage.

[kdsilva, 2016/07/22 13:55:57]

Done.

+  std::string retUrl = url;

[mlamouri (slow - plz ping), 2016/07/21 15:55:07]

This should move after the early return so we avoid allocating a string that we
will not use.

[kdsilva, 2016/07/22 13:55:57]

Done.

+
+  if (!gurl.DomainIs("youtube.com") || gurl.path().find("/v/") != 0

[mlamouri (slow - plz ping), 2016/07/21 15:55:07]

Maybe add a comment explaining why these checks? :) 

[kdsilva, 2016/07/22 13:55:57]

Done.

+      || retUrl.find("enablejsapi=1") != (size_t) -1)

[mlamouri (slow - plz ping), 2016/07/21 15:55:07]

`(size_t) -1` should be `string::npos`, right?

[kdsilva, 2016/07/22 13:55:57]

Yup! My bad.

+    return "";
+
+  // If the website is using an invalid YouTube URL, we'll try and
+  // fix the URL by ensuring that IF there are multiple parameters,
+  // the parameter string begins with a "?" and then follows with "&"
+  // for each subsequent parameter.
+  bool invalidUrl = false;
+  size_t indexAmp = retUrl.find("&");
+  size_t indexQm = retUrl.find("?");
+
+  // URLs are considered invalid if a "&" appears before a "?", however either
+  // a "?" or a "&" must exist in the URL.
+  if (indexQm >= indexAmp &&
+    (indexAmp != std::string::npos || indexQm != std::string::npos))

[mlamouri (slow - plz ping), 2016/07/21 15:55:07]

What about using std::string::find_first_of?
http://www.cplusplus.com/reference/string/string/find_first_of/

I was thinking of a logic like:
```
size_it index = retUrl.find_first_of("&?");
bool invalidUrl = index != std::string::npos && retUrl[index] == '&';
```

You might be able to re-use this `index` to do the replace after.

[kdsilva, 2016/07/22 13:55:57]

Wow, that is a much better way of doing it than what I'd written.

+      invalidUrl = true;
+
+  if (invalidUrl) {
+      // Replace all instances of ? with &
+      size_t start_pos = 0;
+      while((start_pos = retUrl.find("?", start_pos)) != std::string::npos) {
+          retUrl.replace(start_pos, 1, "&");
+          start_pos += 1;
+      }
+
+      // ? should appear first before all parameters
+      if (indexAmp != std::string::npos) {
+        retUrl.replace(indexAmp, 1, "?");
+      }
+  }
+  retUrl.replace(retUrl.find("/v/"), 3, "/embed/");

[mlamouri (slow - plz ping), 2016/07/21 15:55:07]

Could we do this before? Like just after we know it's an URL we want to rewrite.
It would allow us to do something like:
```
if (!invalid_url)
  return result;

// Fix invalid url.
[...]
return result;
```

It would also make things easier to read because really, the most important part
is this replace :)

[kdsilva, 2016/07/22 13:55:57]

Done.

+  return retUrl;
+}