Bug 68824 – [MutationObservers] implement MutationRecord

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

[Adam Klein]

[MutationObservers] implement MutationRecord

[Adam Klein]

Created attachment 108719 [details]
Patch

[Darin Adler]

Comment on attachment 108719 [details]
Patch

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

> Source/WebCore/dom/MutationRecord.h:90
> +    enum MutationTypeFlag {
> +        ChildListFlag = 1 << 0,
> +        AttributesFlag = 1 << 1,
> +        CharacterDataFlag = 1 << 2
> +    };

If these are three mutually exclusive values, then why are we shifting to make them separate bits? It seems this should just be a normal enum.

> Source/WebCore/dom/MutationRecord.h:91
> +    MutationTypeFlag typeFlag() { return m_typeFlag; }

I don’t understand the use of the word “flag” here.

> Source/WebCore/dom/MutationRecord.h:115
> +    void* m_extraData;

Since we’re already paying the cost of an object for this data, seems we should just spend one more pointer in size and have this be OwnPtr<Data> and have all three of the specific classes inherit from a Data (or ExtraData or MutationData) base class with a virtual destructor. If we did that we would not need the code in ~MutationRecord and we would be properly doing adoptPtr next to each new inside the class's implementation.

> Source/WebCore/dom/MutationRecord.idl:41
> +        readonly attribute DOMString type;
> +        readonly attribute Node      target;
> +
> +        readonly attribute NodeList  addedNodes;
> +        readonly attribute NodeList  removedNodes;
> +        readonly attribute Node      previousSibling;
> +        readonly attribute Node      nextSibling;

These should not be lined up with extra spaces. We don’t do that kind of lining up in new WebKit code, even though some old IDL files have that format.

[Adam Klein]

Comment on attachment 108719 [details]
Patch

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

>> Source/WebCore/dom/MutationRecord.h:90
>> +    };
> 
> If these are three mutually exclusive values, then why are we shifting to make them separate bits? It seems this should just be a normal enum.

