Bug 78040 – Add pixelSnappedX/Y/Width/Height methods

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

[Emil A Eklund]

Add pixel snapped versions of x/y/width/height methods. These return the same value as the x/w/width/height methods for now but once we move over to sub pixel layout they will snap the subpixel value to a device pixel and return an integer value.

When snapping the left and top edge is simply rounded to the nearest device pixel.
The right and bottom edges are computed by subtracting the rounded left/top edge from the precise location and size. This ensures that the edges all line up with device pixels and that the total size of an object, including borders, is at most one pixel off.

x: round(x)
y: round(y)
maxX: round(x + width) - round(x)
maxY: round(y + height) - round(y)

We use the term pixel snapped to indicate that the numbers are not merely rounded. Aligned might have been a better name but the term is already quite overloaded.

[Emil A Eklund]

Created attachment 125945 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 125945 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.h:579
> +        int pixelSnappedX() const { return x(); }
> +        int pixelSnappedMaxX() const { return maxX(); }
> +        int pixelSnappedY() const { return y(); }
> +        int pixelSnappedMaxY() const { return maxY(); }
> +        int pixelSnappedWidth() const { return width(); }
> +        int pixelSnappedHeight() const { return height(); }

You should add a note/FIXME next to functions like this that will change when we move to app-units.

> Source/WebCore/rendering/RenderBlock.h:618
> +    int pixelSnappedLogicalTopForFloat(const FloatingObject* child) const { return logicalTopForFloat(child); }
> +    int pixelSnappedLogicalBottomForFloat(const FloatingObject* child) const { return logicalBottomForFloat(child); }
> +    int pixelSnappedLogicalLeftForFloat(const FloatingObject* child) const { return logicalLeftForFloat(child); }
> +    int pixelSnappedLogicalRightForFloat(const FloatingObject* child) const { return logicalRightForFloat(child); }

This method name sounds like you'e supposed to pass it a "float".  But I'm not sure I have better suggestions.

[Emil A Eklund]

Created attachment 125949 [details]
Patch

[Emil A Eklund]

> You should add a note/FIXME next to functions like this that will change when we move to app-units.

Added a couple of FIXMEs, let me know if I went overboard.


> > +    int pixelSnappedLogicalRightForFloat(const FloatingObject* child) const { return logicalRightForFloat(child); }
> 
> This method name sounds like you'e supposed to pass it a "float".  But I'm not sure I have better suggestions.

It's unfortunate that Float is overloaded to mean both a floating object and floating point. Hopefully the context will make this distinction obvious.

[Eric Seidel (no email)]

Comment on attachment 125949 [details]
Patch

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

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:271
> -    String widthNumberPart = " " + String::number(modelObject ? adjustForAbsoluteZoom(modelObject->offsetWidth(), modelObject) : boundingBox.width());
> +    String widthNumberPart = " " + String::number(modelObject ? adjustForAbsoluteZoom(modelObject->pixelSnappedOffsetWidth(), modelObject) : boundingBox.width());

Are we intentionally adjusting for zoom after snapping to pixel boundaries?

> Source/WebCore/rendering/RenderTreeAsText.cpp:623
> +        if (l.renderBox() && l.renderBox()->pixelSnappedClientWidth() != l.pixelSnappedScrollWidth())
> +            ts << " scrollWidth " << l.pixelSnappedScrollWidth();
> +        if (l.renderBox() && l.renderBox()->pixelSnappedClientHeight() != l.pixelSnappedScrollHeight())
> +            ts << " scrollHeight " << l.pixelSnappedScrollHeight();

Do we need a fixme about dumping non-snapped values eventually?  Or should we always dump snapped values?

[Emil A Eklund]

Comment on attachment 125949 [details]
Patch

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

>> Source/WebCore/inspector/DOMNodeHighlighter.cpp:271
>> +    String widthNumberPart = " " + String::number(modelObject ? adjustForAbsoluteZoom(modelObject->pixelSnappedOffsetWidth(), modelObject) : boundingBox.width());
> 
> Are we intentionally adjusting for zoom after snapping to pixel boundaries?

Good catch, we should clearly not round before adjusting for zoom.

>> Source/WebCore/rendering/RenderTreeAsText.cpp:623
>> +            ts << " scrollHeight " << l.pixelSnappedScrollHeight();
> 
> Do we need a fixme about dumping non-snapped values eventually?  Or should we always dump snapped values?

For these values we probably always want them to be snapped as that best represents what we are painting.

[Emil A Eklund]

Created attachment 125958 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 125958 [details]
Patch

OK.

[WebKit Review Bot]

Comment on attachment 125958 [details]
Patch

Clearing flags on attachment: 125958

Committed r107032: <http://trac.webkit.org/changeset/107032>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 125958 [details]
Patch

I think I would have preferred the term "aligned" to "snapped".