[webkit-reviews] review granted: [Bug 101110] [V8] IntrusiveDOMWrapperMap should be usable for more than just Nodes : [Attachment 172376] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 5 12:48:34 PST 2012


Kentaro Hara <haraken at chromium.org> has granted Adam Barth
<abarth at webkit.org>'s request for review:
Bug 101110: [V8] IntrusiveDOMWrapperMap should be usable for more than just
Nodes
https://bugs.webkit.org/show_bug.cgi?id=101110

Attachment 172376: Patch
https://bugs.webkit.org/attachment.cgi?id=172376&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=172376&action=review


super cool!

> Source/WebCore/ChangeLog:9
> +	   to be useable for more than just nodes. After this patch, any object


Typo: usable

> Source/WebCore/bindings/v8/DOMDataStore.cpp:59
> +    if (!isolate)
> +	   isolate = v8::Isolate::GetCurrent();
>      V8PerIsolateData* data = V8PerIsolateData::from(isolate);

You can use V8PerIsolateData::current(isolate = 0)

> Source/WebCore/bindings/v8/DOMDataStore.cpp:75
> +{

You can add ASSERT(m_type == MainWorld).

> Source/WebCore/bindings/v8/DOMDataStore.cpp:81
> +    // Note: |object| might not be equal to |key|, e.g., if ScriptWrappable
isn't a left-most base class.

Nit: This comment could be put on the above line.

> Source/WebCore/bindings/v8/DOMDataStore.h:67
> +	   if (wrapperIsStoredInObject(object))
> +	       return getWrapperFromObject(object);
> +	   return m_hashMap.get(object);

How about killing wrapperIsStoredInObject() and writing the code like this?

v8::Handle<v8::Object> wrapper = getWrapperFromObject(object);
if (!wrapper.IsEmpty())
    return wrapper;
return m_hashMap.get(object);

> Source/WebCore/bindings/v8/DOMDataStore.h:81
>  protected:

private: ?

> Source/WebCore/bindings/v8/DOMDataStore.h:83
> +    inline bool wrapperIsStoredInObject(void*) const { return false; }
> +    inline bool wrapperIsStoredInObject(ScriptWrappable*) const { return
m_type == MainWorld; }

As mentioned above, I think you can remove these methods.

> Source/WebCore/bindings/v8/DOMDataStore.h:86
> +    inline v8::Handle<v8::Object> getWrapperFromObject(ScriptWrappable* key)
const { return key->wrapper(); }

You can add ASSERT(m_type == MainWorld).

> Source/WebCore/bindings/v8/DOMDataStore.h:89
> +    inline bool storeWrapperInObject(void*, v8::Persistent<v8::Object>) {
return false; }
> +    inline bool storeWrapperInObject(ScriptWrappable* key,
v8::Persistent<v8::Object> wrapper)

setWrapperInObject() would be better. (c.f. getWrapperFromObject())

> Source/WebCore/bindings/v8/DOMDataStore.h:101
>      Type m_type;

m_worldType might be better. You can also rename Type to WorldType.

> Source/WebCore/bindings/v8/DOMDataStore.h:102
> +    DOMWrapperHashMap<void> m_hashMap;

m_wrapperMap might be better.


More information about the webkit-reviews mailing list