[Webkit-unassigned] [Bug 64516] [CSSRegions] Collect flowed elements in different render element

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=64516


Dave Hyatt <hyatt at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #101146|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #3 from Dave Hyatt <hyatt at apple.com>  2011-07-19 13:32:19 PST ---
(From update of attachment 101146)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.