[webkit-reviews] review granted: [Bug 227800] Add topLayerElements() and activeModalDialog() to Document : [Attachment 433206] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Tim Nguyen (:ntim)
<ntim at apple.com>'s request for review:
Bug 227800: Add topLayerElements() and activeModalDialog() to Document
https://bugs.webkit.org/show_bug.cgi?id=227800

Attachment 433206: Patch

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




--- Comment #3 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 433206
  --> https://bugs.webkit.org/attachment.cgi?id=433206
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.