Bug 78647

Created attachment 127094 [details] Patch

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?

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.

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.

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

Created attachment 127253 [details] Patch

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

Darin? Eric? Take a look?

One more friendly ping :)

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...

Created attachment 128525 [details] Patch

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

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. :(

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.

Comment on attachment 128525 [details] Patch Clearing flags on attachment: 128525 Committed r108719: <http://trac.webkit.org/changeset/108719>

All reviewed patches have been landed. Closing bug.