Bug 70788 – [MutationObservers] Implement subtree observation of transiently disconnected nodes

• Component: DOM
• Status: RESOLVED FIXED

[Rafael Weinstein]

From the public-webapps webkit/mozilla proposal: 

Subtree Observation
If the subtree option is requested during registration, the observer
is delivered mutations which occur to a set of observed nodes which is
computed as follows:

- At the time of registration, the set includes |target| and all
descendant nodes
- At any point, if a node becomes a descendant of |target|, it is
synchronously added to the observed set
- Immediately before delivering all pending MutationRecords to the
observer, all nodes which are no longer descendants of |target| are
removed from the observed set.

[Rafael Weinstein]

Created attachment 112287 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 112287 [details]
Patch

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

> Source/WebCore/dom/WebKitMutationObserver.cpp:67
> +    TransientObservedNodesMap::iterator mapIter = m_transientObservedNodes.find(node);

Here, I can't easily tell what the types of mapIter->first and mapIter->second are.

> Source/WebCore/dom/WebKitMutationObserver.h:88
> +    typedef HashMap<Node*, HashSet<RefPtr<Node> > > TransientObservedNodesMap;
> +    TransientObservedNodesMap m_transientObservedNodes;

I'd prefer type-defing HashSet<RefPtr<Node> > (e.g. NodeHashSet) and leaving HashSet<Node*, NodeHashSet> so that the types of keys and values are self-evident wherever this type is used.

> Source/WebCore/dom/WebKitMutationObserver.h:92
> +typedef HashMap<WebKitMutationObserver*, MutationObserverOptions> MutationObserverOptionsMap;
> +

I'd prefer seeing this definition wherever MutationObserverOptionsMap is used. The definition makes the types of first and second clear whereas if we typedef-ed, I have to keep looking at this header file to know what types keys and values were.

