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.
Created attachment 125232 [details] Proposed simplification.
Comment on attachment 125232 [details] Proposed simplification. This looks reasonable to me. I hope you plan to continue cleanup here. :)
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 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 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
Committed r108557: <http://trac.webkit.org/changeset/108557>
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.
(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.
Created attachment 128873 [details] Fixed change: properly reset the RowStruct baseline to do the right calculation, covered by the failing SVG test.
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 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>
All reviewed patches have been landed. Closing bug.