Bug 70861 – [MutationObservers] Support attributeOldValue for attribute mutations

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

[Adam Klein]

[MutationObservers] Implement attributeOldValue for attribute mutations

[Adam Klein]

Created attachment 112784 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 112784 [details]
Patch

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

> Source/WebCore/dom/MutationRecord.cpp:47
> +class MutationRecordBase : public MutationRecord {

I'm a bit confused here why does MutationRecord*Base* inherit from MutationRecord? It seems backwards.

> Source/WebCore/dom/MutationRecord.cpp:131
> +    virtual const AtomicString& type() { return m_record->type(); }
> +    virtual Node* target() { return m_record->target(); }
> +    virtual NodeList* addedNodes() { return m_record->addedNodes(); }
> +    virtual NodeList* removedNodes() { return m_record->removedNodes(); }
> +    virtual Node* previousSibling() { return m_record->previousSibling(); }
> +    virtual Node* nextSibling() { return m_record->nextSibling(); }
> +    virtual const AtomicString& attributeName() { return m_record->attributeName(); }
> +    virtual const AtomicString& attributeNamespace() { return m_record->attributeNamespace(); }

Could we use OVERRIDE macro? (Can be done in a follow up patch since all other classes don't us it) The fact all these functions are virtual isn't great. It might affect performance.

> LayoutTests/fast/mutation/observe-attributes.html:311
> +        div.setAttribute('foo', 'bar');
> +        div.setAttribute('foo', 'baz');

Can we add a test case where a reflected IDL attribute is modified?

[Adam Klein]

Comment on attachment 112784 [details]
Patch

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

>> Source/WebCore/dom/MutationRecord.cpp:47
>> +class MutationRecordBase : public MutationRecord {
> 
> I'm a bit confused here why does MutationRecord*Base* inherit from MutationRecord? It seems backwards.

This class implements target() (storing m_target).  I introduced it because MutationRecordWithNullOldValue doesn't store its own MutationTarget.

Given your confusion, though, it's not worthwhile.  Turns out it's fewer lines just to give each of the three subclasses that need it an m_target member.

>> Source/WebCore/dom/MutationRecord.cpp:131
>> +    virtual const AtomicString& attributeNamespace() { return m_record->attributeNamespace(); }
> 
> Could we use OVERRIDE macro? (Can be done in a follow up patch since all other classes don't us it) The fact all these functions are virtual isn't great. It might affect performance.

I didn't see OVERRIDE used anywhere in WebCore when I wrote this up originally. But I see some now, so I'm happy to add it (love OVERRIDE).

As for the use of virtual methods, I'd originally used a different approach, and Darin suggested (in https://bugs.webkit.org/show_bug.cgi?id=68824) that I go with virtual methods instead.

>> LayoutTests/fast/mutation/observe-attributes.html:311
>> +        div.setAttribute('foo', 'baz');
> 
> Can we add a test case where a reflected IDL attribute is modified?

Done.

[Adam Klein]

Created attachment 112799 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 112799 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:642
> +    RefPtr<MutationRecord> mutation = MutationRecord::createAttributes(element, name, isOldValueRequested(observers) ? oldValue : nullAtom);
> +    RefPtr<MutationRecord> mutationWithNullOldValue = MutationRecord::createWithNullOldValue(mutation);

It'll be nice if we could lazily allocate these objects since they live in heap but that might be a pre-mature optimization given createWithNullOldValue only stores a pointer.

> LayoutTests/fast/mutation/observe-attributes.html:346
> +        observer = new WebKitMutationObserver(function(mutations) {
> +            window.mutations = mutations;
> +        });
> +        observer2 = new WebKitMutationObserver(function(mutations) {
> +            window.mutations2 = mutations;
> +        });

Can we give more descriptive names to observer and observer2?

> LayoutTests/fast/mutation/observe-attributes.html:358
> +        shouldBe('mutations[0].oldValue', '"bar"');
> +        shouldBe('mutations2.length', '1');

Ditto on mutations vs. mutations2.

[Adam Klein]

Created attachment 112941 [details]
Patch

[Adam Klein]

Comment on attachment 112799 [details]
Patch

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

>> Source/WebCore/dom/Element.cpp:642
>> +    RefPtr<MutationRecord> mutationWithNullOldValue = MutationRecord::createWithNullOldValue(mutation);
> 
> It'll be nice if we could lazily allocate these objects since they live in heap but that might be a pre-mature optimization given createWithNullOldValue only stores a pointer.

Made mutationWithNullOldValue lazy.

>> LayoutTests/fast/mutation/observe-attributes.html:346
>> +        });
> 
> Can we give more descriptive names to observer and observer2?

Done.

>> LayoutTests/fast/mutation/observe-attributes.html:358
>> +        shouldBe('mutations2.length', '1');
> 
> Ditto on mutations vs. mutations2.

Done.

[Adam Klein]

Created attachment 112947 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 112947 [details]
Patch for landing

Clearing flags on attachment: 112947

Committed r98791: <http://trac.webkit.org/changeset/98791>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.