Bug 84063 – Make borderBoxRect sub-pixel precise and add a pixel snapped version

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Levi Weintraub]

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.

[Levi Weintraub]

Created attachment 137389 [details]
Patch

[Eric Seidel (no email)]

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]

(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]

(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]

Created attachment 137415 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 137415 [details]
Patch for landing

Clearing flags on attachment: 137415

Committed r114315: <http://trac.webkit.org/changeset/114315>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.