Bug 84063

Summary: Make borderBoxRect sub-pixel precise and add a pixel snapped version
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: Layout and RenderingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: eae, eric, jchaffraix, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60318    
Attachments:
Description Flags
Patch
none
Patch for landing none

Levi Weintraub
Reported 2012-04-16 12:42:20 PDT
borderBoxRect is a mildly dangerous function in our sub-pixel world because it zero's the location of the box, which causes pixel snapping to be incorrect. First, we tried to fix this by making it integer only, but there are a couple places in the code where we actually need the sub-pixel size. We could either refactor the code that needs sub-pixel precision off of borderBoxRect (which should really just be renamed borderBoxSize since the location part of the rect is always empty) or make the fact that we're pixel snapping explicit in the other locations. I'm opting for the latter.
Attachments
Patch (15.10 KB, patch)
2012-04-16 13:22 PDT, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Patch for landing (15.12 KB, patch)
2012-04-16 15:24 PDT, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Levi Weintraub
Comment 1 2012-04-16 13:22:06 PDT
Created attachment 137389 [details] Patch
Eric Seidel (no email)
Comment 2 2012-04-16 13:54:40 PDT
Comment on attachment 137389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137389&action=review > Source/WebCore/ChangeLog:10 > + at (0,0), and therefor won't snap to the same size as the element it's covering. you mean "therefore" > Source/WebCore/rendering/RenderBox.h:458 > + bool hasVisualOverflow() const { return m_overflow && !borderBoxRect().contains(m_overflow->visualOverflowRect()); } Do we know what the concequences of having sub-pixel overflow detection here is? Can we test this?
Levi Weintraub
Comment 3 2012-04-16 15:12:31 PDT
(In reply to comment #2) > (From update of attachment 137389 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137389&action=review > > > Source/WebCore/ChangeLog:10 > > + at (0,0), and therefor won't snap to the same size as the element it's covering. > > you mean "therefore" Thanks :) > > > Source/WebCore/rendering/RenderBox.h:458 > > + bool hasVisualOverflow() const { return m_overflow && !borderBoxRect().contains(m_overflow->visualOverflowRect()); } > > Do we know what the concequences of having sub-pixel overflow detection here is? Can we test this? We've done extensive testing to assure that overflow handling happens properly if and only if there will actually be rendered overflow.
Levi Weintraub
Comment 4 2012-04-16 15:13:13 PDT
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 137389 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=137389&action=review > > > > > Source/WebCore/ChangeLog:10 > > > + at (0,0), and therefor won't snap to the same size as the element it's covering. > > > > you mean "therefore" > > Thanks :) > > > > > > Source/WebCore/rendering/RenderBox.h:458 > > > + bool hasVisualOverflow() const { return m_overflow && !borderBoxRect().contains(m_overflow->visualOverflowRect()); } > > > > Do we know what the concequences of having sub-pixel overflow detection here is? Can we test this? > > We've done extensive testing to assure that overflow handling happens properly if and only if there will actually be rendered overflow. Sorry, I meant to also say I'll add a test for that in our branch :)
Levi Weintraub
Comment 5 2012-04-16 15:24:41 PDT
Created attachment 137415 [details] Patch for landing
WebKit Review Bot
Comment 6 2012-04-16 16:24:34 PDT
Comment on attachment 137415 [details] Patch for landing Clearing flags on attachment: 137415 Committed r114315: <http://trac.webkit.org/changeset/114315>
WebKit Review Bot
Comment 7 2012-04-16 16:24:39 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.