[webkit-reviews] review denied: [Bug 27655] [v8] cache v8 strings when converting from webcore string to v8 string : [Attachment 33453] Initial version

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied anton muhin <antonm at chromium.org>'s
request for review:
Bug 27655: [v8] cache v8 strings when converting from webcore string to v8
string
https://bugs.webkit.org/show_bug.cgi?id=27655

Attachment 33453: Initial version
https://bugs.webkit.org/attachment.cgi?id=33453&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Just a few things to clean up.

> Index: third_party/WebKit/WebCore/bindings/v8/V8Binding.cpp

> +HashMap<StringImpl*, v8::String*> stringCache;

Better to use DEFINE_STATE_LOCAL.  (Then it is allocated on demand instead of
at start up and it won't have to get deleted on shutdown. You could make some
statically linked function to expose this.)

> +
> +static void cachedStringCallback(v8::Persistent<v8::Value> wrapper, void*
parameter)
> +{

Assert that this is only called on the main thread (since HashMap isn't
threadsafe).

> +    StringImpl* stringImpl = static_cast<StringImpl*>(parameter);
> +    ASSERT(stringCache.contains(stringImpl));
> +    stringCache.remove(stringImpl);
> +    wrapper.Dispose();
>  }
>  
>  v8::Local<v8::String> v8ExternalString(const String& string)

A "to" prefix is usually used on conversion functions like this.  (Or perhaps
you want findCachedV8String?)

>  {

Assert that this is only called on the main thread (since HashMap isn't
threadsafe). (As long as, that's true.)

> +
> +    StringImpl* stringImpl = string.impl();
> +    v8::String* s = stringCache.get(stringImpl);
> +    if (s) return v8::Local<v8::String>(s);

Avoid abbreviations:
s/s/cachedV8String/