Bug 70861 - [MutationObservers] Support attributeOldValue for attribute mutations
Summary: [MutationObservers] Support attributeOldValue for attribute mutations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on:
Blocks: 68729
  Show dependency treegraph
 
Reported: 2011-10-25 16:31 PDT by Adam Klein
Modified: 2011-10-28 18:31 PDT (History)
6 users (show)

See Also:


Attachments
Patch (17.24 KB, patch)
2011-10-27 16:57 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (19.62 KB, patch)
2011-10-27 18:15 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (20.29 KB, patch)
2011-10-28 16:57 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (20.41 KB, patch)
2011-10-28 17:24 PDT, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.