Issue 1280 - Update v8

src/Utils.cpp

Index: src/Utils.cpp
===================================================================
--- a/src/Utils.cpp
+++ b/src/Utils.cpp
@@ -43,9 +43,10 @@
     return std::string();
 }
 
-v8::Local<v8::String> Utils::ToV8String(const std::string& str)
+v8::Local<v8::String> Utils::ToV8String(v8::Isolate* isolate, const std::string& str)
 {
-  return v8::String::New(str.c_str(), str.length());
+  return v8::String::NewFromUtf8(isolate, str.c_str(),
+    v8::String::NewStringType::kInternalizedString, str.length());

[Wladimir Palant, 2014/11/01 22:30:25]

I doubt that using internalized strings makes sense here - chances are that we
have a string that will only be used once and never again, and we will waste
resources unnecessarily on internalizing it. The default is kNormalString.

[Felix Dahlke, 2014/11/02 03:55:20]

Somehow thought kInternalizedString was the default and this was just an API
change for the V8 update. I agree that:

a) We should not mix this up with issue 1280 if it's not necessary for the V8
update.
b) String interning doesn't seem to make sense for us as Wladimir said, we're
dealing with a huge number of unique strings.

[sergei, 2014/11/03 12:58:19]

Thanks, I see what it means, anyway it's not important for that update, so I've
reverted it back.

 }
 
 
@@ -84,22 +85,20 @@
 {
   HRESULT hr;
 
-  std::wstring canonizedUrl;
   DWORD canonizedUrlLength = 2049; // de-facto limit of url length
-
-  canonizedUrl.resize(canonizedUrlLength);
-  hr = UrlCanonicalize(url.c_str(), &canonizedUrl[0], &canonizedUrlLength, 0);
+  std::wstring canonizedUrl(canonizedUrlLength, L'\0');
+  DWORD flags = 0; // nothing special
+  hr = UrlCanonicalizeW(url.c_str(), &canonizedUrl[0], &canonizedUrlLength, flags);
   canonizedUrl.resize(canonizedUrlLength);
   if (FAILED(hr))
   {
-    hr = UrlCanonicalize(url.c_str(), &canonizedUrl[0], &canonizedUrlLength, 0);
+    hr = UrlCanonicalizeW(url.c_str(), &canonizedUrl[0], &canonizedUrlLength, flags);
     if (FAILED(hr))
     {
       throw std::runtime_error("CanonizeUrl failed\n");
     }
   }
   return canonizedUrl;
-
 }

[Wladimir Palant, 2014/11/01 22:30:25]

The changes to this function are again completely unrelated.

[Felix Dahlke, 2014/11/02 03:55:20]

I actually presumed switching to UrlCanonicalizeW was somehow necessary for the
V8 update, but as the description says, it's just unrelated. Let's please not
mix up dozens of unrelated changes in one commit, particularly not for a P2
issue that could cause tons of regressions already, being a dependency update.

 #endif