Bug 64516 – [CSSRegions] Collect flowed elements in different render element

• Component: CSS
• Status: RESOLVED FIXED

[Alexandru Chiculita]

Elements that specify a different flow name should be collected inside an anonymous render block. It will later be used to layout the content inside multiple regions.

[Alexandru Chiculita]

Created attachment 101144 [details]
Patch

[Alexandru Chiculita]

Created attachment 101146 [details]
Patch

Re-added fast/regions to the skipped list.

[Dave Hyatt]

Comment on attachment 101146 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101146&action=review

So my big concern with this patch is with the nextRenderer() and previousRenderer() functions. These seem like they could be very slow.

At a bare minimum I want to make sure that nextRenderer() is fast, since that function is called for every element that is put into the render tree.

This problem of picking out the correct order from the overall DOM tree feels similar to the issue we encounter with <style> elements in the body. In that case we dirty the style selector and then figure things out later.

Rather than moving through all the nodes of the document in previousRenderer and nextRenderer, I think you should walk the current RenderFlowThread children instead. That way you can find the correct node much more quickly using compareDocumentPosition.

Check out addStyleSheetCandidateNode in Document.cpp to see what I mean.

> Source/WebCore/dom/Document.cpp:5067
> +RenderFlowThread* Document::renderFlowThreadWithName(const AtomicString& flowThread)

I don't think it's appropriate to have code in the document for this. Just implement this on RenderView instead. The code will look much cleaner too.

> Source/WebCore/dom/Document.h:162
> +#if ENABLE(CSS_REGIONS)
> +class RenderFlowThread;
> +#endif

Move to RenderView.

> Source/WebCore/dom/Document.h:1104
> +#if ENABLE(CSS_REGIONS)
> +    RenderFlowThread* renderFlowThreadWithName(const AtomicString& flowThread);
> +#endif

Move to RenderView.

> Source/WebCore/dom/NodeRenderingContext.cpp:166
> +        // Search the whole document for elements that are attached to the same flow thread.
> +        for (Node* node = m_node->traverseNextNode(); node; node = node->traverseNextNode())

This seems super slow. It seems like we should be able to do better. It also seems like this code should go right before the normal traversal loop rather than above the special case code to avoid o(n^2) issues when the parent isn't attached yet.

> Source/WebCore/dom/NodeRenderingContext.cpp:213
> +        // Search the whole document for elements that are attached to the same flow thread.
> +        for (Node* node = m_node->traversePreviousNode(); node; node = node->traversePreviousNode())

This seems like it could be super slow also.

> Source/WebCore/dom/NodeRenderingContext.cpp:306
> +    if (!m_style || m_style->flowThread().isEmpty())
> +        return;
> +    m_flowThread = m_style->flowThread();

This shouldn't run for text nodes. Is there something I'm missing that would keep it from running for text nodes?

> Source/WebCore/rendering/RenderFlowThread.cpp:53
> +    RefPtr<RenderStyle> newStyle = RenderStyle::create();
> +    newStyle->setDisplay(BLOCK);
> +    newStyle->setPosition(AbsolutePosition);
> +    newStyle->setLeft(Length(0, Fixed));
> +    newStyle->setTop(Length(0, Fixed));
> +    newStyle->setWidth(Length(100, Percent));
> +    newStyle->setHeight(Length(100, Percent));
> +    newStyle->setOverflowX(OHIDDEN);
> +    newStyle->setOverflowY(OHIDDEN);
> +    newStyle->font().update(0);

I'd pull this out into a static createFlowThreadStyle function.

> Source/WebCore/rendering/RenderFlowThread.cpp:65
> +void RenderFlowThread::paint(PaintInfo&, const LayoutPoint&)
> +{
> +}
> +
> +bool RenderFlowThread::nodeAtPoint(const HitTestRequest&, HitTestResult&, const LayoutPoint& /*pointInContainer*/, const LayoutPoint& /*accumulatedOffset*/, HitTestAction)
> +{
> +    return false;
> +}

Just inline these in the header right where they're declared.

> Source/WebCore/rendering/RenderFlowThread.h:49
> +    virtual void paint(PaintInfo&, const LayoutPoint&);
> +    virtual bool nodeAtPoint(const HitTestRequest&, HitTestResult&, const LayoutPoint& pointInContainer, const LayoutPoint& accumulatedOffset, HitTestAction);

I think it would be good to comment why these methods do nothing. It's an opportunity to explain what's special about RenderFlowThread (that it's just collecting the objects).

> Source/WebCore/rendering/RenderFlowThread.h:57
> +    ASSERT(!object || !strcmp(object->renderName(), "RenderFlowThread"));

Why not just assert that the object isRenderFlowThread instead?

[Alexandru Chiculita]

Created attachment 101457 [details]
Patch

Thanks for review! I think I fixed them all.

[WebKit Review Bot]

Attachment 101457 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderFlowThread.cpp:34:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/rendering/RenderFlowThread.cpp:35:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/rendering/RenderFlowThread.cpp:49:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebCore/rendering/RenderFlowThread.h:70:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderFlowThread.h:71:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexandru Chiculita]

Some tasks related to this patch:

1. Overriding the RenderFlowThread::paint and RenderFlowThread::nodeAtPoint do not seem enough. Child elements might render inside different RenderLayers, so the right approach would have been to force the RenderLayer to avoid rendering the RenderFlowThread and any descendant layer. The same for nodeAtPoint. I will post a new patch with that.

2. Elements inside the RenderFlowThread that require a RenderLayerBacking should only be created if the RenderFlowThread is attached to RenderRegions. The layers need to be attached to the layer of the regions instead of attaching them to the layer of the 'virtual' RenderFlowThread.

[Alexandru Chiculita]

Created attachment 101462 [details]
Patch

Fixed code style.

[Alexandru Chiculita]

Created attachment 101467 [details]
Patch with forced RenderLayer

Using RenderLayer to avoid drawing the RenderFlowThread.

[Dave Hyatt]

Comment on attachment 101467 [details]
Patch with forced RenderLayer

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.

[Alexandru Chiculita]

Created attachment 101566 [details]
Patch

Thanks for the tips! I've integrated the fixes.

updateZOrderLists also fixes the RenderLayers that have accelerated rendering. When RenderRegion patch will be committed more logic can be added to distribute the layers directly to the region's layer based on their position in the flow thread.

[Alexandru Chiculita]

Created attachment 101570 [details]
Patch

Forgot to add the ChangeLogs in the diff file.

[Dave Hyatt]

Comment on attachment 101570 [details]
Patch

Looks good. r=me.

[WebKit Review Bot]

Comment on attachment 101570 [details]
Patch

Rejecting attachment 101570 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
file Source/WebCore/rendering/RenderLayer.cpp
patching file Source/WebCore/rendering/RenderObject.h
Hunk #1 succeeded at 316 (offset 1 line).
patching file Source/WebCore/rendering/RenderTreeAsText.cpp
patching file Source/WebCore/rendering/RenderView.cpp
patching file Source/WebCore/rendering/RenderView.h
patching file Source/WebCore/rendering/style/RenderStyle.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'David Hyatt', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9249086

[Alexandru Chiculita]

Created attachment 101974 [details]
Rebased patch

I'm not a committer, so I rebased the patch and I'm adding it for commit queue.

[WebKit Review Bot]

Comment on attachment 101974 [details]
Rebased patch

Clearing flags on attachment: 101974

Committed r91760: <http://trac.webkit.org/changeset/91760>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.