Bug 71577 - [MutationObservers] Refactor MutationObserverRegistration into it's own class that is referenced by registration points
Summary: [MutationObservers] Refactor MutationObserverRegistration into it's own class...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rafael Weinstein
URL:
Keywords:
Depends on:
Blocks: 68729
  Show dependency treegraph
 
Reported: 2011-11-04 11:38 PDT by Rafael Weinstein
Modified: 2011-11-07 13:36 PST (History)
8 users (show)

See Also:


Attachments
Patch (42.85 KB, patch)
2011-11-04 11:45 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch for landing (185.51 KB, patch)
2011-11-04 15:53 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (43.58 KB, patch)
2011-11-04 16:23 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rafael Weinstein 2011-11-04 11:38:10 PDT
Right now, copies are made when transient registrations are created. This isn't terrible now, but when the registration needs to hold the attributeFilter hash, we want to avoid making lots of copies of that.
Comment 1 Rafael Weinstein 2011-11-04 11:45:56 PDT
Created attachment 113690 [details]
Patch
Comment 2 Ojan Vafai 2011-11-04 15:01:21 PDT
Comment on attachment 113690 [details]
Patch

Just a bunch of comment and/or other nits.

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

> Source/WebCore/ChangeLog:11
> +        MutationObserverRegistration is now owned by the node which is observed. If transient
> +        registrations are created, they hold a reference to this object.

This is the appropriate place for all your comments in the code. :) Would be good if you could make this description more detailed. Specifically, a condensed version of the description you gave to me in person about how this works would be perfect.

> Source/WebCore/dom/ChildListMutationScope.cpp:44
> +#include <wtf/text/AtomicString.h>

Is this needed?

> Source/WebCore/dom/MutationObserverRegistration.cpp:64
> +

Nit: don't see the need for this line-break.

> Source/WebCore/dom/MutationObserverRegistration.cpp:77
> +        m_transientRegistrationNodes = adoptPtr(new NodeHashSet);
> +        m_registrationNodeKeepAlive = m_registrationNode; // Balanced in clearTransientRegistrations.

Should this ASSERT(!m_registrationNodeKeepAlive)?

> Source/WebCore/dom/MutationObserverRegistration.cpp:99
> +    m_registrationNode->unregisterMutationObserver(this); // This call will cause this object to be deleted.

Webkit is generally anti "what" comments. Here and above. What happens when the code is refactored so this is no longer true? It's very likely whoever does the refactor won't notice this comment.

I needed your in person descriptions to understand these comments anyways. :)

Might be worth making the comment a bit more explicit, like:
m_registrationNode->unregisterMutationObserver(this);
// The above line causes this object to be deleted, so don't do any more in this function.

> Source/WebCore/dom/MutationObserverRegistration.h:65
> +    Node* m_registrationNode; // Back-pointer to the Node which owns this registration.
> +    RefPtr<Node> m_registrationNodeKeepAlive; // This registration takes reference to its owning node while it has transient registrations.

These comments aren't super useful. Again, without you explaining to me how this all hooks together, these comments didn't actually help me understand it.

> Source/WebCore/dom/Node.cpp:2736
> +        collectMatchingObserversForMutation(observers, node, type);

Nit: I don't really see the benefit of moving this into a helper function. I'd just inline the function code here. Your call though.

> Source/WebCore/dom/Node.cpp:2762
>      size_t index = registry->find(registration);
>      ASSERT(index != notFound);
>      if (index == notFound)

Should this just be an assert as in 2780?

> Source/WebCore/dom/WebKitMutationObserver.h:88
> +    HashSet<MutationObserverRegistration*> m_registrations; // Back-pointers to registrations which are holding references to this observer.

ditto re: comment.
Comment 3 Rafael Weinstein 2011-11-04 15:44:41 PDT
Comment on attachment 113690 [details]
Patch

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

>> Source/WebCore/dom/ChildListMutationScope.cpp:44
>> +#include <wtf/text/AtomicString.h>
> 
> Is this needed?

done. missed from splitting out attributeFilter patch.

>> Source/WebCore/dom/MutationObserverRegistration.cpp:64
>> +
> 
> Nit: don't see the need for this line-break.

done

>> Source/WebCore/dom/MutationObserverRegistration.cpp:77
>> +        m_registrationNodeKeepAlive = m_registrationNode; // Balanced in clearTransientRegistrations.
> 
> Should this ASSERT(!m_registrationNodeKeepAlive)?

done

>> Source/WebCore/dom/MutationObserverRegistration.cpp:99
>> +    m_registrationNode->unregisterMutationObserver(this); // This call will cause this object to be deleted.
> 
> Webkit is generally anti "what" comments. Here and above. What happens when the code is refactored so this is no longer true? It's very likely whoever does the refactor won't notice this comment.
> 
> I needed your in person descriptions to understand these comments anyways. :)
> 
> Might be worth making the comment a bit more explicit, like:
> m_registrationNode->unregisterMutationObserver(this);
> // The above line causes this object to be deleted, so don't do any more in this function.

done

>> Source/WebCore/dom/MutationObserverRegistration.h:65
>> +    RefPtr<Node> m_registrationNodeKeepAlive; // This registration takes reference to its owning node while it has transient registrations.
> 
> These comments aren't super useful. Again, without you explaining to me how this all hooks together, these comments didn't actually help me understand it.

removed

>> Source/WebCore/dom/Node.cpp:2736
>> +        collectMatchingObserversForMutation(observers, node, type);
> 
> Nit: I don't really see the benefit of moving this into a helper function. I'd just inline the function code here. Your call though.

done

>> Source/WebCore/dom/Node.cpp:2762
>>      if (index == notFound)
> 
> Should this just be an assert as in 2780?

This needs to exit early if the ASSERT fails. The removal below will ignore the attempt to remove a non-existing element.
Comment 4 Rafael Weinstein 2011-11-04 15:53:35 PDT
Created attachment 113724 [details]
Patch for landing
Comment 5 WebKit Review Bot 2011-11-04 15:54:22 PDT
Comment on attachment 113724 [details]
Patch for landing

Rejecting attachment 113724 [details] from commit-queue.

rafaelw@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 6 Early Warning System Bot 2011-11-04 16:07:28 PDT
Comment on attachment 113724 [details]
Patch for landing

Attachment 113724 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10336102
Comment 7 Rafael Weinstein 2011-11-04 16:23:21 PDT
Created attachment 113728 [details]
Patch
Comment 8 WebKit Review Bot 2011-11-04 17:45:01 PDT
Comment on attachment 113728 [details]
Patch

Clearing flags on attachment: 113728

Committed r99338: <http://trac.webkit.org/changeset/99338>
Comment 9 WebKit Review Bot 2011-11-04 17:45:07 PDT
All reviewed patches have been landed.  Closing bug.