Bug 70861

Summary: [MutationObservers] Support attributeOldValue for attribute mutations
Product: WebKit Reporter: Adam Klein <adamk>
Component: DOMAssignee: Adam Klein <adamk>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ojan, rafaelw, rniwa, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 68729    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Adam Klein 2011-10-25 16:31:35 PDT
[MutationObservers] Implement attributeOldValue for attribute mutations
Comment 1 Adam Klein 2011-10-27 16:57:41 PDT
Created attachment 112784 [details]
Patch
Comment 2 Ryosuke Niwa 2011-10-27 17:16:35 PDT
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?
Comment 3 Adam Klein 2011-10-27 18:14:46 PDT
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.
Comment 4 Adam Klein 2011-10-27 18:15:04 PDT
Created attachment 112799 [details]
Patch
Comment 5 Ryosuke Niwa 2011-10-28 16:56:29 PDT
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.
Comment 6 Adam Klein 2011-10-28 16:57:35 PDT
Created attachment 112941 [details]
Patch
Comment 7 Adam Klein 2011-10-28 16:59:19 PDT
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.
Comment 8 Adam Klein 2011-10-28 17:24:04 PDT
Created attachment 112947 [details]
Patch for landing
Comment 9 WebKit Review Bot 2011-10-28 18:31:21 PDT
Comment on attachment 112947 [details]
Patch for landing

Clearing flags on attachment: 112947

Committed r98791: <http://trac.webkit.org/changeset/98791>
Comment 10 WebKit Review Bot 2011-10-28 18:31:26 PDT
All reviewed patches have been landed.  Closing bug.