[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


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33628|review?                     |review-
               Flag|                            |




--- Comment #9 from Eric Seidel <eric at webkit.org>  2009-07-28 10:27:12 PDT ---
(From update of attachment 33628)
There are tabs in your ChangeLog.

m_atomic_impl is not WebKit style.  m_atomicString would be, or m_atomicImpl
(although we've historically tried to kill Impl, excepting a few places)

+        if (m_atomic_impl.isNull()) {
+            m_atomic_impl = AtomicString(m_impl);
+        }

is not WK style.

WK uses 4 space indent:
+    if (!v8String->MakeExternal(resource)) {
+      // In case of a failure delete the external resource as it was not used.
+      delete resource;
+    }

I think you should consider running WebKit's copy of cpplint, it's found in
WebKitTools/Scripts/


JSC does this by passing UStrings directly to an AtomicString constructor
(they're implicitly constructed when getElementById is called).

PassRefPtr<StringImpl> AtomicString::add(const JSC::UString& ustring)

is where the real magic is.  (Just hashes the string and makes a StringImpl
wrapper if necessary.)

Does V8 not want to just do the same thing?  Add a new constructor for
AtomicString for a Handle<String> (or whatever the appropriate v8 string base
type is).

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