[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.
Created attachment 90287 [details] patch
Created attachment 90290 [details] patch: fixed a typo
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.
Created attachment 90793 [details] patch: less nasty dependencies Won't compile without bug 59250.
Created attachment 90797 [details] patch: less nasty dependencies Removed accidental changes. Still won't compile without bug 59250.
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 ?
Attachment 90797 [details] did not build on qt: Build output: http://queues.webkit.org/results/8486964
Created attachment 90823 [details] patch
(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.
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.
Attachment 90797 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8495650
Attachment 90797 [details] did not build on win: Build output: http://queues.webkit.org/results/8496577
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
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