Bug 77705 – Clean-up RenderTableSection::calcRowLogicalHeight

• Component: Tables
• Status: RESOLVED FIXED

[Julien Chaffraix]

Currently the function is pretty complicated to understand as we reuse some variables, whose naming is not great, some unneeded variables and the function could be split a bit.

Patch forthcoming.

[Julien Chaffraix]

Created attachment 125232 [details]
Proposed simplification.

[Eric Seidel (no email)]

Comment on attachment 125232 [details]
Proposed simplification.

This looks reasonable to me.  I hope you plan to continue cleanup here. :)

[Robert Hogan]

Comment on attachment 125232 [details]
Proposed simplification.

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

> Source/WebCore/rendering/RenderTableCell.cpp:102
> +    if (document()->inQuirksMode() || style()->boxSizing() == BORDER_BOX) {
> +        // Explicit heights use the border box in quirks mode.
> +        // Don't adjust height.
> +    } else {

This seemed odd to me at first but I can see why you do it - it avoids the need to use the same return statement twice or write hard-to-read conditions. But is that a good enough reason to avoid the early return?

[Julien Chaffraix]

Comment on attachment 125232 [details]
Proposed simplification.

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

>> Source/WebCore/rendering/RenderTableCell.cpp:102
>> +    } else {
> 
> This seemed odd to me at first but I can see why you do it - it avoids the need to use the same return statement twice or write hard-to-read conditions. But is that a good enough reason to avoid the early return?

I wanted to keep the structure of the old code (including the comment added in bug 69425) to ease review. I don't think an early return would be more readable but it's arguable in both ways. If you see a better way to structure this code, I would be happy to review such a change!

[WebKit Review Bot]

Comment on attachment 125232 [details]
Proposed simplification.

Rejecting attachment 125232 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
fuzz 3.
patching file Source/WebCore/rendering/RenderTableCell.cpp
patching file Source/WebCore/rendering/RenderTableCell.h
patching file Source/WebCore/rendering/RenderTableSection.cpp
Hunk #1 FAILED at 334.
Hunk #3 FAILED at 373.
2 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/rendering/RenderTableSection.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Seidel']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11559604

[Julien Chaffraix]

Committed r108557: <http://trac.webkit.org/changeset/108557>

[Nikolas Zimmermann]

This made zooming fail on svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm - a <p> with display: table-cell, isn't resizing correctly anymore.

I reverted this in r108892. Bug 79455 tracks the chromium changes that can now be reverted as well.
See bug 78613 which looked to be faulty, but wasn't, it was this comment which broke the file.

[Nikolas Zimmermann]

(In reply to comment #7)
> This made zooming fail on svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm - a <p> with display: table-cell, isn't resizing correctly anymore.
> 
> I reverted this in r108892. Bug 79455 tracks the chromium changes that can now be reverted as well.
> See bug 78613 which looked to be faulty, but wasn't, it was this comment which broke the file.
s/comment/commit/, s/file/test/ , too early for me ;-)
In bug 78613 there's also an explanation whats going on with the testcase, it might be helpful to fix this bug.

[Julien Chaffraix]

Created attachment 128873 [details]
Fixed change: properly reset the RowStruct baseline to do the right calculation, covered by the failing SVG test.

[Nikolas Zimmermann]

Comment on attachment 128873 [details]
Fixed change: properly reset the RowStruct baseline to do the right calculation, covered by the failing SVG test.

Thanks Julien for the quick investigation, r=me.

[WebKit Review Bot]

Comment on attachment 128873 [details]
Fixed change: properly reset the RowStruct baseline to do the right calculation, covered by the failing SVG test.

Clearing flags on attachment: 128873

Committed r108914: <http://trac.webkit.org/changeset/108914>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.