[webkit-reviews] review granted: [Bug 27762] [v8] Cache atomic strings in externalized v8 strings : [Attachment 33879] Fixed memory waste etc.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted David Levin <levin at chromium.org>'s
request for review:
Bug 27762: [v8] Cache atomic strings in externalized v8 strings
https://bugs.webkit.org/show_bug.cgi?id=27762

Attachment 33879: Fixed memory waste etc.
https://bugs.webkit.org/attachment.cgi?id=33879&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Just a few things that can be fixed on landing..


> Index: WebCore/ChangeLog
> ===================================================================
> +2009-07-31  Christian Plesner Hansen  <christian.plesner.hansen at gmail.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   [V8] Cache atomic strings in externalized v8 strings
> +	   https://bugs.webkit.org/show_bug.cgi?id=27762
> +

I should have said this before, but typically there is a comment here about why
no layout tests were added if none were.

In this case, there is no change in functionality, so you could say:

	No change in behavior, so no tests.


> Index: WebCore/bindings/v8/V8Binding.cpp
> ===================================================================

> +String v8StringToWebCoreString(v8::Handle<v8::String> v8String, ExternalMode
external,
> +	   StringType type)

This is typically indented to align with the (.



> Index: WebCore/bindings/v8/V8Binding.h

Enum members should user InterCaps with an initial capital letter.

> +    enum ExternalMode { EXTERNALIZE, DONT_EXTERNALIZE };

Externalize, DoNotExternalize

> +    enum StringType { PLAIN, ATOMIC };

Since these enum are in the WebCore namespace (and cased like classes), it
feels like the names for StringType should be a little more verbose.

Here's some simple alternatives:
PlainStringType
AtomicStringType