Bug 105185 – [CSSRegions] RenderFlowThread should keep a count of auto height regions

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Mihnea Ovidenie]

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.

[Mihnea Ovidenie]

Created attachment 179752 [details]
Patch

[Alexandru Chiculita]

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.

[Mihnea Ovidenie]

Created attachment 184181 [details]
Patch 2

[Mihnea Ovidenie]

(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.

[Julien Chaffraix]

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.

[Mihnea Ovidenie]

Created attachment 184439 [details]
Patch 3

[Mihnea Ovidenie]

Created attachment 184677 [details]
Patch for landing

[WebKit Review Bot]

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

[Mihnea Ovidenie]

Created attachment 184940 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 184940 [details]
Patch for landing

Clearing flags on attachment: 184940

Committed r140948: <http://trac.webkit.org/changeset/140948>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.