Bug 78647 – Switch drawLineForBoxSide to use integers

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

[Levi Weintraub]

drawLineForBoxSide handles painting lines for boxes which it's important to be done on pixel boundaries. Its interface doesn't make it possible to pixel snap properly within the function itself -- it draws one side of the box at a time, and the logical right and bottom lines can only be properly determined using the logical top and left positions -- so it needs to be treated like a graphics context function, whereby the caller handles the proper pixel snapping before passing the values in.

[Levi Weintraub]

Created attachment 127094 [details]
Patch

[Darin Adler]

Comment on attachment 127094 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127094&action=review

> Source/WebCore/rendering/RenderBlock.cpp:2506
> +                // FIXME: These values will be pixel snapped before being passed to drawLineForBoxSide.

Why does this code have to wait? Can’t we add some pixel-snaping inline functions now that do nothing when fractional positioning is off?

[Levi Weintraub]

Comment on attachment 127094 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127094&action=review

>> Source/WebCore/rendering/RenderBlock.cpp:2506
>> +                // FIXME: These values will be pixel snapped before being passed to drawLineForBoxSide.
> 
> Why does this code have to wait? Can’t we add some pixel-snaping inline functions now that do nothing when fractional positioning is off?

Here is the code being left out: http://trac.webkit.org/changeset/107640/branches/subpixellayout/Source/WebCore/rendering/RenderBlock.cpp

There are a few reasons why I've chosen to not use entirely inline functions to get trunk looking like our subpixel branch:
- There are places where pixel snapping involves calling additional non-inline functions, the results of which will just be thrown out.
- We'll need to use explicit wrapping on constants (think zero mostly) used in places like ternary operators.
- I'm partial to putting methods like round() directly on the FractionalLayoutUnit class, which we obviously can't do for integral types.

The last point was my rationale here. If you disagree, I'll happily go the inline route.

[Darin Adler]

Comment on attachment 127094 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127094&action=review

Please consider my comments.

>>> Source/WebCore/rendering/RenderBlock.cpp:2506
>>> +                // FIXME: These values will be pixel snapped before being passed to drawLineForBoxSide.
>> 
>> Why does this code have to wait? Can’t we add some pixel-snaping inline functions now that do nothing when fractional positioning is off?
> 
> Here is the code being left out: http://trac.webkit.org/changeset/107640/branches/subpixellayout/Source/WebCore/rendering/RenderBlock.cpp
> 
> There are a few reasons why I've chosen to not use entirely inline functions to get trunk looking like our subpixel branch:
> - There are places where pixel snapping involves calling additional non-inline functions, the results of which will just be thrown out.
> - We'll need to use explicit wrapping on constants (think zero mostly) used in places like ternary operators.
> - I'm partial to putting methods like round() directly on the FractionalLayoutUnit class, which we obviously can't do for integral types.
> 
> The last point was my rationale here. If you disagree, I'll happily go the inline route.

> There are places where pixel snapping involves calling additional functions, the results of which will just be thrown out.

I’d like to raise the level of abstraction a bit and use a well named function rather than four lines of code in cases like this:

    int pixelSnappedRuleLeft = ruleLeft.round(); 
    int pixelSnappedRuleRight = snapSizeToPixel(ruleRight - ruleLeft, ruleLeft) + pixelSnappedRuleLeft; 
    int pixelSnappedRuleTop = ruleTop.round(); 
    int pixelSnappedRuleBottom = snapSizeToPixel(ruleBottom - ruleTop, ruleTop) + pixelSnappedRuleTop; 

I think this should be a function that takes a rectangle or other struct and returns one rather than code written out like this. The name of the function can be the best documentation for something like this.

I also think that drawLineForBoxSide should take a struct, not four separate layout unit arguments.

> We'll need to use explicit wrapping on constants (think zero mostly) used in places like ternary operators.

I think that best for this is a named constant zeroLayoutUnit or something like that. Actual explicit wrapping is far inferior.

> I'm partial to putting methods like round() directly on the FractionalLayoutUnit class, which we obviously can't do for integral types.

I don’t think that member functions are better than non-member functions for this sort of thing. I’d like to see us add these functions and start using them in TOT instead of having adding them be a part of flipping the switch. I’d like to see almost no code changes at all required when flipping the switch.

[Levi Weintraub]

