[Webkit-unassigned] [Bug 27762] [v8] Cache atomic strings in externalized v8 strings

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27762


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33879|review?                     |review+
               Flag|                            |




--- Comment #33 from David Levin <levin at chromium.org>  2009-07-31 09:40:06 PDT ---
(From update of attachment 33879)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.