Bug 227800 – Add topLayerElements() and activeModalDialog() to Document

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Tim Nguyen (:ntim)]

Just add the DOM bits, so they can be used for bug 227534 and bug 110952, before the rendering bits get implemented.

[Radar WebKit Bug Importer]

<rdar://problem/80330046>

[Tim Nguyen (:ntim)]

Created attachment 433206 [details]
Patch

[Antti Koivisto]

Comment on attachment 433206 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:8419
> +void Document::removeFromTopLayer(Element* element)
> +{
> +    size_t position = m_topLayerElements.find(element);
> +    if (position != notFound)
> +        m_topLayerElements.remove(position);
> +
> +    element->invalidateStyle();
> +}

With ListHasSet you can just do m_topLayerElements.remove(element).

> Source/WebCore/dom/Document.cpp:8426
> +    for (auto it = m_topLayerElements.rbegin(); it != m_topLayerElements.rend(); ++it) {
> +        if (is<HTMLDialogElement>(*it))
> +            return downcast<HTMLDialogElement>((*it).get());
> +    }

for (auto& element : m_topLayerElement) {
    ...
}

> Source/WebCore/dom/Document.h:1506
> +    void addToTopLayer(Element*);
> +    void removeFromTopLayer(Element*);

These shouldn't be null so please use Element&.

> Source/WebCore/dom/Document.h:2198
> +    Vector<RefPtr<Element>> m_topLayerElements;

You might want ListHashSet for O(1) removals and membership testing (though a reasonable case shouldn't have many of these).

> Source/WebCore/html/HTMLDialogElement.cpp:126
> +            if (m_isModal)
> +                document().removeFromTopLayer(this);
> +
>              m_isModal = false;

could put m_isModal = false inside the branch as well.

[Antti Koivisto]

> for (auto& element : m_topLayerElement) {
>     ...
> }

that would be

for (auto& element : WTF::makeReversedRange(m_topLayerElement)) {
    ...
}

[Tim Nguyen (:ntim)]

Created attachment 433208 [details]
Patch

[Antti Koivisto]

Comment on attachment 433208 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:8416
> +void Document::removeFromTopLayer(Element& element)
> +{
> +    m_topLayerElements.remove(&element);
> +
> +    element.invalidateStyle();
> +}

You shouldn't invalidate style here unless the element was actually found from m_topLayerElements (this is being called unconditionally from removedFromAncestor).

[Tim Nguyen (:ntim)]

Created attachment 433209 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 433210 [details]
Patch

[Tim Nguyen (:ntim)]

Committed r279780 (239547@main): <https://commits.webkit.org/239547@main>