[MutationObservers] Implement attributeOldValue for attribute mutations
Created attachment 112784 [details] Patch
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 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.
Created attachment 112799 [details] Patch
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.
Created attachment 112941 [details] Patch
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.
Created attachment 112947 [details] Patch for landing
Comment on attachment 112947 [details] Patch for landing Clearing flags on attachment: 112947 Committed r98791: <http://trac.webkit.org/changeset/98791>
All reviewed patches have been landed. Closing bug.