[MutationObservers] Implement basic subtree observation
Created attachment 111668 [details] Patch
Comment on attachment 111668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111668&action=review > Source/WebCore/dom/Node.cpp:2695 > + const size_t size = observerEntries->size(); maybe extract a static utility function here?
Comment on attachment 111668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111668&action=review >> Source/WebCore/dom/Node.cpp:2695 >> + const size_t size = observerEntries->size(); > > maybe extract a static utility function here? That's how I started, but it the inner if-statement differs depending on whether we're looking at the node itself or one of its ancestors. This means that either we do extra checking inside the loop (bad for performance), or we extract the test out into a functor (bad for readability, imho). That's why I ended up "unrolling" the code manually. Let me know if you'd prefer the functor approach (I really don't want to do any more work in the inner loop than I have to).
Comment on attachment 111668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111668&action=review >>> Source/WebCore/dom/Node.cpp:2695 >> >> maybe extract a static utility function here? > > That's how I started, but it the inner if-statement differs depending on whether we're looking at the node itself or one of its ancestors. This means that either we do extra checking inside the loop (bad for performance), or we extract the test out into a functor (bad for readability, imho). That's why I ended up "unrolling" the code manually. Let me know if you'd prefer the functor approach (I really don't want to do any more work in the inner loop than I have to). I think we should keep matches to take MutationObserverOptions and do: if (Vector<MutationObserverEntry>* observerEntries = mutationObserverEntries()) addMatchedObserverEntriesToSet(observerSet, *observerEntries, type); for (Node* node = parentNode(); node; node = node->parentNode()) { if (Vector<MutationObserverEntry>* observerEntries = node->mutationObserverEntries()) addMatchedObserverEntriesToSet(observerSet, *observerEntries, type | WebKitMutationObserver::Subtree); }
Created attachment 111856 [details] Extracted helper function
Comment on attachment 111668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111668&action=review >>>> Source/WebCore/dom/Node.cpp:2695 >>>> + const size_t size = observerEntries->size(); >>> >>> maybe extract a static utility function here? >> >> That's how I started, but it the inner if-statement differs depending on whether we're looking at the node itself or one of its ancestors. This means that either we do extra checking inside the loop (bad for performance), or we extract the test out into a functor (bad for readability, imho). That's why I ended up "unrolling" the code manually. Let me know if you'd prefer the functor approach (I really don't want to do any more work in the inner loop than I have to). > > After talking to Rafael offline, came up with a fix (just make matches() stricter). I've now extracted the loop into its own function.
Created attachment 112009 [details] Merged to r98133
Comment on attachment 112009 [details] Merged to r98133 View in context: https://bugs.webkit.org/attachment.cgi?id=112009&action=review r=me provided problems listed below are all addressed. > Source/WebCore/ChangeLog:9 > + Note that this patch only implements "basic" subtree > + semantics, not the fully robust semantics described in Nit: semantics should probably on the previous line to avoid awkward line break. > Source/WebCore/ChangeLog:22 > + (WebCore::addMatchingObservers): Static helper method for the below. I'm not sure what you're referring to by "the below". Could you spell out the function name here? > Source/WebCore/dom/Node.cpp:2703 > + if (observerEntries) { We prefer early exiting over nesting ifs especially when the if statement appears at the beginning of a function and the normal flow is to do whatever inside the if. > LayoutTests/fast/mutation/observe-subtree.html:32 > + observer = new WebKitMutationObserver(function(m) { > + mutations = m; Nit: please avoid using one-letter variable. > LayoutTests/fast/mutation/observe-subtree.html:75 > + observer = new WebKitMutationObserver(function(m) { > + mutations = m; > + }); > + observer2 = new WebKitMutationObserver(function(m) { > + mutations2 = m; > + }); Ditto about one-letter variables. > LayoutTests/fast/mutation/observe-subtree.html:116 > + observer = new WebKitMutationObserver(function(m) { > + mutations = m; > + ++calls; > + }); Ditto.
Comment on attachment 112009 [details] Merged to r98133 View in context: https://bugs.webkit.org/attachment.cgi?id=112009&action=review >> Source/WebCore/ChangeLog:9 >> + semantics, not the fully robust semantics described in > > Nit: semantics should probably on the previous line to avoid awkward line break. Done. >> Source/WebCore/ChangeLog:22 >> + (WebCore::addMatchingObservers): Static helper method for the below. > > I'm not sure what you're referring to by "the below". Could you spell out the function name here? Done. >> Source/WebCore/dom/Node.cpp:2703 >> + if (observerEntries) { > > We prefer early exiting over nesting ifs especially when the if statement appears at the beginning of a function and the normal flow is to do whatever inside the if. Done. >> LayoutTests/fast/mutation/observe-subtree.html:32 >> + mutations = m; > > Nit: please avoid using one-letter variable. Done. >> LayoutTests/fast/mutation/observe-subtree.html:75 >> + }); > > Ditto about one-letter variables. Done. >> LayoutTests/fast/mutation/observe-subtree.html:116 >> + }); > > Ditto. Done.
Created attachment 112033 [details] Patch for landing
Comment on attachment 112033 [details] Patch for landing Clearing flags on attachment: 112033 Committed r98163: <http://trac.webkit.org/changeset/98163>
All reviewed patches have been landed. Closing bug.