Bug 171909 – Simplify relationship between Attr and Element now that Attr is childless

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

No need to invalidate Attr node childNodeLists caches when its value changes since Attr Nodes can no longer have children after Bug 171688.

[Chris Dumez]

Created attachment 309576 [details]
Patch

[Chris Dumez]

Created attachment 309580 [details]
Patch

[Ryosuke Niwa]

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.

[Chris Dumez]

Created attachment 309581 [details]
Patch

[Chris Dumez]

(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.

[Ryosuke Niwa]

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.

[Chris Dumez]

(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.

[Antti Koivisto]

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!

[Chris Dumez]

Created attachment 309607 [details]
Patch

[Chris Dumez]

Created attachment 309608 [details]
Patch

[Chris Dumez]

Comment on attachment 309608 [details]
Patch

Ryosuke made some improvement suggestions offline. I am trying them out.

[Chris Dumez]

Created attachment 309635 [details]
Patch

[Ryosuke Niwa]

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?

[Chris Dumez]

Created attachment 309642 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 309642 [details]
Patch

Clearing flags on attachment: 309642

Committed r216632: <http://trac.webkit.org/changeset/216632>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.