[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