Bug 77705 - Clean-up RenderTableSection::calcRowLogicalHeight
Summary: Clean-up RenderTableSection::calcRowLogicalHeight
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: 79455
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-02 18:48 PST by Julien Chaffraix
Modified: 2012-02-25 16:19 PST (History)
3 users (show)

See Also:


Attachments
Proposed simplification. (7.82 KB, patch)
2012-02-02 19:00 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Fixed change: properly reset the RowStruct baseline to do the right calculation, covered by the failing SVG test. (8.00 KB, patch)
2012-02-25 10:37 PST, Julien Chaffraix
no flags 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-02-02 18:48:05 PST
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.
Comment 1 Julien Chaffraix 2012-02-02 19:00:52 PST
Created attachment 125232 [details]
Proposed simplification.
Comment 2 Eric Seidel (no email) 2012-02-22 12:56:40 PST
Comment on attachment 125232 [details]
Proposed simplification.

This looks reasonable to me.  I hope you plan to continue cleanup here. :)
Comment 3 Robert Hogan 2012-02-22 13:14:30 PST
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?
Comment 4 Julien Chaffraix 2012-02-22 13:40:45 PST
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!
Comment 5 WebKit Review Bot 2012-02-22 13:45:23 PST
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
Comment 6 Julien Chaffraix 2012-02-22 15:10:43 PST
Committed r108557: <http://trac.webkit.org/changeset/108557>
Comment 7 Nikolas Zimmermann 2012-02-25 01:48:09 PST
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.
Comment 8 Nikolas Zimmermann 2012-02-25 02:47:52 PST
(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.
Comment 9 Julien Chaffraix 2012-02-25 10:37:32 PST
Created attachment 128873 [details]
Fixed change: properly reset the RowStruct baseline to do the right calculation, covered by the failing SVG test.
Comment 10 Nikolas Zimmermann 2012-02-25 14:59:41 PST
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.
Comment 11 WebKit Review Bot 2012-02-25 16:19:43 PST
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>
Comment 12 WebKit Review Bot 2012-02-25 16:19:47 PST
All reviewed patches have been landed.  Closing bug.