[webkit-reviews] review granted: [Bug 110624] [New Multicolumn] Rewrite the painting/stacking model to be spec compliant : [Attachment 190593] New patch with the paint transform refactoring removed.

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dave Hyatt
<hyatt at apple.com>'s request for review:
Bug 110624: [New Multicolumn] Rewrite the painting/stacking model to be spec
compliant
https://bugs.webkit.org/show_bug.cgi?id=110624

Attachment 190593: New patch with the paint transform refactoring removed.
https://bugs.webkit.org/attachment.cgi?id=190593&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=190593&action=review


r=me but see comments and consider adding more test cases.

> Source/WebCore/ChangeLog:20
> +	   and overlap multiple columns. In addition clips may apply across
pagination
> +	   boundaries, and any overlap caused by opacity has to do the right
thing and
> +	   treat the paginated and unpaginated components together as a single
unit.

I don't see this clipping and opacity behavior tested by your single new test
case. Perhaps more test cases are in order?

> Source/WebCore/rendering/RenderFlowThread.cpp:962
> +void RenderFlowThread::collectLayerFragments(Vector<LayerFragment>&
layerFragments, const LayoutRect& layerBoundingBox, const LayoutRect&
dirtyRect)

Use LayerFragments.

> Source/WebCore/rendering/RenderFlowThread.h:147
> +    void collectLayerFragments(Vector<LayerFragment>&, const LayoutRect&
layerBoundingBox, const LayoutRect& dirtyRect);

Use LayerFragments.

Why is layerBoundingBox referring to a layer, and not, say, a region or column
box?

> Source/WebCore/rendering/RenderLayer.cpp:947
>      bool newColumnsUsed = useRegionBasedColumns();

Don't like "new" in this context. Just called it regionBasedColumns?

> Source/WebCore/rendering/RenderLayer.cpp:957
> +	       if (m_enclosingPaginationLayer &&
m_enclosingPaginationLayer->hasTransform())
> +		   m_enclosingPaginationLayer = 0;

This deserves a comment (and a test case?)

> Source/WebCore/rendering/RenderLayer.cpp:974
> +		   if (m_enclosingPaginationLayer &&
m_enclosingPaginationLayer->hasTransform())
> +		       m_enclosingPaginationLayer = 0;

Ditto.

> Source/WebCore/rendering/RenderLayer.cpp:3812
> +	   // FIXME: It is incorrect to just clip to the damageRect here once
multiple fragments are involved.

File a bugzilla?

> Source/WebCore/rendering/RenderLayer.cpp:3904
> +    // layers between us and the pagination context. It's important to
minimize the # of fragments we need to create and this helps with
> +    // that.

# -> number
Avoid the orphan?

> Source/WebCore/rendering/RenderLayer.cpp:3913
> +    // Take our bounding box within the flow thread and clip it. This will
help minimize the # of fragments we have to create.

# -> number

> Source/WebCore/rendering/RenderLayer.cpp:4009
> +	   LayerFragment fragment = layerFragments.at(i);

Do you need a copy of the fragment on the stack?

> Source/WebCore/rendering/RenderLayer.cpp:4035
> +    RenderObject* paintingRootForRenderer, bool selectionOnly, bool
forceBlackText)

Why do you need both the bools and the PaintBehavior flags? Can't the caller
set up the PaintBehavior flags?

> Source/WebCore/rendering/RenderLayer.cpp:4076
> +	   LayerFragment fragment = layerFragments.at(i);

Need LayerFragment on the stack here?

> Source/WebCore/rendering/RenderLayer.cpp:4097
> +	   LayerFragment fragment = layerFragments.at(i);

Ditto.

> Source/WebCore/rendering/RenderLayer.cpp:4113
> +	   LayerFragment fragment = layerFragments.at(i);

Ditto.

> Source/WebCore/rendering/RenderLayer.cpp:4366
>      offset.moveBy(translationOffset);
>  
> +    offset.moveBy(translationOffset);

Merge error?

> Source/WebCore/rendering/RenderLayer.cpp:4580
> +	   LayerFragment fragment = layerFragments.at(i);

Need a copy of the fragment?

> Source/WebCore/rendering/RenderLayer.cpp:4598
> +	   LayerFragment fragment = layerFragments.at(i);

Ditto.

> Source/WebCore/rendering/RenderLayer.cpp:5004
> +    // If we cross into a different pagination context, then we can't rely
on the cache.
> +    // Just switch over to using TemporaryClipRects.

We might want to solve this eventually. Temporary clip rects have O(n^2)
behavior.