Bug 68824 - [MutationObservers] implement MutationRecord
Summary: [MutationObservers] implement MutationRecord
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-09-26 12:05 PDT by Adam Klein
Modified: 2011-09-26 18:43 PDT (History)
7 users (show)

See Also:


Attachments
Patch (28.55 KB, patch)
2011-09-26 13:59 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (28.44 KB, patch)
2011-09-26 14:39 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Polymorphic, no MutationTypeFlag (27.81 KB, patch)
2011-09-26 17:05 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (27.82 KB, patch)
2011-09-26 17:20 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-09-26 12:05:01 PDT
[MutationObservers] implement MutationRecord
Comment 1 Adam Klein 2011-09-26 13:59:36 PDT
Created attachment 108719 [details]
Patch
Comment 2 Darin Adler 2011-09-26 14:07:49 PDT
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.
Comment 3 Adam Klein 2011-09-26 14:38:09 PDT
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.
Comment 4 Adam Klein 2011-09-26 14:39:40 PDT
Created attachment 108724 [details]
Patch
Comment 5 Adam Klein 2011-09-26 14:51:00 PDT
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).
Comment 6 Darin Adler 2011-09-26 15:18:16 PDT
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.
Comment 7 Adam Klein 2011-09-26 16:28:36 PDT
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?
Comment 8 Adam Klein 2011-09-26 16:43:10 PDT
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.
Comment 9 Adam Klein 2011-09-26 17:05:06 PDT
Created attachment 108757 [details]
Polymorphic, no MutationTypeFlag
Comment 10 Darin Adler 2011-09-26 17:15:52 PDT
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".
Comment 11 Adam Klein 2011-09-26 17:20:05 PDT
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
Comment 12 Adam Klein 2011-09-26 17:20:22 PDT
Created attachment 108759 [details]
Patch for landing
Comment 13 WebKit Review Bot 2011-09-26 18:43:44 PDT
Comment on attachment 108759 [details]
Patch for landing

Clearing flags on attachment: 108759

Committed r96064: <http://trac.webkit.org/changeset/96064>
Comment 14 WebKit Review Bot 2011-09-26 18:43:49 PDT
All reviewed patches have been landed.  Closing bug.