(In reply to comment #4)
> (From update of attachment 127094 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127094&action=review
> 
> Please consider my comments.
> 
> >>> Source/WebCore/rendering/RenderBlock.cpp:2506
> >>> +                // FIXME: These values will be pixel snapped before being passed to drawLineForBoxSide.
> >> 
> >> Why does this code have to wait? Can’t we add some pixel-snaping inline functions now that do nothing when fractional positioning is off?
> > 
> > Here is the code being left out: http://trac.webkit.org/changeset/107640/branches/subpixellayout/Source/WebCore/rendering/RenderBlock.cpp
> > 
> > There are a few reasons why I've chosen to not use entirely inline functions to get trunk looking like our subpixel branch:
> > - There are places where pixel snapping involves calling additional non-inline functions, the results of which will just be thrown out.
> > - We'll need to use explicit wrapping on constants (think zero mostly) used in places like ternary operators.
> > - I'm partial to putting methods like round() directly on the FractionalLayoutUnit class, which we obviously can't do for integral types.
> > 
> > The last point was my rationale here. If you disagree, I'll happily go the inline route.
> 
> > There are places where pixel snapping involves calling additional functions, the results of which will just be thrown out.
> 
> I’d like to raise the level of abstraction a bit and use a well named function rather than four lines of code in cases like this:
> 
>     int pixelSnappedRuleLeft = ruleLeft.round(); 
>     int pixelSnappedRuleRight = snapSizeToPixel(ruleRight - ruleLeft, ruleLeft) + pixelSnappedRuleLeft; 
>     int pixelSnappedRuleTop = ruleTop.round(); 
>     int pixelSnappedRuleBottom = snapSizeToPixel(ruleBottom - ruleTop, ruleTop) + pixelSnappedRuleTop; 
> 
> I think this should be a function that takes a rectangle or other struct and returns one rather than code written out like this. The name of the function can be the best documentation for something like this.

Good point.

> 
> I also think that drawLineForBoxSide should take a struct, not four separate layout unit arguments.

Agreed. It only ends up being really ugly in RenderInline::paintOutlineForLine.

> 
> > We'll need to use explicit wrapping on constants (think zero mostly) used in places like ternary operators.
> 
> I think that best for this is a named constant zeroLayoutUnit or something like that. Actual explicit wrapping is far inferior.

We've been trying to avoid explicit wrapping wherever possible, but there are quite a few un-named constants that seem to come from nowhere. Just adding a zero constant would solve many of these... I'll create that one.

> 
> > I'm partial to putting methods like round() directly on the FractionalLayoutUnit class, which we obviously can't do for integral types.
> 
> I don’t think that member functions are better than non-member functions for this sort of thing. I’d like to see us add these functions and start using them in TOT instead of having adding them be a part of flipping the switch. I’d like to see almost no code changes at all required when flipping the switch.

Thanks for sharing your thoughts. I'll work to make that happen.

[Levi Weintraub]

Created attachment 127253 [details]
Patch

[Levi Weintraub]

(In reply to comment #6)
> Created an attachment (id=127253) [details]
> Patch

Hey Darin, I think this is a better patch (nothing is left out :)

Mind taking another look?

[Levi Weintraub]

Darin? Eric? Take a look?

[Levi Weintraub]

One more friendly ping :)

[Eric Seidel (no email)]

Comment on attachment 127253 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127253&action=review

> Source/WebCore/rendering/RenderBlock.cpp:2537
> +            IntRect pixelSnappedRuleRect = pixelSnappedIntRect(ruleRect);
>              drawLineForBoxSide(paintInfo.context, ruleRect.x(), ruleRect.y(), ruleRect.maxX(), ruleRect.maxY(), boxSide, ruleColor, ruleStyle, 0, 0, antialias);

Um...

[Levi Weintraub]

Created attachment 128525 [details]
Patch

[Levi Weintraub]

(In reply to comment #10)
> (From update of attachment 127253 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127253&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:2537
> > +            IntRect pixelSnappedRuleRect = pixelSnappedIntRect(ruleRect);
> >              drawLineForBoxSide(paintInfo.context, ruleRect.x(), ruleRect.y(), ruleRect.maxX(), ruleRect.maxY(), boxSide, ruleColor, ruleStyle, 0, 0, antialias);
> 
> Um...

Good catch... take another look?

[Eric Seidel (no email)]

Comment on attachment 128525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128525&action=review

> Source/WebCore/rendering/RenderBlock.cpp:2597
> +                drawLineForBoxSide(paintInfo.context, pixelSnappedRuleRect.x(), pixelSnappedRuleRect.y(), pixelSnappedRuleRect.maxX(), pixelSnappedRuleRect.maxY(), boxSide, ruleColor, ruleStyle, 0, 0, antialias);

Seems we should have a version of this method which takes a rect?  Possibly a LayoutRect event...

> Source/WebCore/rendering/RenderInline.cpp:1459
> +    IntRect pixelSnappedBox = pixelSnappedIntRect(box);

I would have used "snappedBox" or something shorter.  pixelBox?  intBox?

> Source/WebCore/rendering/RenderInline.cpp:1534
> +            pixelSnappedBox.maxY(),
> +            pixelSnappedBox.maxX() + outlineWidth,
> +            pixelSnappedBox.maxY() + outlineWidth,

This seems like a theme... seems like we should have a helper method to help with this theme...

> Source/WebCore/rendering/RenderObject.cpp:1184
> +    int leftOuter = outer.x();
> +    int leftInner = inner.x();
> +    int rightOuter = outer.maxX();
> +    int rightInner = inner.maxX();

Oh, split coords, you are so error prone. :(

[Levi Weintraub]

Comment on attachment 128525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128525&action=review

>> Source/WebCore/rendering/RenderBlock.cpp:2597
>> +                drawLineForBoxSide(paintInfo.context, pixelSnappedRuleRect.x(), pixelSnappedRuleRect.y(), pixelSnappedRuleRect.maxX(), pixelSnappedRuleRect.maxY(), boxSide, ruleColor, ruleStyle, 0, 0, antialias);
> 
> Seems we should have a version of this method which takes a rect?  Possibly a LayoutRect event...

We could turn the current contract into an inline version that builds an IntRect for the real implementation. We wouldn't want to use a LayoutRect though... we don't have enough information by the time we get into this function to do pixel snapping in the RenderInline case.

[WebKit Review Bot]

Comment on attachment 128525 [details]
Patch

Clearing flags on attachment: 128525

Committed r108719: <http://trac.webkit.org/changeset/108719>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.