Bug 81678 - Make distributeExtraLogicalHeightToRows return the consumed logical height
Summary: Make distributeExtraLogicalHeightToRows return the consumed logical height
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-20 10:18 PDT by Julien Chaffraix
Modified: 2012-03-20 14:02 PDT (History)
2 users (show)

See Also:


Attachments
Proposed fix 1. (8.79 KB, patch)
2012-03-20 10:29 PDT, Julien Chaffraix
tony: review+
jchaffraix: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-03-20 10:18:04 PDT
Currently this function returns the remaining logical height. This is painful as the distribution algorithm is interested in consumed logical height to decrease the available logical height. This also means that there will be a need for some unneeded house-keeping.

Let's change that.
Comment 1 Julien Chaffraix 2012-03-20 10:29:10 PDT
Created attachment 132847 [details]
Proposed fix 1.
Comment 2 Tony Chang 2012-03-20 11:19:11 PDT
Comment on attachment 132847 [details]
Proposed fix 1.

View in context: https://bugs.webkit.org/attachment.cgi?id=132847&action=review

> Source/WebCore/rendering/RenderTableSection.cpp:427
> -int RenderTableSection::distributeExtraLogicalHeightToPercentRows(int extraLogicalHeight, int totalPercent)
> +void RenderTableSection::distributeExtraLogicalHeightToPercentRows(int& extraLogicalHeight, int totalPercent)

Nit: Maybe we should reverse the parameter order. I think in/out params normally come last.

> Source/WebCore/rendering/RenderTableSection.cpp:454
> -int RenderTableSection::distributeExtraLogicalHeightToAutoRows(int extraLogicalHeight, unsigned autoRowsCount)
> +void RenderTableSection::distributeExtraLogicalHeightToAutoRows(int& extraLogicalHeight, unsigned autoRowsCount)

Ditto.
Comment 3 WebKit Review Bot 2012-03-20 11:30:21 PDT
Comment on attachment 132847 [details]
Proposed fix 1.

Attachment 132847 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12009301

New failing tests:
fast/dom/error-to-string-stack-overflow.html
Comment 4 Julien Chaffraix 2012-03-20 12:55:37 PDT
Comment on attachment 132847 [details]
Proposed fix 1.

View in context: https://bugs.webkit.org/attachment.cgi?id=132847&action=review

The test failing is unrelated to my change (I am hitting it locally without the patch).

>> Source/WebCore/rendering/RenderTableSection.cpp:427
>> +void RenderTableSection::distributeExtraLogicalHeightToPercentRows(int& extraLogicalHeight, int totalPercent)
> 
> Nit: Maybe we should reverse the parameter order. I think in/out params normally come last.

I prefer it this way as it looks like |totalPercent| is an added parameter compared to distributeExtraLogicalHeightToRows.
Comment 5 Julien Chaffraix 2012-03-20 14:02:51 PDT
Committed r111435: <http://trac.webkit.org/changeset/111435>