Issue 1280 - Update v8

src/Utils.cpp

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 25 matching lines @@
 
 std::string Utils::FromV8String(v8::Handle<v8::Value> value)
 {
   v8::String::Utf8Value stringValue(value);
   if (stringValue.length())
     return std::string(*stringValue, stringValue.length());
   else
     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.

 }
 
 
 #ifdef _WIN32
 std::wstring Utils::ToUtf16String(const std::string& str)
 {
   size_t length = str.size();
   if (length == 0)
     return std::wstring();
 
@@ skipping 18 matching lines @@
 
   std::string utf8String(utf8StringLength, '\0');
   WideCharToMultiByte(CP_UTF8, 0, str.c_str(), length, &utf8String[0], utf8StringLength, 0, 0);
   return utf8String;
 }
 
 std::wstring Utils::CanonizeUrl(const std::wstring& url)
 {
   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