Bug 70860 – [MutationObservers] Support attributeFilter for attribute mutations

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Adam Klein]

[MutationObservers] Implement attributeFilter for attribute mutations

[Rafael Weinstein]

Created attachment 112960 [details]
Patch

[Rafael Weinstein]

Still work to do, but all tests are written and working in V8. Still needs JSC support & cleanup.

[Rafael Weinstein]

Created attachment 113083 [details]
Patch

[Rafael Weinstein]

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

[Rafael Weinstein]

This is ready for review.

[Adam Klein]

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

[Rafael Weinstein]

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

[Rafael Weinstein]

Created attachment 113366 [details]
Patch

[Rafael Weinstein]

Created attachment 113582 [details]
Patch

[Rafael Weinstein]

Created attachment 113602 [details]
Patch

[Rafael Weinstein]

No with build fixes & delicious new refactor

[Ryosuke Niwa]

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?

[Rafael Weinstein]

Comment on attachment 113602 [details]
Patch

This is now blocked on the refactor which was split out.

[Rafael Weinstein]

Created attachment 113748 [details]
Patch

[Rafael Weinstein]

This is ready for review again. It's much simpler and shorter now that the refactor in bug 71577 went in.

[Ryosuke Niwa]

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?

[Rafael Weinstein]

Created attachment 114168 [details]
Patch

[Rafael Weinstein]

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.

[Ryosuke Niwa]

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).

[Ryosuke Niwa]

Comment on attachment 114168 [details]
Patch

r=me for everything except V8-binding. Dimitri, could you take a look at V8 part?

[Adam Klein]

Comment on attachment 114168 [details]
Patch

Clearing flags on attachment: 114168

Committed r99992: <http://trac.webkit.org/changeset/99992>

[Adam Klein]

All reviewed patches have been landed.  Closing bug.