No need to invalidate Attr node childNodeLists caches when its value changes since Attr Nodes can no longer have children after Bug 171688.
Created attachment 309576 [details] Patch
Created attachment 309580 [details] Patch
Comment on attachment 309580 [details] Patch I think we should hold off on these refactorings until next STP goes out to make reverting Attr node change is possible.
Created attachment 309581 [details] Patch
(In reply to Ryosuke Niwa from comment #3) > Comment on attachment 309580 [details] > Patch > > I think we should hold off on these refactorings until next STP goes out to > make reverting Attr node change is possible. Chrome and Firefox have done this a while back already without trouble so it seems very unlikely STP would cause this change to be reverted. That said, even if it does need to be reverted manually. It wouldn't been difficult. We are not talking about a lot of code.
Comment on attachment 309581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309581&action=review > Source/WebCore/dom/Node.cpp:894 > + for (auto* node = this; node; node = node->parentNode()) { > + if (auto* lists = node->nodeLists()) > + lists->invalidateCaches(attrName); This refactoring would result in a pref regression. It's very important for hasRareData() check to be inlined in this loop.
(In reply to Ryosuke Niwa from comment #6) > Comment on attachment 309581 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=309581&action=review > > > Source/WebCore/dom/Node.cpp:894 > > + for (auto* node = this; node; node = node->parentNode()) { > > + if (auto* lists = node->nodeLists()) > > + lists->invalidateCaches(attrName); > > This refactoring would result in a pref regression. It's very important for > hasRareData() check to be inlined in this loop. Oh I did not realize that. I’ll revert this part, thanks.
Comment on attachment 309581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309581&action=review > Source/WebCore/dom/Element.cpp:-1281 > - // FIXME: Refactor this so it makes some sense. Nice!
Created attachment 309607 [details] Patch
Created attachment 309608 [details] Patch
Comment on attachment 309608 [details] Patch Ryosuke made some improvement suggestions offline. I am trying them out.
Created attachment 309635 [details] Patch
Comment on attachment 309635 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309635&action=review > Source/WebCore/ChangeLog:18 > + Attr::setValueForBindings() wa thus renamed to setValue(). Its implementation Typo: wa -> was. > Source/WebCore/ChangeLog:21 > + Element::setAttributeInternal(). Maybe you can put this in the pervious line to make it read better? > Source/WebCore/dom/Node.cpp:841 > +bool Document::shouldInvalidateNodeListAndCollectionCaches() const Add inline here? > Source/WebCore/dom/Node.cpp:850 > +bool Document::shouldInvalidateNodeListAndCollectionCachesForAttribute(const QualifiedName& attrName) const Add inline here?
Created attachment 309642 [details] Patch
Comment on attachment 309642 [details] Patch Clearing flags on attachment: 309642 Committed r216632: <http://trac.webkit.org/changeset/216632>
All reviewed patches have been landed. Closing bug.