[webkit-reviews] review granted: [Bug 78647] Switch drawLineForBoxSide to use integers : [Attachment 127094] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Levi Weintraub <leviw at chromium.org>'s
request for review:
Bug 78647: Switch drawLineForBoxSide to use integers
https://bugs.webkit.org/show_bug.cgi?id=78647

Attachment 127094: Patch
https://bugs.webkit.org/attachment.cgi?id=127094&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.