[webkit-reviews] review denied: [Bug 29825] [V8] Refactored V8 event listeners. : [Attachment 40360] updated patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Sep 30 10:26:49 PDT 2009


Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Vitaly Repeshko
<vitalyr at chromium.org>'s request for review:
Bug 29825: [V8] Refactored V8 event listeners.
https://bugs.webkit.org/show_bug.cgi?id=29825

Attachment 40360: updated patch
https://bugs.webkit.org/attachment.cgi?id=40360&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
This is super-awesome. I only have style nits.

I would really like to use enums for isAttribute params in getEventListener.
That should clarify the call sites.

> +    V8Proxy* proxy = V8Proxy::retrieve(worker->scriptExecutionContext());
> +    if (proxy) {
> +	   return findOnly ? V8EventListenerList::findWrapper(value,
isAttribute) :
V8EventListenerList::findOrCreateWrapper<V8EventListener>(proxy->frame(),
value, isAttribute);
> +    }

Don't need braces here.

> +    if (proxy) {
> +	   return findOnly ? V8EventListenerList::findWrapper(value,
isAttribute) :
V8EventListenerList::findOrCreateWrapper<V8EventListener>(proxy->frame(),
value, isAttribute);
> +    }

Ditto.

> +v8::Handle<v8::String> V8HiddenPropertyName::listener()
> +{
> +    static v8::Persistent<v8::String>* string =
createString("WebCore::V8HiddenPropertyName::listener");
> +

no line break here.

> +    return *string;
> +}
> +
> +v8::Handle<v8::String> V8HiddenPropertyName::attributeListener()
> +{
> +    static v8::Persistent<v8::String>* string =
createString("WebCore::V8HiddenPropertyName::attributeListener");

Ditto.

> +void V8Proxy::disconnectEventListeners()
> +{
> +}
> +

Why is this here? Can we just get rid of it?


More information about the webkit-reviews mailing list