[webkit-reviews] review denied: [Bug 89993] [CSS Exclusions] Enable css exclusions for multiple blocks per element : [Attachment 162833] Incorporating feedback

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Bear Travis <betravis at adobe.com>'s
request for review:
Bug 89993: [CSS Exclusions] Enable css exclusions for multiple blocks per
element
https://bugs.webkit.org/show_bug.cgi?id=89993

Attachment 162833: Incorporating feedback
https://bugs.webkit.org/attachment.cgi?id=162833&action=review

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


In general I have concerns here about propagating everything through
LayoutState. I'm not sure of this approach. It could be I have a
misunderstanding of WrapShapeInfo though. Is this struct used both for being
inside and outside, i.e., is it used both to fit lines inside a shape as well
as to help lines avoid intruding into a shape from the outside? If it's the
latter, I am concerned about mindlessly propagating every last bit of
information from ancestors down to children. It seems like you'd run into
performance problems if you haven't limited the scope somehow.

Mostly though I'm concerned about writing modes, since connecting to
LayoutState is going to get you into trouble.

Another question I have is it looks like an inner WrapShapeInfo will override
an outer WrapShapeInfo? Does only one truly apply? Can multiple ones not apply?
Do you not have to go up the LayoutState stack to examine all of them?

> Source/WebCore/rendering/LayoutState.cpp:46
> +    , m_wrapShapeInfo(0)

I'd still like to see a rename at some point from wrapShape -> exclusionShape.
Especially once the ifdefs eventually fall away, people aren't going to link
"wrap' with the exclusions feature.

> Source/WebCore/rendering/LayoutState.h:98
> +    WrapShapeInfo* wrapShapeInfo() { return m_wrapShapeInfo; }

This method should be const.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:83
> +	   WrapShapeInfo* wrapShapeInfo =
m_block->view()->layoutState()->wrapShapeInfo();

Rather than removing the RenderBlock method, I think you'd be better off having
it still exist and return view()->layoutState()->wrapShapeInfo().

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:807
> +    WrapShapeInfo* wrapShapeInfo = view()->layoutState()->wrapShapeInfo();

Ditto here.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1298
> +    LayoutUnit logicalTopOffset;

This is not a good name, since it sounds like you're in local coordinates. With
the layout offset applied, though, this is really absolute coordinates.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1302
> +	   logicalTopOffset = logicalTop() +
view()->layoutState()->layoutOffset().height();

You've got a big writing modes problem here. We don't use LayoutState for
flipped blocks writing modes (the LayoutState gets disabled), since right now
m_layoutOffset isn't accounting for flipping.

Connecting propagation to m_layoutOffset creates a big problem, since
LayoutState basically doesn't work with flipped blocks writing modes.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1305
> +    if (wrapShapeInfo && logicalHeight() + logicalTopOffset <
wrapShapeInfo->shapeTop())
> +	   setLogicalHeight(wrapShapeInfo->shapeTop() - logicalTopOffset);

Everything seems completely muddled here with writing modes. You can't just add
the layout offset's height to a logical top. The layout offset is physical and
logicalTop is not.

I still don't know what shapeTop means. Is it logical? Is it physical? I don't
know.

> Source/WebCore/rendering/RenderView.h:227
> +	       || (renderer->isRenderBlock() &&
toRenderBlock(renderer)->wrapShapeInfo())

It's a little weird to me that once a parent has wrap shape info, that you then
will push that along down to every descendant. It seems like your filtering
needs to be smarter than that. Aren't you going to want to limit this scope?