[MutationObservers] Implement attributeFilter for attribute mutations
Created attachment 112960 [details] Patch
Still work to do, but all tests are written and working in V8. Still needs JSC support & cleanup.
Created attachment 113083 [details] Patch
Comment on attachment 113083 [details] Patch This is now ready for review. Note that there is a FIXME in webkit mutation observer pointing out that the addition of the attributeFilter in MutationObserverRegistration means that transient subtree observation now does more copying than necessary. As noted in Adam's characterDataOldValue change, we are waiting until both patches are landed and then will cleanup a number of things with a refactor which will split out MutationObserverRegistration into it's own class which doesn't get copied around
This is ready for review.
Comment on attachment 113083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113083&action=review > Source/WebCore/dom/Node.h:594 > + void getRegisteredMutationObserversOfType(HashMap<WebKitMutationObserver*, MutationObserverOptions>&, WebKitMutationObserver::MutationType, const AtomicString&); The third argument needs a name, since it's not clear from here what it's for. > Source/WebCore/dom/Node.h:600 > + MutationRegistrationResult registerMutationObserver(PassRefPtr<WebKitMutationObserver>, MutationObserverOptions, const HashSet<AtomicString>&, Node* registrationNode = 0); Similar comment here about the second arg; what is this HashSet used for? > Source/WebCore/dom/NodeRareData.h:120 > + return this->caseInsensitiveAttributeFilter().contains(attributeName); No need for explicit "this" > Source/WebCore/dom/NodeRareData.h:130 > + Nit: extra blank line not needed. > Source/WebCore/dom/NodeRareData.h:138 > + return this->m_caseInsensitiveAttributeFilter; No need for explicit "this" here. > Source/WebCore/dom/NodeRareData.h:157 > + HashSet<AtomicString> m_caseInsensitiveAttributeFilter; Maybe make this an OwnPtr<HashSet<...> > and get rid of the bool flag below? > Source/WebCore/dom/WebKitMutationObserver.h:72 > + void observe(Node*, MutationObserverOptions, const HashSet<AtomicString>&); Same comment on naming this new arg. > Source/WebCore/dom/WebKitMutationObserver.h:74 > + void willDetachNodeInObservedSubtree(PassRefPtr<Node> registrationNode, MutationObserverOptions, HashSet<AtomicString>&, PassRefPtr<Node> detachingNode); and here
Comment on attachment 113083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113083&action=review >> Source/WebCore/dom/Node.h:594 >> + void getRegisteredMutationObserversOfType(HashMap<WebKitMutationObserver*, MutationObserverOptions>&, WebKitMutationObserver::MutationType, const AtomicString&); > > The third argument needs a name, since it's not clear from here what it's for. done >> Source/WebCore/dom/Node.h:600 >> + MutationRegistrationResult registerMutationObserver(PassRefPtr<WebKitMutationObserver>, MutationObserverOptions, const HashSet<AtomicString>&, Node* registrationNode = 0); > > Similar comment here about the second arg; what is this HashSet used for? done >> Source/WebCore/dom/NodeRareData.h:120 >> + return this->caseInsensitiveAttributeFilter().contains(attributeName); > > No need for explicit "this" done >> Source/WebCore/dom/NodeRareData.h:130 >> + > > Nit: extra blank line not needed. done >> Source/WebCore/dom/NodeRareData.h:138 >> + return this->m_caseInsensitiveAttributeFilter; > > No need for explicit "this" here. done >> Source/WebCore/dom/NodeRareData.h:157 >> + HashSet<AtomicString> m_caseInsensitiveAttributeFilter; > > Maybe make this an OwnPtr<HashSet<...> > and get rid of the bool flag below? This "class" needs to be copyable at the moment to live in NodeRareData::OwnPtr<Vector<MutationObserverRegistration> > m_mutationObserverRegistry; >> Source/WebCore/dom/WebKitMutationObserver.h:72 >> + void observe(Node*, MutationObserverOptions, const HashSet<AtomicString>&); > > Same comment on naming this new arg. done >> Source/WebCore/dom/WebKitMutationObserver.h:74 >> + void willDetachNodeInObservedSubtree(PassRefPtr<Node> registrationNode, MutationObserverOptions, HashSet<AtomicString>&, PassRefPtr<Node> detachingNode); > > and here done
Created attachment 113366 [details] Patch
Created attachment 113582 [details] Patch
Created attachment 113602 [details] Patch
No with build fixes & delicious new refactor
Comment on attachment 113602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113602&action=review Could you separate the patch to extract MutationObserverRegistration as a class in a separate patch? In particular, I can't make sense of changes to Node.cpp. Also, I'm little worried that we might be introducing many function calls in hot path by extracting this class. > Source/WebCore/dom/CharacterData.cpp:208 > - HashMap<WebKitMutationObserver*, MutationObserverOptions> observers; > + HashMap<WebKitMutationObserver*, MutationRecordDeliveryOptions> observers; Can we put this in a separate patch? (file a bug that blocks this bug and post a patch there that just does renaming). > Source/WebCore/dom/MutationObserverRegistration.cpp:59 > +PassOwnPtr<MutationObserverRegistration> MutationObserverRegistration::create(PassRefPtr<WebKitMutationObserver> observer, Node* registrationNode) > +{ > + return adoptPtr(new MutationObserverRegistration(observer, registrationNode)); > +} > + > +MutationObserverRegistration::MutationObserverRegistration(PassRefPtr<WebKitMutationObserver> observer, Node* registrationNode) > + : m_observer(observer) > + , m_registrationNode(registrationNode) > + , m_options(0) > +{ > + m_observer->observationStarted(this); > +} > + > +MutationObserverRegistration::~MutationObserverRegistration() > +{ > + clearTransientRegistrations(); > + m_observer->observationEnded(this); > +} Can't we just make these functions inline to avoid function calls when creating/destroying MutationObserverRegistration? > Source/WebCore/dom/MutationObserverRegistration.cpp:102 > +void MutationObserverRegistration::unregister() > +{ > + m_registrationNode->unregisterMutationObserver(this); // This call will cause this object to be deleted. > +} This should just be an inline function. > Source/WebCore/dom/MutationObserverRegistration.cpp:106 > + if (!(m_options & type)) We should really have optionsContain(type) or optionsHave(type) > Source/WebCore/dom/MutationObserverRegistration.cpp:118 > + if (node->document()->isHTMLDocument() && node->isHTMLElement()) Why do we care whether node belongs to a HTML document or not? > Source/WebCore/dom/MutationObserverRegistration.cpp:124 > +const HashSet<AtomicString>& MutationObserverRegistration::caseInsensitiveAttributeFilter() Nit: two spaces after &. > Source/WebCore/dom/MutationObserverRegistration.cpp:126 > + if (!m_caseInsensitiveAttributeFilter) { Prefer early exit. > Source/WebCore/dom/MutationObserverRegistration.h:58 > + MutationRecordDeliveryOptions deliveryOptions() { return m_options & (WebKitMutationObserver::AttributeOldValue | WebKitMutationObserver::CharacterDataOldValue); } Can we use using to avoid having to repeat WebKitMutationObserver twice?
Comment on attachment 113602 [details] Patch This is now blocked on the refactor which was split out.
Created attachment 113748 [details] Patch
This is ready for review again. It's much simpler and shorter now that the refactor in bug 71577 went in.
Comment on attachment 113748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113748&action=review I think we want JSC binding before landing this patch. Maybe Sam or Anders can help us? > Source/WebCore/bindings/js/JSWebKitMutationObserverCustom.cpp:81 > + // FIXME: Add support for parsing of the attributeFilter option. We should add JSC binding. > Source/WebCore/dom/MutationObserverRegistration.cpp:127 > +const HashSet<AtomicString>& MutationObserverRegistration::caseInsensitiveAttributeFilter() Nit: two spaces after &. > Source/WebCore/dom/MutationObserverRegistration.h:73 > + OwnPtr<HashSet<AtomicString> >m_caseInsensitiveAttributeFilter; Nit: need a space after the second >. I'm also not convinced that we really need to have OwnPtr of HashSet here. I don't think we're saving much. > LayoutTests/fast/mutation/observe-attributes.html:601 > + observer.observe(div, { attributes: true, attributeFilter: ['ID', 'id', 'booM'] }); Can we add BaZ to make sure we do case-sensitive comparison in non-HTML document?
Created attachment 114168 [details] Patch
Comment on attachment 113748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113748&action=review >> Source/WebCore/dom/MutationObserverRegistration.cpp:127 >> +const HashSet<AtomicString>& MutationObserverRegistration::caseInsensitiveAttributeFilter() > > Nit: two spaces after &. done >> Source/WebCore/dom/MutationObserverRegistration.h:73 >> + OwnPtr<HashSet<AtomicString> >m_caseInsensitiveAttributeFilter; > > Nit: need a space after the second >. I'm also not convinced that we really need to have OwnPtr of HashSet here. I don't think we're saving much. done >> LayoutTests/fast/mutation/observe-attributes.html:601 >> + observer.observe(div, { attributes: true, attributeFilter: ['ID', 'id', 'booM'] }); > > Can we add BaZ to make sure we do case-sensitive comparison in non-HTML document? booM is already testing that.
Comment on attachment 113748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113748&action=review >>> LayoutTests/fast/mutation/observe-attributes.html:601 >>> + observer.observe(div, { attributes: true, attributeFilter: ['ID', 'id', 'booM'] }); >> >> Can we add BaZ to make sure we do case-sensitive comparison in non-HTML document? > > booM is already testing that. But you're setting booM, right? What I want to see is a test case where you listen to BaZ and you only set baz (so that you don't hear anything).
Comment on attachment 114168 [details] Patch r=me for everything except V8-binding. Dimitri, could you take a look at V8 part?
Comment on attachment 114168 [details] Patch Clearing flags on attachment: 114168 Committed r99992: <http://trac.webkit.org/changeset/99992>
All reviewed patches have been landed. Closing bug.