[webkit-reviews] review denied: [Bug 27762] [v8] Cache atomic strings in externalized v8 strings : [Attachment 33628] Check that cached atomic string stay on the main thread

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Christian Plesner Hansen
<christian.plesner.hansen at gmail.com>'s request for review:
Bug 27762: [v8] Cache atomic strings in externalized v8 strings
https://bugs.webkit.org/show_bug.cgi?id=27762

Attachment 33628: Check that cached atomic string stay on the main thread
https://bugs.webkit.org/attachment.cgi?id=33628&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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).