[webkit-reviews] review denied: [Bug 64516] [CSSRegions] Collect flowed elements in different render element : [Attachment 101467] Patch with forced RenderLayer

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 64516: [CSSRegions] Collect flowed elements in different render element
https://bugs.webkit.org/show_bug.cgi?id=64516

Attachment 101467: Patch with forced RenderLayer
https://bugs.webkit.org/attachment.cgi?id=101467&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=101467&action=review


Few more comments.

> Source/WebCore/rendering/RenderFlowThread.cpp:84
> +RenderObject* RenderFlowThread::nextRendererForNode(Node* node) const
> +{
> +    for (RenderObject* child = firstChild(); child; child =
child->nextSibling()) {
> +	   ASSERT(child->node());
> +	   unsigned short position =
node->compareDocumentPosition(child->node());
> +	   if (position & Node::DOCUMENT_POSITION_FOLLOWING)
> +	       return child;
> +    }
> +    return 0;
> +}
> +
> +RenderObject* RenderFlowThread::previousRendererForNode(Node* node) const
> +{
> +    for (RenderObject* child = lastChild(); child; child =
child->previousSibling()) {
> +	   ASSERT(child->node());
> +	   unsigned short position =
node->compareDocumentPosition(child->node());
> +	   if (position & Node::DOCUMENT_POSITION_PRECEDING)
> +	       return child;
> +    }
> +    return 0;
> +}

The one concern I can think of with these functions is that the
RenderFlowThread may have been forced to create anonymous blocks to wrap
content. For example let's say you put both a <div> and a <span> into the same
flow thread. When you put the <span> into the thread, an anonymous block will
be created to wrap the <span>. People can also write weird code like div {
display: table-cell } and throw that into a RenderFlowThread, and a bunch of
anonymous table elements would be created to wrap that object as well.

I think you will probably have to maintain an actual list of the nodes
themselves in the RenderFlowThread and walk that instead. Something similar to
the ListHashSet used for candidate stylesheet nodes that I mentioned in the
previous review.

> Source/WebCore/rendering/RenderLayer.cpp:2551
> +#if ENABLE(CSS_REGIONS)
> +    // Do not let the RenderFlowThread to render directly to screen. It will
only render
> +    // inside the RenderRegions.
> +    if (renderer()->isRenderFlowThread())
> +	   return;
> +#endif

I think a better way to handle this would be to simply avoid collecting the
child layers into the lists. That way you only have to patch one place. I think
you could just patch updateZOrderLists to skip flow thread layers. That seems a
bit simpler to me than patching both painting and hit testing functions.

> Source/WebCore/rendering/RenderLayer.h:-327
> -    bool isStackingContext() const { return !hasAutoZIndex() ||
renderer()->isRenderView(); }

I don't think you'll have to patch this function if you just hack
updateZOrderLists to avoid collecting the layers instead.