Summary: | [CSSRegions] RenderFlowThread should keep a count of auto height regions | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mihnea Ovidenie <mihnea> | ||||||||||||
Component: | CSS | Assignee: | Mihnea Ovidenie <mihnea> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | eric, jchaffraix, ojan.autocc, WebkitBugTracker, webkit.review.bot | ||||||||||||
Priority: | P2 | Keywords: | AdobeTracked | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Mihnea Ovidenie
2012-12-17 08:33:30 PST
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. |