Bug 116296

Created attachment 203796 [details] WIP Patch

Comment on attachment 203796 [details] WIP Patch Attachment 203796 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/662542

Comment on attachment 203796 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203796&action=review > Source/WebCore/ChangeLog:9 > + - the range of a box is always included in the range of its containing block (even for floats); this should be temporary and Temporary until when? > Source/WebCore/ChangeLog:21 > + Test: fast/regions/float-pushed-width-change-2.html Will the final patch have more tests? > Source/WebCore/ChangeLog:47 > + (WebCore::RenderBox::clampToStartAndEndRegions): This is the same function from RenderBlock but moved here i guess. > Source/WebCore/rendering/RenderBlock.cpp:7744 > + ASSERT((startRegion && endRegion) || (!startRegion && !endRegion)); When do you have the (!startRegion && !endRegion) situation, for multicol based regions? > Source/WebCore/rendering/RenderFlowThread.cpp:375 > +RenderRegion* RenderFlowThread::regionAtBlockOffset(const RenderBlock* block, LayoutUnit offset, bool extendLastRegion, RegionAutoGenerationPolicy autoGenerationPolicy, bool clamp) Assuming that the block is not null, i would add an ASSERT here. > Source/WebCore/rendering/RenderFlowThread.cpp:384 > + region = m_regionList.isEmpty() ? 0 : m_regionList.first(); Can you use here firstRegion() method on the RenderFlowThread? > Source/WebCore/rendering/RenderFlowThread.cpp:528 > +RenderRegion* RenderFlowThread::mapFromFlowToRegion(const RenderBlock* block, TransformState& transformState) const What is the meaning of block parameter here? Did you add it to be able to call regionAtBlockOffset or was for another purpose? Could you pass this instead of block below when calling regionAtBlockOffset? > Source/WebCore/rendering/RenderFlowThread.cpp:684 > +void RenderFlowThread::clearRenderBoxRegioInfo(const RenderBox* box, RenderRegion* startRegion, RenderRegion* endRegion) Regio -> Region

Comment on attachment 203796 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203796&action=review >> Source/WebCore/ChangeLog:9 >> + - the range of a box is always included in the range of its containing block (even for floats); this should be temporary and > > Temporary until when? Until we stabilize the regions overflow implementation. Having a child box in a region that's not in the range of its containing block brings more complexity to the code. >> Source/WebCore/ChangeLog:21 >> + Test: fast/regions/float-pushed-width-change-2.html > > Will the final patch have more tests? Yes. This is just WIP. >> Source/WebCore/ChangeLog:47 >> + (WebCore::RenderBox::clampToStartAndEndRegions): > > This is the same function from RenderBlock but moved here i guess. Yes, this functionality is not block specific. >> Source/WebCore/rendering/RenderBlock.cpp:7744 >> + ASSERT((startRegion && endRegion) || (!startRegion && !endRegion)); > > When do you have the (!startRegion && !endRegion) situation, for multicol based regions? I initially added this before having the early return when there's no start or end region. It should have covered the case of an empty region chain. It will be removed in the final patch. >> Source/WebCore/rendering/RenderFlowThread.cpp:375 >> +RenderRegion* RenderFlowThread::regionAtBlockOffset(const RenderBlock* block, LayoutUnit offset, bool extendLastRegion, RegionAutoGenerationPolicy autoGenerationPolicy, bool clamp) > > Assuming that the block is not null, i would add an ASSERT here. Will do. Actually, I think I'll remove the clamp parameter and if block == 0 then there's no clamping. >> Source/WebCore/rendering/RenderFlowThread.cpp:384 >> + region = m_regionList.isEmpty() ? 0 : m_regionList.first(); > > Can you use here firstRegion() method on the RenderFlowThread? Will do. >> Source/WebCore/rendering/RenderFlowThread.cpp:528 >> +RenderRegion* RenderFlowThread::mapFromFlowToRegion(const RenderBlock* block, TransformState& transformState) const > > What is the meaning of block parameter here? Did you add it to be able to call regionAtBlockOffset or was for another purpose? Could you pass this instead of block below when calling regionAtBlockOffset? When you map a coordinate local to a box you want to stay inside the region range of that box. If I pass "this" then it's no longer possible. >> Source/WebCore/rendering/RenderFlowThread.cpp:684 >> +void RenderFlowThread::clearRenderBoxRegioInfo(const RenderBox* box, RenderRegion* startRegion, RenderRegion* endRegion) > > Regio -> Region Will do.

