WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
71577
[MutationObservers] Refactor MutationObserverRegistration into it's own class that is referenced by registration points
https://bugs.webkit.org/show_bug.cgi?id=71577
Summary
[MutationObservers] Refactor MutationObserverRegistration into it's own class...
Rafael Weinstein
Reported
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.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Rafael Weinstein
Comment 1
2011-11-04 11:45:56 PDT
Created
attachment 113690
[details]
Patch
Ojan Vafai
Comment 2
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.
Rafael Weinstein
Comment 3
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.
Rafael Weinstein
Comment 4
2011-11-04 15:53:35 PDT
Created
attachment 113724
[details]
Patch for landing
WebKit Review Bot
Comment 5
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.
Early Warning System Bot
Comment 6
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
Rafael Weinstein
Comment 7
2011-11-04 16:23:21 PDT
Created
attachment 113728
[details]
Patch
WebKit Review Bot
Comment 8
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
>
WebKit Review Bot
Comment 9
2011-11-04 17:45:07 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug