[webkit-reviews] review denied: [Bug 27655] [v8] cache v8 strings when converting from webcore string to v8 string : [Attachment 33718] Next iteration

[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 33718: Next iteration
https://bugs.webkit.org/attachment.cgi?id=33718&action=review

------- Additional Comments from David Levin <levin at chromium.org>
> Index: third_party/WebKit/WebCore/ChangeLog
> +2009-07-24  Anton Muhin  <antonm at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Cache v8 strings when converting from WebCore::String to v8 string.
> +	   https://bugs.webkit.org/show_bug.cgi?id=27655
> +
> +	   * bindings/v8/V8Binding.cpp:
> +	   (WebCore::v8String):
> +	   (WebCore::cachedStringCallback):
> +	   (WebCore::v8ExternalString):

This needs to be updated.  Also per function comments are encourage for
non-trivial changes to explain what changes you did to the functions.


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

> +static bool stringImplCacheEnabled = false;
> +
> +void enableStringImplCache()
> +{
> +    stringImplCacheEnabled = true;
> +}

We discussed a compile time flag or runtime.  After seeing this, I much prefer
the compile time define because 
1. I think there are several places in the v8 bindings that will need this.
2. I don't think this is a runtime decision, and making it compile time will
have the benefits of making the separate code pieces easier to see, reduce
complexity of thinking about what happens if the flag is set after some class
are done, get rid of an unnecessary branch in your code.

Perhaps:
  V8_SINGLE_THREADED
and only chrome builds define this.


> +    v8::Persistent<v8::String> wrapper =
v8::Persistent<v8::String>::New(newString);
> +    if (wrapper.IsEmpty())
> +	   return newString;

Does "wrapper.Dispose();" need to be called before the return? (I'm not totally
familiar with persistent handles in v8.)


> Index: third_party/WebKit/WebCore/bindings/v8/V8Binding.h
> +    // Enables caching v8 wrappers created for WebCore::StringImpl. 
Currently this cache requires
> +    // all the calls (both to convert WebCore::String to v8::String and to
GC the handle)
> +    // to be peformed on the main thread.

typo: peformed
(just in case this comment is kept)