Right now, only the FlowThreadController keeps the count of all auto height regions from all the flow threads. By making each flow thread keep a count of its auto height regions, we can perform specific operations for the two pass layout only for the flow threads that have auto height regions.
Created attachment 179752 [details] Patch
Comment on attachment 179752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179752&action=review Looks good. > Source/WebCore/rendering/RenderFlowThread.cpp:746 > + if (!needsLayout() || !hasAutoLogicalHeightRegions()) How is this working with dynamic changes from height: auto to some specific height? Is there a test for it? > Source/WebCore/rendering/RenderRegion.cpp:187 > + view()->flowThreadController()->incrementAutoLogicalHeightRegions(); Looks like view()->flowThreadController()->incrementAutoLogicalHeightRegions(); would actually look better on the FlowThread itself. Maybe we could rename it to something that says "number of flow threads that have auto-height-regions". I understand if that comes in a different bug though.
Created attachment 184181 [details] Patch 2
(In reply to comment #2) > (From update of attachment 179752 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179752&action=review > > Looks good. > > > Source/WebCore/rendering/RenderFlowThread.cpp:746 > > + if (!needsLayout() || !hasAutoLogicalHeightRegions()) > > How is this working with dynamic changes from height: auto to some specific height? Is there a test for it? > Yes, we have region-height-auto-to-defined.html and region-height-defined-to-auto.html (fast/regions) > > Source/WebCore/rendering/RenderRegion.cpp:187 > > + view()->flowThreadController()->incrementAutoLogicalHeightRegions(); > > Looks like view()->flowThreadController()->incrementAutoLogicalHeightRegions(); would actually look better on the FlowThread itself. Maybe we could rename it to something that says "number of flow threads that have auto-height-regions". I understand if that comes in a different bug though. Done.
Comment on attachment 184181 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=184181&action=review The change looks fine. Some comments on the naming. > Source/WebCore/ChangeLog:12 > + No new tests, change is covered by existing auto height tests. It's a performance refactoring, you don't expect a change in behavior (which is why you don't add new tests). > Source/WebCore/ChangeLog:41 > + (WebCore::RenderView::checkTwoPassLayoutForAutoHeightRegions): Filing in those details would help a lot in reading the changes. > Source/WebCore/rendering/FlowThreadController.cpp:103 > - ASSERT(isAutoLogicalHeightRegionsFlagConsistent()); > +#ifndef NDEBUG > + checkAutoLogicalHeightRegionsFlagConsistent(); I still prefer the ASSERT style here. That would remove the #ifndef need. > Source/WebCore/rendering/RenderFlowThread.cpp:744 > +bool RenderFlowThread::isAutoLogicalHeightRegionsFlagConsistent() const I don't understand this naming, especially the 'flag' part. Can't we just use 'count' instead of the pretty confusing flag? > Source/WebCore/rendering/RenderFlowThread.h:138 > + bool hasAutoLogicalHeightRegions() const { return m_autoLogicalHeightRegionsCount; } Nit: You could probably just put the |isAutoLogicalHeightRegionsFlagConsistent| here. It would mean some extra iterations over your regions in debug but it would catch inconsistent count more thoroughly.
Created attachment 184439 [details] Patch 3
Created attachment 184677 [details] Patch for landing
Comment on attachment 184677 [details] Patch for landing Rejecting attachment 184677 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 184677, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: e left on device patch: **** Can't create file /tmp/ppcebwTp : No space left on device patch: **** Can't create file /tmp/pppL5J5p : No space left on device patch: **** Can't create file /tmp/ppGzFxFp : No space left on device patch: **** Can't create file /tmp/ppibEkSp : No space left on device patch: **** Can't create file /tmp/ppA0Y8Gq : No space left on device Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/16124133
Created attachment 184940 [details] Patch for landing
Comment on attachment 184940 [details] Patch for landing Clearing flags on attachment: 184940 Committed r140948: <http://trac.webkit.org/changeset/140948>
All reviewed patches have been landed. Closing bug.