[webkit-reviews] review denied: [Bug 84796] Implement the new stacking layer needed by the Fullscreen API and the new <dialog> element : [Attachment 165969] WIP patch

[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 165969: WIP patch
https://bugs.webkit.org/attachment.cgi?id=165969&action=review

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


> Source/WebCore/dom/Document.h:1167
> +    Vector<RefPtr<Element> >& topLayer() { return m_topLayer; }

This function is confusingly named. It doesn't return a layer; it returns a
vector of elements. Maybe elementsInTopLayer(), or topLayerElements()?

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

What's the mechanism for removing elements from this array when they go away?
Do we assume that HTMLDialogElement is the only element for now that can get
into this list?

> Source/WebCore/dom/Element.h:420
> +    virtual void setIsInTopLayer(bool);

I think this should be setInTopLayer()

> Source/WebCore/dom/ElementRareData.h:125
>  #if ENABLE(FULLSCREEN_API)
>      bool m_containsFullScreenElement;
>  #endif
> +
> +#if ENABLE(DIALOG_ELEMENT)
> +    bool m_isInTopLayer;
> +#endif

Should probably group this bools with the previous two bools, and make them
part of the bitfield.

> Source/WebCore/rendering/RenderBox.h:50
> +#if ENABLE(DIALOG_ELEMENT)
> +	   || (node() && node()->isElementNode() &&
toElement(node())->isInTopLayer())
> +#endif

I worry slightly that this will affect performance. Don't top layer elements
have non-auto z-index by virtue of being stacking contexts anyway?

> Source/WebCore/rendering/RenderLayer.cpp:4606
> +    if (isRootLayer()) {
> +	   Vector<RefPtr<Element> >& topLayer =
renderer()->document()->topLayer();
> +	   for (Vector<RefPtr<Element> >::iterator i = topLayer.begin(); i !=
topLayer.end(); ++i) {
> +	       RenderLayer* layer =
toRenderBoxModelObject((*i)->renderer())->layer();
> +	       m_posZOrderList->append(layer);
> +	   }
> +    }

This doesn't seem right. You should never go directly from elements to
RenderLayers; I think we need this top layer stuff represented in the render
tree too. You have to handle stuff like display:none on a <dialog>. What
happens if <dialog> elements have z-index? Can that z-index be negative?

> Source/WebCore/rendering/RenderLayer.h:701
> +	   return !style->hasAutoZIndex()
> +#if ENABLE(DIALOG_ELEMENT)
> +	       || isInTopLayer()
> +#endif
> +	       || isRootLayer();
> +    }

I think top element layers would have non-auto z-index?