[webkit-reviews] review denied: [Bug 78493] [CSSRegions][CSSOM] Implement Element.getRegionFlowRanges : [Attachment 161495] patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Raul Hudea <rhudea at adobe.com>'s request
for review:
Bug 78493: [CSSRegions][CSSOM] Implement Element.getRegionFlowRanges
https://bugs.webkit.org/show_bug.cgi?id=78493

Attachment 161495: patch
https://bugs.webkit.org/attachment.cgi?id=161495&action=review

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


r-. Comments below.

> Source/WebCore/rendering/RenderBlock.cpp:7189
> +    // slower path without layout state
> +    LayoutUnit offsetTop;
> +    const RenderBlock* currBlock = this;

Can you put in a FIXME above this code that it doesn't work for columns or
pages? Right now nothing works outside of layout, but with your change, only
regions will work outside of layout. That's actually fine, since the new
columns and pages are written using regions and will work with this new code
you've put in, but I'd at least like a FIXME for now that it doesn't work with
the current columns and pages code.

Something like;

// FIXME: This is a slower path that doesn't use layout state and relies on
getting your logical top inside the enclosing flow thread. It doesn't
// work with columns or pages currently, but it should once they have been
switched over to using flow threads.

Also, I'd like to see an ASSERT added to prevent anyone from calling this
outside of layout for anything but regions. You can do this by adding:

ASSERT(currBlock->inRenderFlowThread());

before your loop. You could also just go ahead and bail with a return value of
ZERO_LAYOUT_UNIT if currBlock isn't inside a render flow thread (following the
assert).

> Source/WebCore/rendering/RenderBlock.cpp:7192
> +	   offsetTop += currBlock->logicalTop();

This isn't correct. You have to account for changes in writing mode as you
crawl up containing blocks. You're going to need to add isWritingModeRoot
checks and do flipping as needed in order to get the correct offset.

You're going to need some additional tests here. Embed a few orthogonal writing
mode objects inside your regions so that you are testing this, e.g., put a
vertical-rl object inside a horizontal-tb flow thread etc. They don't paginate
and just move atomically, but they should still work correctly as far as
knowing what region they're in.

> Source/WebCore/rendering/RenderFlowThread.cpp:-792
> -

Random whitespace change. Please revert this.

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:378
> +    LayoutRect regionRect(region->flowThreadPortionRect());

I would prefer you call the local flowThreadPortionRect and not regionRect.

Ultimately we're probably going to move to adopt CSS3 Fragmentation's
terminology and start calling these fragmentRects, but for now let's keep using
flowThreadPortion.

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:387
> +	   logicalTopForRegion = logicalTopForBox(isHorizontalWritingMode(),
regionRect);

I think this would be a useful method to put on RenderRegion itself. I can see
this being used by others, and I think there are even existing places in the
code that grab this value that would benefit from such a helper.

Something like:

LayoutUnit RenderRegion::logicalTopForFlowThreadContent() const { return
flowThread()->isHorizontalWritingMode() ? flowThreadPortionRect().y() :
flowThreadPortionRect().x(); }

If you look around you'll see we do the above in other places in the code too.

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:393
> +	   logicalBottomForRegion =
logicalBottomForBox(isHorizontalWritingMode(), regionRect);

LayoutUnit RenderRegion::logicalBottomForFlowThreadContent() const { return
flowThread()->isHorizontalWritingMode() ? flowThreadPortionRect().maxY() :
flowThreadPortionRect().maxX(); }

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:439
> +	       LayoutUnit logicalTopForRenderer =
logicalTopForBox(isHorizontalWritingMode(), boundingBox);
> +	       LayoutUnit logicalBottomForRenderer =
logicalBottomForBox(isHorizontalWritingMode(), boundingBox);

RenderRegion::logicalTopOfFlowThreadContentRect(const LayoutRect& rect) const;
RenderRegion::logicalBottomOfFlowThreadContentRect(const LayoutRect& rect)
const;

Could even implement the previous helpers I suggested and have them call these,
passing in the flowThreadPortionRect().