Bug 93547

Created attachment 157815 [details] Initial patch, based on 89259 Proposed patch. Diff is based off 89259, so it will not be ready until that patch lands.

Created attachment 160472 [details] Updated patch Including changes from main branch, now that bug 89259 has been fixed.

Comment on attachment 160472 [details] Updated patch Attachment 160472 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13594283 New failing tests: fast/exclusions/shape-inside/shape-inside-percentage-auto.html

Created attachment 160488 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 160491 [details] Simplify percentage-auto test

Created attachment 162594 [details] Fixing min/max-height edge case Logical height was not always being calculated correctly when max-height was set

Comment on attachment 162594 [details] Fixing min/max-height edge case View in context: https://bugs.webkit.org/attachment.cgi?id=162594&action=review > Source/WebCore/rendering/RenderBlock.cpp:1416 > + bool percentageLogicalHeightResolvable = percentageLogicalHeightIsResolvableFromBlock(this, document()->inQuirksMode(), false); Why is outOfFlowPositioned hardcoded to false here? > Source/WebCore/rendering/RenderBlock.cpp:1512 > - computeInitialRegionRangeForBlock(); > -#if ENABLE(CSS_EXCLUSIONS) > - // FIXME: Bug 93547: Resolve logical height for percentage based vertical lengths > - if (WrapShapeInfo* wrapShapeInfo = this->wrapShapeInfo()) > - wrapShapeInfo->computeShapeSize(logicalWidth(), 0); > -#endif > + computeInitialLogicalSizeBasedMeasurements(); I'm happy to see this code go into a helper. > Source/WebCore/rendering/RenderBox.cpp:3735 > +bool RenderBox::percentageLogicalHeightIsResolvableFromBlock(const RenderBlock* containingBlock, bool quirksMode, bool outOfFlowPositioned) boolean params :( We appear to ask the document if it's in quirksMode in all cases, so what do we gain by making it a parameter?

Created attachment 162899 [details] Incorporating feedback Fixing up patch. Removing quirksMode argument & changing outOfFlowPositioned to an enum The call in RenderBlock has a hardcoded InFlowPosition because percentage lengths for shape-inside are resolved as if they were in flow

Comment on attachment 162899 [details] Incorporating feedback View in context: https://bugs.webkit.org/attachment.cgi?id=162899&action=review Comments below. > Source/WebCore/rendering/RenderBlock.cpp:1397 > -void RenderBlock::computeInitialRegionRangeForBlock() > +void RenderBlock::computeInitialLogicalSizeBasedMeasurements() My suggestion here would be to retain computeInitialRegionRangeForBlock and have it be called by computeInitialLogicalSizeBasedMeasurements. Then have your own additional function that computeInitialLogicalSizeBasedMeasurements calls to handle exclusions. Note the term "size" is wrong here, since all you care about is logical height, so you should rename to computeLogicalHeightBasedMeasurements. I think you can drop the term "initial" since that really only applied to region ranges and was implying that the region range might be corrected later on. > Source/WebCore/rendering/RenderBox.h:40 > +enum PositionType { InFlowPosition, OutOfFlowPosition }; I'm not following why this needs to be added. I don't like it.

Created attachment 163216 [details] Incorporating feedback Fixing up the patch. computeInitialLogicalSizeBasedMeasurements computes the maximum possible logical height, then passes it, along with the current logical width, along to regions and exclusions. With the calculations removed, computeInitialRegionRangeForBlock was obsoleted, and I added a call to computeRegionRangeForBlock instead. Removed the term initial Modified the code to call the new const versions of computeLogicalHeight. Removed PositionType

Created attachment 163233 [details] Updating patch Merging with more recent changes

Comment on attachment 163233 [details] Updating patch Attachment 163233 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13810400

Created attachment 163249 [details] Fixing windows build

Comment on attachment 163249 [details] Fixing windows build Attachment 163249 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13822124 New failing tests: fast/repaint/region-painting-via-layout.html

Created attachment 163425 [details] Incorporating feedback Responding to feedback from Mihnea. Updating changelog to be more descriptive. Caching inQuirksMode boolean in RenderBox::percentageLogicalHeightIsResolvable. Renaming computeLogicalSizeBasedMeasurements -> updateRegionsAndExclusionsLogicalSize, and removed the logical width/height parameters.

Comment on attachment 163425 [details] Incorporating feedback Attachment 163425 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13825380 New failing tests: fast/repaint/region-painting-via-layout.html

Created attachment 163753 [details] Updating patch

Comment on attachment 163753 [details] Updating patch Attachment 163753 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13826979 New failing tests: fast/repaint/region-painting-via-layout.html

Created attachment 164013 [details] Use old-style updateLogicalHeight Simplify patch to use updateLogicalHeight, rather than the const computeLogicalHeight. The code should eventually use computeLogicalHeight, but several areas of the code need to be modified to not use the side effects of updateLogicalHeight.

Adding bug 96724 to track the const computeLogicalHeight issue.

(In reply to comment #19) > Created an attachment (id=164013) [details] > Use old-style updateLogicalHeight > > Simplify patch to use updateLogicalHeight, rather than the const computeLogicalHeight. The code should eventually use computeLogicalHeight, but several areas of the code need to be modified to not use the side effects of updateLogicalHeight. BTW, one of the know problems with computeLogicalHeight right now is that it's not virtual so if you call it on a pointer and your pointer overrides updateLogicalHeight (e.g., it's an iframe), it'll do the wrong thing. I think long term, we want to make computeLogicalHeight virtual and move code from the subclass updateLogicalHeight into it and then make updateLogicalHeight non-virtual. I've filed bug 96804 for this.

Comment on attachment 164013 [details] Use old-style updateLogicalHeight View in context: https://bugs.webkit.org/attachment.cgi?id=164013&action=review Otherwise this looks about done. > Source/WebCore/rendering/RenderBlock.cpp:1397 > -void RenderBlock::computeInitialRegionRangeForBlock() > +void RenderBlock::updateRegionsAndExclusionsLogicalSize() Rehashing Hyatt's comment, this only deals with top and height, so the name seems wrong.

Comment on attachment 164013 [details] Use old-style updateLogicalHeight View in context: https://bugs.webkit.org/attachment.cgi?id=164013&action=review LGTM. >> Source/WebCore/rendering/RenderBlock.cpp:1397 >> +void RenderBlock::updateRegionsAndExclusionsLogicalSize() > > Rehashing Hyatt's comment, this only deals with top and height, so the name seems wrong. computeExlucsionShapeSize does indeed need and use width, so I take this back.

Comment on attachment 164013 [details] Use old-style updateLogicalHeight Clearing flags on attachment: 164013 Committed r128786: <http://trac.webkit.org/changeset/128786>

All reviewed patches have been landed. Closing bug.