Bug 58953 – [V8] Use implicit references for V8 listeners on DOM nodes.

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

[Vitaly Repeshko]

[V8] Use implicit references for V8 listeners on DOM nodes.

Instead of allocating an auxiliary V8 array referencing V8 listener objects associated with a DOM node and using an extra pointer in every DOM node wrapper, we can register implicit references between nodes and their listeners during GC.

[Vitaly Repeshko]

Created attachment 90287 [details]
patch

[Vitaly Repeshko]

Created attachment 90290 [details]
patch: fixed a typo

[Adam Barth]

Comment on attachment 90287 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90287&action=review

The V8-specific could should be in bindings/v8.  You might need to build some sort of abstraction to make that work, but your patch has it's dependencies backwards.

> Source/WebCore/bindings/v8/V8AbstractEventListener.h:96
> +        v8::Persistent<v8::Object> getExistingListenerObjectPeristentHandle()

getExistingListenerObjectPeristentHandle => listener ?

We don't usually start functions with the word "get".  Also, we can tell it returns a persistent handle by the return type.

> Source/WebCore/dom/EventListener.h:61
> +#if USE(V8)
> +        virtual bool isV8EventListener() const { return false; }
> +#endif

Frown.

> Source/WebCore/dom/EventTarget.cpp:39
> +#if USE(V8)
> +#include "V8AbstractEventListener.h"
> +#endif

This is a backwards dependency.

> Source/WebCore/dom/EventTarget.cpp:381
> +#if USE(V8)

This code shouldn't be here.

[Vitaly Repeshko]

Created attachment 90793 [details]
patch: less nasty dependencies

Won't compile without bug 59250.

[Vitaly Repeshko]

Created attachment 90797 [details]
patch: less nasty dependencies

Removed accidental changes. Still won't compile without bug 59250.

[Adam Barth]

Comment on attachment 90797 [details]
patch: less nasty dependencies

View in context: https://bugs.webkit.org/attachment.cgi?id=90797&action=review

> Source/WebCore/bindings/v8/V8AbstractEventListener.h:94
> +        v8::Persistent<v8::Object> getExistingListenerObjectPersistentHandle()

Please remove "get" prefix.

> Source/WebCore/dom/EventTarget.cpp:394
> +EventListenerIterator::EventListenerIterator() : m_index(0) {}

The { and } should each be on their own line.

> Source/WebCore/dom/EventTarget.cpp:396
> +EventListenerIterator::EventListenerIterator(EventTarget* target) : m_index(0)

": m_index(0)" should be on its own line and indented four spaces.

> Source/WebCore/dom/EventTarget.cpp:410
> +        EventListenerVector& listeners = *m_mapIterator->second;
> +        if (m_index < listeners.size())
> +            return listeners[m_index++].listener.get();

We don't need to reset m_index to zero when we ++m_mapIterator ?

[Early Warning System Bot]

Attachment 90797 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8486964

[Vitaly Repeshko]

Created attachment 90823 [details]
patch

[Vitaly Repeshko]

(In reply to comment #6)
> (From update of attachment 90797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90797&action=review
> 
> > Source/WebCore/bindings/v8/V8AbstractEventListener.h:94
> > +        v8::Persistent<v8::Object> getExistingListenerObjectPersistentHandle()
> 
> Please remove "get" prefix.

Removed.

> > Source/WebCore/dom/EventTarget.cpp:394
> > +EventListenerIterator::EventListenerIterator() : m_index(0) {}
> 
> The { and } should each be on their own line.

Fixed.

> > Source/WebCore/dom/EventTarget.cpp:396
> > +EventListenerIterator::EventListenerIterator(EventTarget* target) : m_index(0)
> 
> ": m_index(0)" should be on its own line and indented four spaces.

Fixed.

> > Source/WebCore/dom/EventTarget.cpp:410
> > +        EventListenerVector& listeners = *m_mapIterator->second;
> > +        if (m_index < listeners.size())
> > +            return listeners[m_index++].listener.get();
> 
> We don't need to reset m_index to zero when we ++m_mapIterator ?

Whoops. Fixed.

[Adam Barth]

Comment on attachment 90823 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90823&action=review

> Source/WebCore/dom/EventTarget.cpp:394
> +EventListenerIterator::EventListenerIterator() : m_index(0)

Sorry, this initializer needs to be on its own line too.

> Source/WebCore/dom/EventTarget.h:197
> +        EventListener* nextListener();

I would add a comment admonishing folks not to modify the listener list durning iteration because that will cause big problems for this iterator.

[WebKit Review Bot]

Attachment 90797 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8495650

[Build Bot]

Attachment 90797 [details] did not build on win:
Build output: http://queues.webkit.org/results/8496577

[Vitaly Repeshko]

The remaining comments addressed on landing.

	M	Source/WebCore/ChangeLog
	M	Source/WebCore/bindings/scripts/CodeGeneratorV8.pm
	M	Source/WebCore/bindings/v8/V8AbstractEventListener.h
	M	Source/WebCore/bindings/v8/V8GCController.cpp
	M	Source/WebCore/dom/EventTarget.cpp
	M	Source/WebCore/dom/EventTarget.h
Committed r84741

[WebKit Review Bot]

http://trac.webkit.org/changeset/84741 might have broken Windows XP Debug (Tests)
The following tests are not passing:
http/tests/misc/will-send-request-returns-null-on-redirect.html