Bug 83017

Summary: Switch baseline values to LayoutUnits in RenderTableSection.
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: Layout and RenderingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: eae, eric, jchaffraix
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60318    
Attachments:
Description Flags
Patch none

Levi Weintraub
Reported 2012-04-03 04:45:29 PDT
While we eventually layout table parts using integers, we should still keep track of baseline positions (which are combined with sub-pixel padding values) in sub-pixel units.
Attachments
Patch (4.24 KB, patch)
2012-04-03 05:01 PDT, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Levi Weintraub
Comment 1 2012-04-03 04:54:21 PDT
Updating the bug title since there's only one class this actually needs to happen in.
Levi Weintraub
Comment 2 2012-04-03 05:01:12 PDT
Created attachment 135312 [details] Patch
Julien Chaffraix
Comment 3 2012-04-03 14:08:17 PDT
Comment on attachment 135312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135312&action=review > Source/WebCore/rendering/RenderTableSection.cpp:391 > + m_rowPos[r + 1] = max<int>(m_rowPos[r + 1], m_grid[r].baseline + baselineDescent); It looks unfortunate that m_rowPos is actually using an integer here.
Emil A Eklund
Comment 4 2012-04-03 15:16:43 PDT
(In reply to comment #3) > (From update of attachment 135312 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135312&action=review > > > Source/WebCore/rendering/RenderTableSection.cpp:391 > > + m_rowPos[r + 1] = max<int>(m_rowPos[r + 1], m_grid[r].baseline + baselineDescent); > > It looks unfortunate that m_rowPos is actually using an integer here. If it makes the code simpler and it reduces the number of conversions we could change m_rowPos to a LayoutUnit. It'll always hold an integer value but we could use an ASSERT (or even the test results I suppose, this code has good coverage) to ensure that.
Julien Chaffraix
Comment 5 2012-04-03 15:24:56 PDT
Comment on attachment 135312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135312&action=review >>> Source/WebCore/rendering/RenderTableSection.cpp:391 >>> + m_rowPos[r + 1] = max<int>(m_rowPos[r + 1], m_grid[r].baseline + baselineDescent); >> >> It looks unfortunate that m_rowPos is actually using an integer here. > > If it makes the code simpler and it reduces the number of conversions we could change m_rowPos to a LayoutUnit. It'll always hold an integer value but we could use an ASSERT (or even the test results I suppose, this code has good coverage) to ensure that. I don't think that's worth it for now, considering that the rest of the table code is using int. However it sounds like we should be considering switching table layout to LayoutUnit at some point in the future.
Levi Weintraub
Comment 6 2012-04-04 03:38:27 PDT
Comment on attachment 135312 [details] Patch Landed in http://trac.webkit.org/changeset/113162
Note You need to log in before you can comment on or make changes to this bug.