[webkit-reviews] review granted: [Bug 207816] Revise DOM iterators implementation and usage : [Attachment 391461] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Feb 22 09:38:55 PST 2020


Antti Koivisto <koivisto at iki.fi> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 207816: Revise DOM iterators implementation and usage
https://bugs.webkit.org/show_bug.cgi?id=207816

Attachment 391461: Patch

https://bugs.webkit.org/attachment.cgi?id=391461&action=review




--- Comment #27 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 391461
  --> https://bugs.webkit.org/attachment.cgi?id=391461
Patch

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

I like red patches.

> Source/WebCore/ChangeLog:39
> +	   - Removed the duplicate descendant iterator, keeping the one that
matches
> +	     the style of the ancestor and child iterators.
> +	   - Removed the non-template elementAncestors, elementChildren,
elementDescendants,
> +	     and elementLineage functions and changed callers to use
xxxOfType<Element> instead.
> +	   - Renamed "IteratorAdapter" templates to "Range", choosing that term
to match the
> +	     upcoming C++20 Ranges library and range-based for loops.
> +	   - Changed the iterators to use an actual "nullptr" for end,
following the "sentinel"
> +	     design pattern from the Ranges library. Still kept a tiny bit of
using an iterator
> +	     for end around, only so we can use iterator library functions like
std::distance
> +	     while waiting for std::ranges::distance, which is compatible with
sentinels.
> +	   - Implemented const correctness by using const types instead of
separate "Const"
> +	     class templates. This cut down on source code size a lot. These
element iterators
> +	     don't need whole separate templates to implement the const
correctness the way
> +	     collection classes like HashMap do.
> +	   - Improved some other details, like using more const and constexpr
on members.
> +	     All the functions on a range are const, because the range itself
doesn't ever
> +	     get modified, and all functions on an iterator are also const,
because only
> +	     operations like ++ and -- actually modify the iterator.
> +	   - For now at least, removed extra code we don't need in practice. We
never need to
> +	     compare iterators to each other except when iterating a range, for
example, so
> +	     kept the != used for range iteration but not ==.
> +	   - Simplified the HTMLCollection implementations by taking advantage
of the null-
> +	     based and sentinel designs. There are various places where we can
write
> +	     simpler code and pass around fewer arguments.
> +	   - Added a new descendantsOfType template that takes a predicate and
filters to only
> +	     the elements that match that predicate. Similar concept to how we
implement HTML
> +	     collections, and possibly could be used even more eventually.
> +	   - Use std::iterator in ElementIterator so we don't need to do that
in derived
> +	     classes. Also made more of ElementIterator protected to make it
more explicit
> +	     that it's an abstract class template and not something to be used
directly.

This is informative!

> Source/WebCore/dom/CollectionIndexCache.h:45
> -    explicit CollectionIndexCache(const Collection&);
> +    CollectionIndexCache();
>  
>      typedef typename std::iterator_traits<Iterator>::value_type NodeType;
>  
>      unsigned nodeCount(const Collection&);
>      NodeType* nodeAt(const Collection&, unsigned index);
>  
> -    bool hasValidCache(const Collection& collection) const { return
m_current != collection.collectionEnd() || m_nodeCountValid || m_listValid; }
> -    void invalidate(const Collection&);
> +    bool hasValidCache() const { return m_current || m_nodeCountValid ||
m_listValid; }
> +    void invalidate();

This sort of work that builds on top of the refactoring would be better done as
an incremental step (making the patch less unwieldy).

> Source/WebCore/dom/ElementAncestorIterator.h:54
> +    static constexpr std::nullptr_t end() { return nullptr; }

This is a nice C++17 feature.

> Source/WebCore/html/HTMLTableElement.h:98
> -    RefPtr<StyleProperties> m_sharedCellStyle;
> +    mutable RefPtr<StyleProperties> m_sharedCellStyle;

Could probably be mutable RefPtr<const StyleProperties>

> Source/WebCore/xml/parser/XMLDocumentParser.cpp:296
> +    if (contextElement) {
> +	   bool stopLookingForDefaultNamespaceURI = false;
> +
> +	   for (auto& element : lineageOfType<Element>(*contextElement)) {
> +	       if (is<SVGForeignObjectElement>(element))
> +		   stopLookingForDefaultNamespaceURI = true;
> +	       else if (!stopLookingForDefaultNamespaceURI)
> +		   defaultNamespaceURI = element.namespaceURI();
> +
> +	       if (!element.hasAttributes())
> +		   continue;
> +
> +	       for (const Attribute& attribute : element.attributesIterator())
{
> +		   if (attribute.prefix() == xmlnsAtom())
> +		       prefixToNamespaceMap.set(attribute.localName(),
attribute.value());
> +		   else if (!stopLookingForDefaultNamespaceURI &&
attribute.prefix() == xmlnsAtom())
> +		       defaultNamespaceURI = attribute.value();
> +	       }
>	   }
>      }

This looks likea good candidate for a lambda.


More information about the webkit-reviews mailing list