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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jul 30 06:24:40 PDT 2009


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

------- Additional Comments from David Levin <levin at chromium.org>
Last thing to fix....(the paths for the files).

> Index: third_party/WebKit/WebCore/ChangeLog
You seem to be creating these patches from outside of your WebKit enlistment.

These lines should start with WebCore (or else committing them is going to be
annoying).

If you create the patch when you are in the third_party/WebKit directory this
should all work. Alternately, you could just carefully edit the patch by hand.

> ===================================================================
> --- third_party/WebKit/WebCore/ChangeLog	(revision 46575)
> +++ third_party/WebKit/WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,24 @@
> +2009-07-30  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): now just immediately calls v8ExternalString
> +	   (WebCore::enableStringImplCache): enables caching of conversions
from WebCore::StringImpl to
> +	   v8::String
> +	   (WebCore::makeExternalString): utilty function to create external
v8::String out of
> +	   WebCore::Strin
typo: Strin

I can fix this on landing, but if you upload a new patch, it would be nice to
fix it.


> I'm lacking knowledge of our build process, but are you sure we have to
> separate binaries for worker and render?  I was under impression that the
> binary is the same and it flags which turn it into a worker or renderer
> process.

Yes. Note: Workers in chromium only run one thread, but we can leave this as a
runtime flag as it all appears to work.

The api feels a bit narrow in scope "enableStringImplCache" vs
"setSingleThreadedMode", but this is fine. It can be broadened if there is ever
any other code that needs this info.


More information about the webkit-reviews mailing list