Created attachment 204904 [details] WIP Patch v2

Comment on attachment 204904 [details] WIP Patch v2 Attachment 204904 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/950320

Comment on attachment 204904 [details] WIP Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=204904&action=review > Source/WebCore/rendering/RenderBox.cpp:124 > + too many spaces > Source/WebCore/rendering/RenderBox.cpp:129 > + here too > LayoutTests/platform/mac/TestExpectations:388 > +fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-bottom-positioned-multiple-shapes.html > LayoutTests/platform/mac/TestExpectations:389 > +fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-multiple-shapes.html I landed the patch from bug #117757 in http://trac.webkit.org/changeset/151703, so you can remove these two skips. Thanks!

Created attachment 205095 [details] Patch

Created attachment 205098 [details] Patch

Comment on attachment 205098 [details] Patch Attachment 205098 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/895902

Comment on attachment 205098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205098&action=review > Source/WebCore/ChangeLog:3 > + [CSS Regions] Compute correct region ranges for boxes Looks good, i have some comments below. > Source/WebCore/rendering/RenderBlock.cpp:7811 > + ASSERT(startRegion && endRegion); setRegionRangeForBox already asserts about startRegion and endRegion, do you still need this one here? > Source/WebCore/rendering/RenderBlock.cpp:7827 > + box->computeLogicalHeight(LayoutUnit::max() / 2, logicalTopForChild(box), estimatedValues); Can you add a comment why are you using LayoutUnit::max() / 2? Looking more at regions code, i see that LayoutUnit::max()/ 2 is used in some places, maybe we can factor it in a meaningfull define/function. > Source/WebCore/rendering/RenderBlock.cpp:7833 > + ASSERT(startRegion && endRegion); Same here, do you still need the assert? > Source/WebCore/rendering/RenderBox.cpp:147 > + if (!startRegion || !endRegion) Should we use an assert instead? Are there any cases in which a box does not have a range with start/end region? If not, i would revisit call sites for getRegionRangeForBox and add asserts where needed. > Source/WebCore/rendering/RenderBox.cpp:152 > + // ASSERT(clampToStartAndEndRegions(region) == region); Can you add a bug for this please? > Source/WebCore/rendering/RenderBox.cpp:154 > + // FIXME: Remove once boxes are painted inside their region range. And for this? > Source/WebCore/rendering/RenderFlowThread.cpp:388 > + RenderRegion* region = 0; I guess we can simplify this function by early return when the region list is empty. > Source/WebCore/rendering/RenderFlowThread.cpp:405 > + return region ? clampBox->clampToStartAndEndRegions(region) : 0; In which cases region is null here? > Source/WebCore/rendering/RenderFlowThread.cpp:411 > + return region ? clampBox->clampToStartAndEndRegions(region) : 0; In which cases is region null here? > Source/WebCore/rendering/RenderFlowThread.cpp:547 > + RenderRegion* renderRegion = const_cast<RenderFlowThread*>(this)->regionAtBlockOffset(block, isHorizontalWritingMode() ? center.y() : center.x(), true, DisallowRegionAutoGeneration); I read you previous explanation about why you need block passed here. However, the only call site for mapFromFlowToRegion is from mapLocalToContainer with the first parameter this == block. Maybe i am missing something.. > Source/WebCore/rendering/RenderFlowThread.cpp:603 > + return true; Can you explain this? > Source/WebCore/rendering/RenderFlowThread.cpp:692 > if (!hasRegions()) I guess this can be turned into an assert - the call site already tested it. > Source/WebCore/rendering/RenderFlowThread.cpp:711 > + if (!hasRegions()) Does it make sense to transform this into an assert? Looks like it is always called from places where you already tested that the flow has regions attached. > LayoutTests/fast/regions/float-pushed-width-change-2-expected.html:5 > + @font-face { Are you using this? > LayoutTests/fast/regions/float-pushed-width-change-2.html:5 > + @font-face { Are you using this declaration? > LayoutTests/platform/efl/TestExpectations:543 > +# FIXME: The widows and orphans implementation interferes with the region range change relayout. Are you adding a bug for this?