Though they're mutually exclusive in their use in this class, they're not mutually exclusive in other cases (this is one thing that'd be clearer if I sent this all-in-one instead of in pieces):
 When observing a node, a single MutationObserver may be listening to multiple types of changes (say, both 'childList' and 'attributes'). That registration will store a mask of (ChildListFlag & AttributesFlag), and that mask will be ORed with the MutationRecord::typeFlag() when deciding whether to deliver a given record to a registered observer.

Would it be clearer if I moved this enum out of MutationRecord.h?

>> Source/WebCore/dom/MutationRecord.h:91
>> +    MutationTypeFlag typeFlag() { return m_typeFlag; }
> 
> I don’t understand the use of the word “flag” here.

It's the same use of "flag" as Node::NodeFlags.  Do you have another suggestion?

>> Source/WebCore/dom/MutationRecord.h:115
>> +    void* m_extraData;
> 
> Since we’re already paying the cost of an object for this data, seems we should just spend one more pointer in size and have this be OwnPtr<Data> and have all three of the specific classes inherit from a Data (or ExtraData or MutationData) base class with a virtual destructor. If we did that we would not need the code in ~MutationRecord and we would be properly doing adoptPtr next to each new inside the class's implementation.

The "cost" of an object for this data is just the pointer, but if you're fine with paying a little more memory for a little more type safety (there are still plenty of "unsafe" casts), I agree that providing an ExtraData base class is cleaner.

Done, and added a templated extraData() accessor to avoid putting static_cast<Foo*>(m_foo.get()) everywhere.

>> Source/WebCore/dom/MutationRecord.idl:41
>> +        readonly attribute Node      nextSibling;
> 
> These should not be lined up with extra spaces. We don’t do that kind of lining up in new WebKit code, even though some old IDL files have that format.

Removed extra spaces.

[Adam Klein]

Created attachment 108724 [details]
Patch

[Adam Klein]

See also https://bugs.webkit.org/attachment.cgi?id=106218&action=review for approximately what the usage of this class will be (though there have been some API changes since that patch was written).

[Darin Adler]

Comment on attachment 108719 [details]
Patch

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

>>> Source/WebCore/dom/MutationRecord.h:90
>>> +    };
>> 
>> If these are three mutually exclusive values, then why are we shifting to make them separate bits? It seems this should just be a normal enum.
> 
> Though they're mutually exclusive in their use in this class, they're not mutually exclusive in other cases (this is one thing that'd be clearer if I sent this all-in-one instead of in pieces):
>  When observing a node, a single MutationObserver may be listening to multiple types of changes (say, both 'childList' and 'attributes'). That registration will store a mask of (ChildListFlag & AttributesFlag), and that mask will be ORed with the MutationRecord::typeFlag() when deciding whether to deliver a given record to a registered observer.
> 
> Would it be clearer if I moved this enum out of MutationRecord.h?

Yes, I think it would. If this type is a “flags” type because of its use elsewhere, then it should probably not be a member.

>>> Source/WebCore/dom/MutationRecord.h:91
>>> +    MutationTypeFlag typeFlag() { return m_typeFlag; }
>> 
>> I don’t understand the use of the word “flag” here.
> 
> It's the same use of "flag" as Node::NodeFlags.  Do you have another suggestion?

My confusion here is caused by reusing the flag enum typedef restricted to a single flag to identify the type of a record. That probably merits a “why” comment. The word “flag” alone raises a question about why we have a type flag instead of just a type. If we want to retain this design, it might improve clarity to use the name typeAsFlag.

It’s analogous to using a set to store a single value. Because we have plenty of extra bits, the extra cost of storing a set is not an issue, but the conceptual muddle is still there.

And to be pedantic, it’s not the same as the use in Node, because there it’s “flags” not “flag” and these issues do not arise.

>>> Source/WebCore/dom/MutationRecord.h:115
>>> +    void* m_extraData;
>> 
>> Since we’re already paying the cost of an object for this data, seems we should just spend one more pointer in size and have this be OwnPtr<Data> and have all three of the specific classes inherit from a Data (or ExtraData or MutationData) base class with a virtual destructor. If we did that we would not need the code in ~MutationRecord and we would be properly doing adoptPtr next to each new inside the class's implementation.
> 
> The "cost" of an object for this data is just the pointer, but if you're fine with paying a little more memory for a little more type safety (there are still plenty of "unsafe" casts), I agree that providing an ExtraData base class is cleaner.
> 
> Done, and added a templated extraData() accessor to avoid putting static_cast<Foo*>(m_foo.get()) everywhere.

You say it’s “just the pointer”, but that’s not right. The cost of storing data in a separate “extra data field” is both the pointer in the mutation record and the memory allocator overhead for the extra data block. Making the extra data block one pointer larger seems affordable to me. If we really wanted to keep things to the absolute minimum, then I think we’d want to find some way to store the data in line in the main object, analogous to how a StringImpl stores its string data. It's fairly ugly to do that, though. The class CSSSelector does it and so does the CSSPrimitiveValue class, but neither is quite so complicated as this class.

Hmm, since MutationRecord objects are immutable, why don’t we make MutationRecord itself a polymorphic class? That would clean up the design a lot and save a memory block. It would make the accessor functions slower because they would be virtual function calls, but that seems worthwhile.

[Adam Klein]

Comment on attachment 108719 [details]
Patch

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

>>>> Source/WebCore/dom/MutationRecord.h:90
>>>> +    };
>>> 
>>> If these are three mutually exclusive values, then why are we shifting to make them separate bits? It seems this should just be a normal enum.
>> 
>> Though they're mutually exclusive in their use in this class, they're not mutually exclusive in other cases (this is one thing that'd be clearer if I sent this all-in-one instead of in pieces):
>>  When observing a node, a single MutationObserver may be listening to multiple types of changes (say, both 'childList' and 'attributes'). That registration will store a mask of (ChildListFlag & AttributesFlag), and that mask will be ORed with the MutationRecord::typeFlag() when deciding whether to deliver a given record to a registered observer.
>> 
>> Would it be clearer if I moved this enum out of MutationRecord.h?
> 
> Yes, I think it would. If this type is a “flags” type because of its use elsewhere, then it should probably not be a member.

Okay, I'll move it to its own file.

>>>> Source/WebCore/dom/MutationRecord.h:91

>>> 
>>> I don’t understand the use of the word “flag” here.
>> 
>> It's the same use of "flag" as Node::NodeFlags.  Do you have another suggestion?
> 
> My confusion here is caused by reusing the flag enum typedef restricted to a single flag to identify the type of a record. That probably merits a “why” comment. The word “flag” alone raises a question about why we have a type flag instead of just a type. If we want to retain this design, it might improve clarity to use the name typeAsFlag.
> 
> It’s analogous to using a set to store a single value. Because we have plenty of extra bits, the extra cost of storing a set is not an issue, but the conceptual muddle is still there.
> 
> And to be pedantic, it’s not the same as the use in Node, because there it’s “flags” not “flag” and these issues do not arise.

I think what I'll do for now is to avoid exposing this directly for now, and wait to expose it in a later patch that makes use of MutationRecords, leaving this patch to just be the basic implementation of MutationRecord.idl.

>>>> Source/WebCore/dom/MutationRecord.h:115
>>>> +    void* m_extraData;
>>> 
>>> Since we’re already paying the cost of an object for this data, seems we should just spend one more pointer in size and have this be OwnPtr<Data> and have all three of the specific classes inherit from a Data (or ExtraData or MutationData) base class with a virtual destructor. If we did that we would not need the code in ~MutationRecord and we would be properly doing adoptPtr next to each new inside the class's implementation.
>> 
>> The "cost" of an object for this data is just the pointer, but if you're fine with paying a little more memory for a little more type safety (there are still plenty of "unsafe" casts), I agree that providing an ExtraData base class is cleaner.
>> 
>> Done, and added a templated extraData() accessor to avoid putting static_cast<Foo*>(m_foo.get()) everywhere.
> 
> You say it’s “just the pointer”, but that’s not right. The cost of storing data in a separate “extra data field” is both the pointer in the mutation record and the memory allocator overhead for the extra data block. Making the extra data block one pointer larger seems affordable to me. If we really wanted to keep things to the absolute minimum, then I think we’d want to find some way to store the data in line in the main object, analogous to how a StringImpl stores its string data. It's fairly ugly to do that, though. The class CSSSelector does it and so does the CSSPrimitiveValue class, but neither is quite so complicated as this class.
> 
> Hmm, since MutationRecord objects are immutable, why don’t we make MutationRecord itself a polymorphic class? That would clean up the design a lot and save a memory block. It would make the accessor functions slower because they would be virtual function calls, but that seems worthwhile.

I was getting to the same conclusion myself, having made an ExtraData class, and agree that we may as well just make MutationRecord an abstract base class. 

This brings up the code organization question. There shouldn't be any need to manipulate the subtypes directly (they all have exactly the same interface), so one approach would be to make a MutationRecordFactory that generates each of the subclasses.  Or they could just each have their own header files, and ::create() methods.  Which would you prefer?

[Adam Klein]

To answer my own question re: code organization, I think I'll just keep the same interface as before (MutationRecord::createChildList/createCharacterData/createAttributes) and implement the subclasses in an anonymous namespace in MutationRecord.cpp.  Patch coming soon.

[Adam Klein]

Created attachment 108757 [details]
Polymorphic, no MutationTypeFlag

[Darin Adler]

Comment on attachment 108757 [details]
Polymorphic, no MutationTypeFlag

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

> Source/WebCore/dom/MutationRecord.cpp:61
> +    virtual const AtomicString& type();
> +    virtual NodeList* addedNodes() { return m_addedNodes.get(); }
> +    virtual NodeList* removedNodes() { return m_removedNodes.get(); }
> +    virtual Node* previousSibling() { return m_previousSibling.get(); }
> +    virtual Node* nextSibling() { return m_nextSibling.get(); }

These can be private.

> Source/WebCore/dom/MutationRecord.cpp:83
> +    virtual const AtomicString& type();
> +    virtual const AtomicString& attributeName() { return m_attributeName; }
> +    virtual const AtomicString& attributeNamespace() { return m_attributeName; }
> +    virtual String oldValue() { return m_oldValue; }
> +    virtual void setOldValue(const String& val) { m_oldValue = val; }

These can be private.

We normally discourage use of abbreviations that are not words, so prefer value over "val".

> Source/WebCore/dom/MutationRecord.cpp:100
> +    virtual const AtomicString& type();
> +    virtual String oldValue() { return m_oldValue; }
> +    virtual void setOldValue(const String& val) { m_oldValue = val; }

These can be private.

We normally discourage use of abbreviations that are not words, so prefer value over "val".

[Adam Klein]

Comment on attachment 108757 [details]
Polymorphic, no MutationTypeFlag

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

>> Source/WebCore/dom/MutationRecord.cpp:61
>> +    virtual Node* nextSibling() { return m_nextSibling.get(); }
> 
> These can be private.

Done

>> Source/WebCore/dom/MutationRecord.cpp:83
>> +    virtual void setOldValue(const String& val) { m_oldValue = val; }
> 
> These can be private.
> 
> We normally discourage use of abbreviations that are not words, so prefer value over "val".

Done

>> Source/WebCore/dom/MutationRecord.cpp:100
>> +    virtual void setOldValue(const String& val) { m_oldValue = val; }
> 
> These can be private.
> 
> We normally discourage use of abbreviations that are not words, so prefer value over "val".

Done

[Adam Klein]

Created attachment 108759 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 108759 [details]
Patch for landing

Clearing flags on attachment: 108759

Committed r96064: <http://trac.webkit.org/changeset/96064>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.