[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