> LayoutTests/fast/mutation/observe-subtree.html:127
> +    function finish() {

I definitely would like to see test cases where an iframe is observed and where an node is adopted from one frame to another. Of course, that could be done in a separate .html file.

[Adam Klein]

Comment on attachment 112287 [details]
Patch

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

> Source/WebCore/dom/Node.cpp:2766
> +            if (!entry.transientOfNode && entry.hasAllOptions(WebKitMutationObserver::Subtree))

!entry.transientOfNode is incorrect here, you often do want to "copy" an already-transient observation.

> Source/WebCore/dom/Node.cpp:2767
> +                entry.observer->observedSubtreeWillDisconnect(node, entry.options, this);

I think this will need to do something slightly different in the already-transient case, since you want to make the new transient entry point back to the original node, not the "new" transient one.

> Source/WebCore/dom/Node.h:598
> +    MutationRegistrationResult registerMutationObserver(PassRefPtr<WebKitMutationObserver>, MutationObserverOptions, PassRefPtr<Node>);

Please name the third argument here (for documentation purposes).  And perhaps give it a default = 0 value?  Not clear to me if that's WebKit style.

> Source/WebCore/dom/Node.h:600
> +    void unregisterMutationObserver(PassRefPtr<WebKitMutationObserver>, PassRefPtr<Node>);

Similar here as above. At the least, the argument should be named.

[Rafael Weinstein]

Created attachment 112392 [details]
Patch

[Rafael Weinstein]

Comment on attachment 112287 [details]
Patch

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

>> Source/WebCore/dom/Node.cpp:2766
>> +            if (!entry.transientOfNode && entry.hasAllOptions(WebKitMutationObserver::Subtree))
> 
> !entry.transientOfNode is incorrect here, you often do want to "copy" an already-transient observation.

good catch. done.

>> Source/WebCore/dom/Node.cpp:2767
>> +                entry.observer->observedSubtreeWillDisconnect(node, entry.options, this);
> 
> I think this will need to do something slightly different in the already-transient case, since you want to make the new transient entry point back to the original node, not the "new" transient one.

done.

>> Source/WebCore/dom/Node.h:598
>> +    MutationRegistrationResult registerMutationObserver(PassRefPtr<WebKitMutationObserver>, MutationObserverOptions, PassRefPtr<Node>);
> 
> Please name the third argument here (for documentation purposes).  And perhaps give it a default = 0 value?  Not clear to me if that's WebKit style.

done.

>> Source/WebCore/dom/Node.h:600
>> +    void unregisterMutationObserver(PassRefPtr<WebKitMutationObserver>, PassRefPtr<Node>);
> 
> Similar here as above. At the least, the argument should be named.

done

>> Source/WebCore/dom/WebKitMutationObserver.cpp:67
>> +    TransientObservedNodesMap::iterator mapIter = m_transientObservedNodes.find(node);
> 
> Here, I can't easily tell what the types of mapIter->first and mapIter->second are.

done

>> Source/WebCore/dom/WebKitMutationObserver.h:88
>> +    TransientObservedNodesMap m_transientObservedNodes;
> 
> I'd prefer type-defing HashSet<RefPtr<Node> > (e.g. NodeHashSet) and leaving HashSet<Node*, NodeHashSet> so that the types of keys and values are self-evident wherever this type is used.

done

>> Source/WebCore/dom/WebKitMutationObserver.h:92
>> +
> 
> I'd prefer seeing this definition wherever MutationObserverOptionsMap is used. The definition makes the types of first and second clear whereas if we typedef-ed, I have to keep looking at this header file to know what types keys and values were.

done

>> LayoutTests/fast/mutation/observe-subtree.html:127
>> +    function finish() {
> 
> I definitely would like to see test cases where an iframe is observed and where an node is adopted from one frame to another. Of course, that could be done in a separate .html file.

Not sure what your concern is here. I'm going to upload the patch without this and we can talk more about what case you're worried about.

[Rafael Weinstein]

Created attachment 112598 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 112598 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        This patch adds support for observing "transiently disconnected" nodes from subtree

I think we normally call a node being attached or detached from a document or a subtree so it'll be nice to keep the terminology consistent here especially because we use disconnect() to stop all observations.

Also, it'll be nice to explain what "transient" means in the change log.

> Source/WebCore/dom/ChildListMutationScope.cpp:76
> -    Vector<WebKitMutationObserver*> m_observers;
> +    HashMap<WebKitMutationObserver*, MutationObserverOptions> m_observers;

I'm a bit confused as to why we need HashMap here. If I understand correctly, we can just have:
Vector<WebKitMutationObserverEntry> m_observers or HashSet<WebKitMutationObserverEntry> m_observers ?

The fact we're storing keys in HashMap confuses me. I think using WebKitMutationObserverEntry makes the semantics clearer.

> Source/WebCore/dom/ContainerNode.cpp:1147
>          mutation.willRemoveChild(c);
> +        c->notifySubtreeObserversOfDisconnection();

It seems more natural for mutation.willRemoveChild to take care of things rather than adding another function call here.

> Source/WebCore/dom/Node.cpp:2730
> +Node::MutationRegistrationResult Node::registerMutationObserver(PassRefPtr<WebKitMutationObserver> observer, MutationObserverOptions options, Node* transientOfNode)

I don't quite understand what "transientOfNode" means. Is that a node in some transient state? Or is it some transient object attached to a node?

> Source/WebCore/dom/Node.cpp:2761
> +void Node::notifySubtreeObserversOfDisconnection()

Can we move this function to WebKitMutationObserver.cpp? or make this function static local since it's only called in Node.cpp?
In general, we try to avoid adding member functions to Node or even to Node.cpp to avoid making it a God class.

Also, detach is a better word to describe a node that has been removed from a container node.

> Source/WebCore/dom/Node.cpp:2771
> +            if (entry.hasAllOptions(WebKitMutationObserver::Subtree)) {

Can we do
if (!entry.hasAllOptions(WebKitMutationObserver::Subtree))
    continue;
instead?

> Source/WebCore/dom/NodeRareData.h:96
> +        , transientOfNode(node)

I think we really need to rename this variable.

> Source/WebCore/dom/NodeRareData.h:112
> +    Node* transientOfNode; // This doesn't need to be a RefPtr because the observer will be holding a RefPtr to the same node at least for the lifetime of this Entry in its m_transientObservedNodes map.

This is a really long line. I'd prefer splitting it into two lines.

> Source/WebCore/dom/WebKitMutationObserver.cpp:118
> +void WebKitMutationObserver::observedSubtreeWillDisconnect(PassRefPtr<Node> prpObservedNode, MutationObserverOptions options, PassRefPtr<Node> prpDisconnectingNode)

To match function naming function, how about willRemoveNodeInObservedSubtree?

I think DisconnectingNode needs mode descriptive name here. How about ancestorToBeDetached or observedAncestorToBeDetached?

> Source/WebCore/dom/WebKitMutationObserver.cpp:130
> +    if (iter == m_transientObservedNodes.end()) {
> +        OwnPtr<NodeHashSet> set = adoptPtr(new NodeHashSet());
> +        set->add(disconnectingNode);
> +        m_transientObservedNodes.set(observedNode, set.leakPtr());
> +    } else
> +        iter->second->add(disconnectingNode);

I'd negate the condition in the if and exit early as in:
if (iter != m_transientObservedNodes.end()) {
    iter->second->add(disconnectingNode);
    return;
}

> Source/WebCore/dom/WebKitMutationObserver.cpp:136
> +    for (HashMap<RefPtr<Node>, NodeHashSet*>::iterator iter = m_transientObservedNodes.begin(); iter != m_transientObservedNodes.end(); ++iter)
> +        unregisterTransientEntries(adoptPtr(iter->second), this, iter->first.get());

Might make sense to wrap HashMap<RefPtr<Node>, NodeHashSet*> in some class and put these logic in that class instead.

> LayoutTests/fast/mutation/observe-subtree.html:143
> +        debug('Testing that transiently disconnected nodes are still observed via subtree.');

Particularly, in this context, I initially thought "disconnected nodes" are those nodes observers stopped observing after a call to disconnect().

> LayoutTests/fast/mutation/observe-subtree.html:157
> +        observer.observe(div, {attributes: true, characterData: true, subtree: true});
> +        subDiv.setAttribute('foo', 'bar');
> +        div.removeChild(subDiv);
> +        subDiv.setAttribute('test', 'test');
> +        setTimeout(checkDeliveredAndChangeAgain, 0);

Can we also test that if we are observing div in <div><span><b>hello</b></span></div>, and "hello" was modified after b is detached after span is detached from div? (i.e. transient node's subtree was detached).

[Rafael Weinstein]

Comment on attachment 112598 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        This patch adds support for observing "transiently disconnected" nodes from subtree
> 
> I think we normally call a node being attached or detached from a document or a subtree so it'll be nice to keep the terminology consistent here especially because we use disconnect() to stop all observations.
> 
> Also, it'll be nice to explain what "transient" means in the change log.

done

>> Source/WebCore/dom/ChildListMutationScope.cpp:76

> 
> I'm a bit confused as to why we need HashMap here. If I understand correctly, we can just have:
> Vector<WebKitMutationObserverEntry> m_observers or HashSet<WebKitMutationObserverEntry> m_observers ?
> 
> The fact we're storing keys in HashMap confuses me. I think using WebKitMutationObserverEntry makes the semantics clearer.

This is a refactor which is anticipating needing to support attributeOldValue, characterDataOldValue. The idea here is that this is not just a list of the interested observers, but also the *union* of options of their matching registrations. It says not only *who* the mutation record should get delivered to, but also what the mutation record needs to look like for that observer.

To be fair, this class doesn't care because childList doesn't have any options which affect it. It just seemed silly to pre-filter this to a Vector. If you feel strongly, I'll do that.

>> Source/WebCore/dom/ContainerNode.cpp:1147
>> +        c->notifySubtreeObserversOfDisconnection();
> 
> It seems more natural for mutation.willRemoveChild to take care of things rather than adding another function call here.

I'd prefer not to do that. It violates a nice seperation of concerns. ChildLIstMutationScope doesn't play any part in maintaining observation registration. Doing so would give it such responsibility.

>> Source/WebCore/dom/Node.cpp:2730
>> +Node::MutationRegistrationResult Node::registerMutationObserver(PassRefPtr<WebKitMutationObserver> observer, MutationObserverOptions options, Node* transientOfNode)
> 
> I don't quite understand what "transientOfNode" means. Is that a node in some transient state? Or is it some transient object attached to a node?

per discussion: changed to registrationNode

>> Source/WebCore/dom/Node.cpp:2761
>> +void Node::notifySubtreeObserversOfDisconnection()
> 
> Can we move this function to WebKitMutationObserver.cpp? or make this function static local since it's only called in Node.cpp?
> In general, we try to avoid adding member functions to Node or even to Node.cpp to avoid making it a God class.
> 
> Also, detach is a better word to describe a node that has been removed from a container node.

This method needs the node's registry, so it can't be handled externally (e.g. by WebKitMutationObserver). The ideal place to do this would be Node::willRemove() but that appears not to be safe because ContainerNode::willRemove calls willRemove on all of its children.

Maybe we can talk about this further. I've changed the name.

>> Source/WebCore/dom/Node.cpp:2771
>> +            if (entry.hasAllOptions(WebKitMutationObserver::Subtree)) {
> 
> Can we do
> if (!entry.hasAllOptions(WebKitMutationObserver::Subtree))
>     continue;
> instead?

done.

>> Source/WebCore/dom/NodeRareData.h:96
>> +        , transientOfNode(node)
> 
> I think we really need to rename this variable.

done. now registrationNode (per discussion)

>> Source/WebCore/dom/NodeRareData.h:112
>> +    Node* transientOfNode; // This doesn't need to be a RefPtr because the observer will be holding a RefPtr to the same node at least for the lifetime of this Entry in its m_transientObservedNodes map.
> 
> This is a really long line. I'd prefer splitting it into two lines.

done

>> Source/WebCore/dom/WebKitMutationObserver.cpp:118
>> +void WebKitMutationObserver::observedSubtreeWillDisconnect(PassRefPtr<Node> prpObservedNode, MutationObserverOptions options, PassRefPtr<Node> prpDisconnectingNode)
> 
> To match function naming function, how about willRemoveNodeInObservedSubtree?
> 
> I think DisconnectingNode needs mode descriptive name here. How about ancestorToBeDetached or observedAncestorToBeDetached?

changed. let me know

>> Source/WebCore/dom/WebKitMutationObserver.cpp:130
>> +        iter->second->add(disconnectingNode);
> 
> I'd negate the condition in the if and exit early as in:
> if (iter != m_transientObservedNodes.end()) {
>     iter->second->add(disconnectingNode);
>     return;
> }

done

>> Source/WebCore/dom/WebKitMutationObserver.cpp:136
>> +        unregisterTransientEntries(adoptPtr(iter->second), this, iter->first.get());
> 
> Might make sense to wrap HashMap<RefPtr<Node>, NodeHashSet*> in some class and put these logic in that class instead.

not done -- per discussion

>> LayoutTests/fast/mutation/observe-subtree.html:143
>> +        debug('Testing that transiently disconnected nodes are still observed via subtree.');
> 
> Particularly, in this context, I initially thought "disconnected nodes" are those nodes observers stopped observing after a call to disconnect().

done

>> LayoutTests/fast/mutation/observe-subtree.html:157
>> +        setTimeout(checkDeliveredAndChangeAgain, 0);
> 
> Can we also test that if we are observing div in <div><span><b>hello</b></span></div>, and "hello" was modified after b is detached after span is detached from div? (i.e. transient node's subtree was detached).

that case is covered below in the detailed version

[Rafael Weinstein]

Created attachment 112626 [details]
Patch

[WebKit Review Bot]

Attachment 112626 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295

Died at Tools/Scripts/update-webkit line 157.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 112626 [details]
Patch

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

I would have r+ed if you were a committer but sadly r- for now.

> Source/WebCore/dom/Node.cpp:553
> +    Vector<MutationObserverRegistration>* registry = mutationObserverRegistry();
> +    if (registry) {

registry should be declared inside if as in:
if (Vector<MutationObserverRegistration>* registry = mutationObserverRegistry()) {

> Source/WebCore/dom/Node.cpp:2718
> +            pair<HashMap<WebKitMutationObserver*, MutationObserverOptions>::iterator, bool> result = observers.add(entry.observer.get(), entry.options);
> +            if (!result.second)
> +                result.first->second |= entry.options;

This code is unreadable to me. I'd prefer approach like:
if (HashMap<WebKitMutationObserver*, MutationObserverOptions>::iterator it = observers.find(entry.observer.get()))
    it->options |= entry.options;
else
    observers.add(entry.observer.get(), entry.options);
This code shouldn't be much slower since we're using a hash map but it reads much better.

> Source/WebCore/dom/Node.cpp:2732
> +    Vector<MutationObserverRegistration>* registry = ensureRareData()->ensureMutationObserverRegistry();

Can we define a reference here instead?

> Source/WebCore/dom/Node.cpp:2741
> +    (*registry)[index].options = registration.options;

So that we can avoid these awkward de-referencing.

> Source/WebCore/dom/Node.cpp:2761
> +void Node::createTransientMutationObservers()

Hm... this function isn't really creating observers though. How about registerTransientMutationObservations or startTransientMutationObservations?

> Source/WebCore/dom/Node.cpp:2774
> +            RefPtr<Node> observedNode = registration.registrationNode;

We can use a raw pointer here, can't we?

> Source/WebCore/dom/WebKitMutationObserver.cpp:59
> +static void unregisterTransientRegistrations(PassOwnPtr<NodeHashSet> popTransientNodes, WebKitMutationObserver* observer, Node* registrationNode)

inline keyword?

> Source/WebCore/dom/WebKitMutationObserver.cpp:61
> +    OwnPtr<NodeHashSet> transientNodes = popTransientNodes;

We don't need this local variable. This is a two-line function and the correctness is clear without it.

> Source/WebCore/dom/WebKitMutationObserver.cpp:141
> +void WebKitMutationObserver::clearTransientEntries()
> +{
> +    for (HashMap<RefPtr<Node>, NodeHashSet*>::iterator iter = m_transientObservedNodes.begin(); iter != m_transientObservedNodes.end(); ++iter)
> +        unregisterTransientRegistrations(adoptPtr(iter->second), this, iter->first.get());
> +
> +    m_transientObservedNodes.clear();
> +}

There are bunch of different naming conventions used here. clearTransientEntries, m_transientObservedNodes, unregisterTransientRegistrations.
We should make them consistent. How about m_transientObservedNodes, clearAllTransientObservations, stopTransientObservations/unregisterTransientObservers?
I'd like to avoid using the term "registration" for the third one since that term is associated with the rare data in Node and the function happens to destroy NodeHashSet as well.

> LayoutTests/ChangeLog:7
> +

Would you care to describe what kind of test cases you're adding here?

> LayoutTests/fast/mutation/observe-subtree.html:161
> +        debug('...both changes should be received. Change disconnected subDiv again.');

Can we s/disconnect/detach/g?

> LayoutTests/fast/mutation/observe-subtree.html:190
> +        debug('...reattached subtree should now be observable. Try disconnecting and re-observing.');

Ditto.

[Rafael Weinstein]

Comment on attachment 112626 [details]
Patch

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

>> Source/WebCore/dom/Node.cpp:553
>> +    if (registry) {
> 
> registry should be declared inside if as in:
> if (Vector<MutationObserverRegistration>* registry = mutationObserverRegistry()) {

done

>> Source/WebCore/dom/Node.cpp:2718
>> +                result.first->second |= entry.options;
> 
> This code is unreadable to me. I'd prefer approach like:
> if (HashMap<WebKitMutationObserver*, MutationObserverOptions>::iterator it = observers.find(entry.observer.get()))
>     it->options |= entry.options;
> else
>     observers.add(entry.observer.get(), entry.options);
> This code shouldn't be much slower since we're using a hash map but it reads much better.

Note that this was more readable when HashMap<WebKitMutationObserver*, MutationObserverOptions> was typedef'd -- partly for this reason -- because it gets unreadable. You wanted it untypedef'd in the last review.

This code is in a critical path. I'd prefer not to incur an unnecessary hash lookup.

Also, i've inverted the if and *continue* early -- to make it a bit clearer.

>> Source/WebCore/dom/Node.cpp:2732
>> +    Vector<MutationObserverRegistration>* registry = ensureRareData()->ensureMutationObserverRegistry();
> 
> Can we define a reference here instead?

not done -- per discussion

>> Source/WebCore/dom/Node.cpp:2741
>> +    (*registry)[index].options = registration.options;
> 
> So that we can avoid these awkward de-referencing.

changed to registry->at(index)

>> Source/WebCore/dom/Node.cpp:2761
>> +void Node::createTransientMutationObservers()
> 
> Hm... this function isn't really creating observers though. How about registerTransientMutationObservations or startTransientMutationObservations?

changed to notifyMutationObserversNodeWillDetach

>> Source/WebCore/dom/Node.cpp:2774
>> +            RefPtr<Node> observedNode = registration.registrationNode;
> 
> We can use a raw pointer here, can't we?

done

>> Source/WebCore/dom/WebKitMutationObserver.cpp:59
>> +static void unregisterTransientRegistrations(PassOwnPtr<NodeHashSet> popTransientNodes, WebKitMutationObserver* observer, Node* registrationNode)
> 
> inline keyword?

done.

>> Source/WebCore/dom/WebKitMutationObserver.cpp:61
>> +    OwnPtr<NodeHashSet> transientNodes = popTransientNodes;
> 
> We don't need this local variable. This is a two-line function and the correctness is clear without it.

done

>> Source/WebCore/dom/WebKitMutationObserver.cpp:141
>> +}
> 
> There are bunch of different naming conventions used here. clearTransientEntries, m_transientObservedNodes, unregisterTransientRegistrations.
> We should make them consistent. How about m_transientObservedNodes, clearAllTransientObservations, stopTransientObservations/unregisterTransientObservers?
> I'd like to avoid using the term "registration" for the third one since that term is associated with the rare data in Node and the function happens to destroy NodeHashSet as well.

I've got clearAllTransientObservations, and clearTransientObservationsForRegistration (which takes a registrationNode as it's second arg).

how's that?

>> LayoutTests/ChangeLog:7
>> +
> 
> Would you care to describe what kind of test cases you're adding here?

done

>> LayoutTests/fast/mutation/observe-subtree.html:161
>> +        debug('...both changes should be received. Change disconnected subDiv again.');
> 
> Can we s/disconnect/detach/g?

done.

>> LayoutTests/fast/mutation/observe-subtree.html:190
>> +        debug('...reattached subtree should now be observable. Try disconnecting and re-observing.');
> 
> Ditto.

done

[Rafael Weinstein]

Created attachment 112715 [details]
Patch

[Ryosuke Niwa]

(In reply to comment #12)
> >> Source/WebCore/dom/Node.cpp:2718
> >> +                result.first->second |= entry.options;
> > 
> > This code is unreadable to me. I'd prefer approach like:
> > if (HashMap<WebKitMutationObserver*, MutationObserverOptions>::iterator it = observers.find(entry.observer.get()))
> >     it->options |= entry.options;
> > else
> >     observers.add(entry.observer.get(), entry.options);
> > This code shouldn't be much slower since we're using a hash map but it reads much better.
> 
> Note that this was more readable when HashMap<WebKitMutationObserver*, MutationObserverOptions> was typedef'd -- partly for this reason -- because it gets unreadable. You wanted it untypedef'd in the last review.

No, typedef'ing HashMap doesn't help here at all. The problem is with:
if (!result.second)
    result.first->second |= entry.options;

I literally had to look up the definition of HashMap to know what the second value of the pair indicates whether a new value is added not. And it's impossible to tell what "first->second" means without looking up definitions of struct, etc...

> This code is in a critical path. I'd prefer not to incur an unnecessary hash lookup.

I'd argue that we should do:
if (HashMap<WebKitMutationObserver*, MutationObserverOptions>::iterator it = observers.find(entry.observer.get()))
    it->options |= entry.options;
else
    observers.add(entry.observer.get(), entry.options); 

unless it shows up on a profiler result (Shark, Instruments, etc...). If it indeed DOES appear in the profiler, I'd declare local variable to document the meaning of each variables as in:

pair<HashMap<WebKitMutationObserver*, MutationObserverOptions>::iterator, bool> result = observers.add(entry.observer.get(), entry.options);
const bool newItemAdded = result.second;
if (!newItemAdded) {
    MutationObserverOptions& optionsInRecord = result.first->second;
    optionsInRecord |= entry.options;
}

And I'd like see how much it's affecting the performance in seconds or percentage.

[Ryosuke Niwa]

Comment on attachment 112715 [details]
Patch

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

The rest of the patch looks great to me!

> Source/WebCore/dom/NodeRareData.h:116
> +    // least for the lifetime of this Entry in its m_transientObservedNodes map.

s/Entry/Registration/

[Ryosuke Niwa]

Comment on attachment 112715 [details]
Patch

Alright, apparently, there are lots of instances of first->second so I guess we'll have to live with them.

[Ryosuke Niwa]

Comment on attachment 112715 [details]
Patch

Oops, we need to fix s/Entry/Registration/ first.

[Rafael Weinstein]

Created attachment 112748 [details]
Patch

[Rafael Weinstein]

Comment on attachment 112715 [details]
Patch

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

>> Source/WebCore/dom/NodeRareData.h:116
>> +    // least for the lifetime of this Entry in its m_transientObservedNodes map.
> 
> s/Entry/Registration/

done

[WebKit Review Bot]

Comment on attachment 112748 [details]
Patch

Clearing flags on attachment: 112748

Committed r98659: <http://trac.webkit.org/changeset/98659>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.