Bug 35245 – [V8] Clean up V8DOMWrapper::getEventListener()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

We currently have 10 variants of V8DOMWrapper::getEventListener(). There's no reason we can't reduce that to 1.

This cleanup will also make it feasible to generate most of V8's addEventListener and removeEventListener callbacks. The exception is DOMWindow, which has some additional security restrictions on its add/remove functions.

[Nate Chapin]

Created attachment 49224 [details]
patch

[Dimitri Glazkov (Google)]

Comment on attachment 49224 [details]
patch

Awesome, with comments:

> +  static v8::Handle<v8::Value> ${functionName}EventListenerCallback(const v8::Arguments& args)
> +  {
> +    INC_STATS("DOM.${implClassName}.${functionName}EventListener()");
> +    RefPtr<EventListener> listener = V8DOMWrapper::getEventListener(args[1], false, ListenerFind${lookupType});
> +    if (listener) {
> +      V8${implClassName}::toNative(args.Holder())->${functionName}EventListener(v8ValueToAtomicWebCoreString(args[0]), listener${passRefPtrHandling}, args[2]->BooleanValue());
> +      ${hiddenDependencyAction}HiddenDependency(args.Holder(), args[1], V8${implClassName}::cacheIndex);
> +    }
> +    return v8::Undefined();
> +  }

Can haz proper 4-space indent here? I know we seem to generate 2-spacers, but that should change too.

> +    if (WorkerContextExecutionProxy::retrieve())
> +        return V8EventListenerList::findOrCreateWrapper<V8WorkerContextEventListener>(value, isAttribute);

Can we not get rid of this using your new fancy trick? Or is this already using it?

[Nate Chapin]

> > +    if (WorkerContextExecutionProxy::retrieve())
> > +        return V8EventListenerList::findOrCreateWrapper<V8WorkerContextEventListener>(value, isAttribute);
> 
> Can we not get rid of this using your new fancy trick? Or is this already using
> it?

I'm already using it above, so I think this may be redundant.  I think that the only case where we would have no WorkerContextExecutionProxy and no V8Proxy would be the early exit case due to there being no current v8::Context.  Does that seem right?

[Dimitri Glazkov (Google)]

(In reply to comment #3)
> > > +    if (WorkerContextExecutionProxy::retrieve())
> > > +        return V8EventListenerList::findOrCreateWrapper<V8WorkerContextEventListener>(value, isAttribute);
> > 
> > Can we not get rid of this using your new fancy trick? Or is this already using
> > it?
> 
> I'm already using it above, so I think this may be redundant.  I think that the
> only case where we would have no WorkerContextExecutionProxy and no V8Proxy
> would be the early exit case due to there being no current v8::Context.  Does
> that seem right?

Sure.

[anton muhin]

Sorry for being late---we were on holidays Mon/Tue here.

Vitaly ([e-mail]) worked with event listeners, Vitaly, any comments?

Nice cleanup, btw, Nate.

[Nate Chapin]

(In reply to comment #5)
> Sorry for being late---we were on holidays Mon/Tue here.
> 
> Vitaly ([e-mail]) worked with event listeners, Vitaly, any
> comments?
> 
> Nice cleanup, btw, Nate.

Thanks! :)

This was landed in http://trac.webkit.org/changeset/55096, sorry for not updating the bug.