[webkit-reviews] review denied: [Bug 84796] Implement the new stacking layer needed by the Fullscreen API and the new <dialog> element : [Attachment 168181] represent top layer in render tree as well

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Matt Falkenhagen
<falken at chromium.org>'s request for review:
Bug 84796: Implement the new stacking layer needed by the Fullscreen API and
the new <dialog> element
https://bugs.webkit.org/show_bug.cgi?id=84796

Attachment 168181: represent top layer in render tree as well
https://bugs.webkit.org/attachment.cgi?id=168181&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=168181&action=review


I'd like to see some tests to actually ensure that the rendering is correct,
e.g. tests with high z-index elements and dialogs, as well as nested dialogs. I
think you also need to test removing showing dialog elements (esp those with
dialog children) via JS.

> Source/WebCore/dom/Document.h:1467
> +    Vector<RefPtr<Element> > m_topLayer;

Should be m_topLayerElements

> Source/WebCore/dom/NodeRareData.h:327
> +    bool isInTopLayer() { return m_isInTopLayer; }

Should be const

> Source/WebCore/html/HTMLDialogElement.cpp:56
> +    setIsInTopLayer(false);

Is close() always called when a <dialog> gets destroyed? What if it's removed
from the DOM while showing?

> Source/WebCore/rendering/RenderLayer.cpp:4635
> +#if ENABLE(DIALOG_ELEMENT)
> +    if (isRootLayer()) {
> +	   RenderObject* view = renderer()->view();
> +	   for (RenderObject* child = view->firstChild(); child; child =
child->nextSibling()) {
> +	       Element* childElement = child->node()->isElementNode() ?
toElement(child->node()) : 0;
> +	       if (childElement && childElement->isInTopLayer()) {
> +		   RenderLayer* layer = toRenderBoxModelObject(child)->layer();

> +		   m_posZOrderList->append(layer);
> +	       }
> +	   }
> +    }
> +#endif

I'm not sure why you need this code here, since your createRendererIfNeeded()
changes ensure that the dialog renderers are parented in the view. Or does this
ensure that they get appended after z-index sorting? Some comments would help.