[webkit-reviews] review canceled: [Bug 84796] Implement the new stacking layer needed by the Fullscreen API and the new <dialog> element : [Attachment 171781] review comments

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 5 03:45:23 PST 2012


Julien Chaffraix <jchaffraix at webkit.org> has canceled 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 171781: review comments
https://bugs.webkit.org/attachment.cgi?id=171781&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=171781&action=review


High-level comments: because we insert top layer renderers' into the
RenderView's children, they should interact with them during layout. I don't
know how much of a pain this is.

> Source/WebCore/ChangeLog:13
> +	   This adds the top layer element stack to Document. The stack is
> +	   stored in the DOM because pushing and popping from the top layer
> +	   occurs via the DOM functions showModal() and close() for dialog
> +	   elements. It seems difficult to store the stack as part of rendering

> +	   because, for example, an element without a renderer can be pushed on

> +	   the top layer.

I think more to the point: the FullScreen specification mandates that we track
the ordering of the DOM nodes in the top layer, not the renderers. That makes
it hard to implement on the rendering side only.

> Source/WebCore/dom/Document.cpp:5553
> +    ASSERT(m_topLayerElements.contains(element));
> +    size_t position = m_topLayerElements.find(element);

You could replace that with:

size_t position = m_topLayerElements.find(element);
ASSERT(position != notFound);

which would remove one iteration over the Vector in Debug.

> Source/WebCore/dom/Document.h:1165
> +    Vector<RefPtr<Element> >& topLayerElements() { return
m_topLayerElements; }

This should be a |const| function, probably returning a const Vector as we
don't expect callers to modify it.

> Source/WebCore/dom/Node.cpp:1328
> +	       ASSERT(!renderer || renderer->inRenderFlowThread() ||
(renderer->enclosingLayer()->isInTopLayerSubtree()));

Note that this will involve walking up the render tree in Debug for every
detach.

> Source/WebCore/dom/NodeRenderingContext.cpp:230
> +    size_t topLayerPosition = topLayerElements.find(element);
> +    ASSERT(topLayerPosition != notFound);

Couldn't we just find the next renderer from the topLayerElements instead of
iterating on all the RenderView's children?

Note that because you compute the positions in the Vector in the loop, the code
is O(n^2).

> Source/WebCore/rendering/RenderLayer.cpp:4669
> +	   for (RenderObject* child = view->firstChild(); child; child =
child->nextSibling()) {
> +	       Element* childElement = child->node()->isElementNode() ?
toElement(child->node()) : 0;
> +	       if (childElement && childElement->isInTopLayer()) {

Same comment here: couldn't we use the Document's list?

> LayoutTests/ChangeLog:16
> +

Per discussion, there should be a test for percent length resolution. This
would prove that we correctly layout top-layer elements with their containing
block as the initial containing block.

> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking.html:21
> +    -webkit-transform: rotateY(45deg);

As discussed I don't know if that covers the hardware accelerated case but it
needs to be covered by one test as we disable AC on named flow-thread.

> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking.html:27
> +<dialog id="z" style="display: none">This should not be displayed.</dialog>

You should add some red for good measure.

> LayoutTests/fast/dom/HTMLDialogElement/top-layer-stacking.html:49
> +document.getElementById('x').showModal();
> +document.getElementById('y').showModal();
> +document.getElementById('z').showModal();

x, y, z don't really speak to me. Better names: topHiddenDialog,
middleFixedDialog, bottomDialog.


More information about the webkit-reviews mailing list