Rewrite YouTube Flash embeds

chrome/renderer/chrome_content_renderer_client.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 <memory>
 #include <utility>
 
 #include "base/command_line.h"
@@ skipping 1372 matching lines @@
 // chrome.system.network.getNetworkInterfaces provides the same
 // information. Also, the enforcement of sending and binding UDP is already done
 // by chrome extension permission model.
 bool ChromeContentRendererClient::ShouldEnforceWebRTCRoutingPreferences() {
 #if defined(ENABLE_EXTENSIONS)
   return !IsStandaloneExtensionProcess();
 #else
   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;
+}