Comment on attachment 205098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205098&action=review > Source/WebCore/rendering/RenderFlowThread.cpp:504 > return region ? region->pageLogicalWidth() : contentLogicalWidth(); Should you be passing "true" for the extendLastRegion parameter?

Comment on attachment 205098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205098&action=review > Source/WebCore/rendering/RenderBlock.cpp:2889 > + r->layoutIfNeeded(); Why did you take the layoutIfNeeded call out of the if statement?

Comment on attachment 205098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205098&action=review > Source/WebCore/rendering/RenderBlock.cpp:7803 > + LayoutUnit offsetFromLogicalTopOfFirstRegion = box->offsetFromLogicalTopOfFirstPage(); You're using "box" without validating it. > Source/WebCore/rendering/RenderBlock.cpp:7815 > +void RenderBlock::estimateRegionRangeForBoxChild(RenderBox* box) const Shouldn't box be const? > Source/WebCore/rendering/RenderBlock.cpp:7837 > +void RenderBlock::updateRegionRangeForBoxChild(RenderBox* box) const Shouldn't box be const? > Source/WebCore/rendering/RenderBlock.cpp:7857 > + box->setNeedsLayout(true, MarkOnlyThis); I believe it would be better to have this method return true if the region range has changed and set the box as needing layout after calling this method. Either that or changing this method's name to indicate it will mark the box as needing layout. > Source/WebCore/rendering/RenderFlowThread.cpp:695 > + ASSERT(startRegion && endRegion); Should box also be validated?

