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

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
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?