[webkit-reviews] review denied: [Bug 66921] [CSSRegions] RenderFlowThread::renderRegionForLine should use a faster search method : [Attachment 196358] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Andrei Bucur <abucur at adobe.com>'s
request for review:
Bug 66921: [CSSRegions] RenderFlowThread::renderRegionForLine should use a
faster search method
https://bugs.webkit.org/show_bug.cgi?id=66921

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

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


Please handle

expandToEncompassFlowThreadContentsIfNeeded()

which hacks the flow thread portion rect. You'll want to make sure that
interval gets correctly updated.

> Source/WebCore/rendering/RenderFlowThread.cpp:827
> +	  
m_regionIntervalTree.add(RegionIntervalTree::createInterval(regionRect.y(),
regionRect.maxY(), region));

I think because of writing modes it would read better not to use y() and maxY()
here. You could just use logicalHeight and logicalHeight + regionLogicalHeight,
and I think that would read slightly better.

>> Source/WebCore/rendering/RenderFlowThread.cpp:968
>> +	if (interval.data()->isRenderRegionSet())
> 
> Is it possible we'd want to return a region set that's not the last region?
The old code seemed to have allowed it.

Yes, it will be possible to have multiple region sets. Your code is going to be
applicable to region sets.