Comment on attachment 205098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205098&action=review >> Source/WebCore/rendering/RenderBlock.cpp:2889 >> + r->layoutIfNeeded(); > > Why did you take the layoutIfNeeded call out of the if statement? Initially I've placed updateRegionRangeForBoxChild() before the last layout so I took it out of the if. I'll put it back in the final patch. >> Source/WebCore/rendering/RenderBlock.cpp:7803 >> + LayoutUnit offsetFromLogicalTopOfFirstRegion = box->offsetFromLogicalTopOfFirstPage(); > > You're using "box" without validating it. This function is called on the child boxes which can't be null. I don't see how a null reference can be passed to this function without being caught much earlier in the layout algorithm. >> Source/WebCore/rendering/RenderBlock.cpp:7811 >> + ASSERT(startRegion && endRegion); > > setRegionRangeForBox already asserts about startRegion and endRegion, do you still need this one here? No. I'll remove it. >> Source/WebCore/rendering/RenderBlock.cpp:7815 >> +void RenderBlock::estimateRegionRangeForBoxChild(RenderBox* box) const > > Shouldn't box be const? Very likely it should be. I'll try to make it const in the next version. >> Source/WebCore/rendering/RenderBlock.cpp:7827 >> + box->computeLogicalHeight(LayoutUnit::max() / 2, logicalTopForChild(box), estimatedValues); > > Can you add a comment why are you using LayoutUnit::max() / 2? Looking more at regions code, i see that LayoutUnit::max()/ 2 is used in some places, maybe we can factor it in a meaningfull define/function. I'll add a paragraph about this in the ChangeLog. >> Source/WebCore/rendering/RenderBlock.cpp:7833 >> + ASSERT(startRegion && endRegion); > > Same here, do you still need the assert? No. >> Source/WebCore/rendering/RenderBlock.cpp:7837 >> +void RenderBlock::updateRegionRangeForBoxChild(RenderBox* box) const > > Shouldn't box be const? If I leave the setNeedsLayout call there, no. >> Source/WebCore/rendering/RenderBlock.cpp:7857 >> + box->setNeedsLayout(true, MarkOnlyThis); > > I believe it would be better to have this method return true if the region range has changed and set the box as needing layout after calling this method. Either that or changing this method's name to indicate it will mark the box as needing layout. You are right this is a bit black-boxish, but considering you always need to make a layout whenever you call this function, I've placed the code inside it to minimize duplication. >> Source/WebCore/rendering/RenderBox.cpp:147 >> + if (!startRegion || !endRegion) > > Should we use an assert instead? Are there any cases in which a box does not have a range with start/end region? If not, i would revisit call sites for getRegionRangeForBox and add asserts where needed. Inline blocks don't have a range assigned to them. This is just a simplified fix for those cases. >> Source/WebCore/rendering/RenderBox.cpp:152 >> + // ASSERT(clampToStartAndEndRegions(region) == region); > > Can you add a bug for this please? After the patch lands, yes. >> Source/WebCore/rendering/RenderBox.cpp:154 >> + // FIXME: Remove once boxes are painted inside their region range. > > And for this? Same :). >> Source/WebCore/rendering/RenderFlowThread.cpp:388 >> + RenderRegion* region = 0; > > I guess we can simplify this function by early return when the region list is empty. Will do. >> Source/WebCore/rendering/RenderFlowThread.cpp:405 >> + return region ? clampBox->clampToStartAndEndRegions(region) : 0; > > In which cases region is null here? I don't think it can be null here, you are right. >> Source/WebCore/rendering/RenderFlowThread.cpp:411 >> + return region ? clampBox->clampToStartAndEndRegions(region) : 0; > > In which cases is region null here? If extendLastRegion is false the region can be null. >> Source/WebCore/rendering/RenderFlowThread.cpp:504 >> return region ? region->pageLogicalWidth() : contentLogicalWidth(); > > Should you be passing "true" for the extendLastRegion parameter? This is what the old code did. >> Source/WebCore/rendering/RenderFlowThread.cpp:547 >> + RenderRegion* renderRegion = const_cast<RenderFlowThread*>(this)->regionAtBlockOffset(block, isHorizontalWritingMode() ? center.y() : center.x(), true, DisallowRegionAutoGeneration); > > I read you previous explanation about why you need block passed here. However, the only call site for mapFromFlowToRegion is from mapLocalToContainer with the first parameter this == block. Maybe i am missing something.. The mapLocalToContainer function was refactored a while back and made virtual. This code predates that change. I'll change the parameter. >> Source/WebCore/rendering/RenderFlowThread.cpp:603 >> + return true; > > Can you explain this? Yes, this is an ugly fix to force flow thread children to relayout and compute their ranges. The following code doesn't work when this == the render flow thread because it will always have updated region range info. I think I can use the m_pageLogicalSizeChanged flag again to have this return true only when necessary. >> Source/WebCore/rendering/RenderFlowThread.cpp:692 >> if (!hasRegions()) > > I guess this can be turned into an assert - the call site already tested it. Will do. >> Source/WebCore/rendering/RenderFlowThread.cpp:695 >> + ASSERT(startRegion && endRegion); > > Should box also be validated? As I previously said the code is very integrated and it should never happen to call this with a null box. >> Source/WebCore/rendering/RenderFlowThread.cpp:711 >> + if (!hasRegions()) > > Does it make sense to transform this into an assert? Looks like it is always called from places where you already tested that the flow has regions attached. Will do. >> LayoutTests/fast/regions/float-pushed-width-change-2-expected.html:5 >> + @font-face { > > Are you using this? No, I'll remove it. >> LayoutTests/fast/regions/float-pushed-width-change-2.html:5 >> + @font-face { > > Are you using this declaration? Same as above. >> LayoutTests/platform/efl/TestExpectations:543 >> +# FIXME: The widows and orphans implementation interferes with the region range change relayout. > > Are you adding a bug for this? When the patch lands, yes.

Created attachment 206324 [details] Patch

Created attachment 208540 [details] Patch

Comment on attachment 208540 [details] Patch r=me

Comment on attachment 208540 [details] Patch Clearing flags on attachment: 208540 Committed r153990: <http://trac.webkit.org/changeset/153990>

All reviewed patches have been landed. Closing bug.