[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