Bug 78040

Summary: Add pixelSnappedX/Y/Width/Height methods
Product: WebKit Reporter: Emil A Eklund <eae>
Component: Layout and RenderingAssignee: Emil A Eklund <eae>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, hyatt, leviw, simon.fraser, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60318    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Emil A Eklund
Reported 2012-02-07 14:16:52 PST
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.
Attachments
Patch (21.09 KB, patch)
2012-02-07 15:39 PST, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Patch (21.77 KB, patch)
2012-02-07 16:09 PST, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Patch (20.51 KB, patch)
2012-02-07 16:59 PST, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Emil A Eklund
Comment 1 2012-02-07 15:39:22 PST
Created attachment 125945 [details] Patch
Eric Seidel (no email)
Comment 2 2012-02-07 15:58:12 PST
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
Comment 3 2012-02-07 16:09:17 PST
Created attachment 125949 [details] Patch
Emil A Eklund
Comment 4 2012-02-07 16:10:44 PST
> 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 5 2012-02-07 16:15:23 PST
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 6 2012-02-07 16:54:47 PST
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
Comment 7 2012-02-07 16:59:49 PST
Created attachment 125958 [details] Patch
Eric Seidel (no email)
Comment 8 2012-02-07 17:10:23 PST
Comment on attachment 125958 [details] Patch OK.
WebKit Review Bot
Comment 9 2012-02-07 19:56:44 PST
Comment on attachment 125958 [details] Patch Clearing flags on attachment: 125958 Committed r107032: <http://trac.webkit.org/changeset/107032>
WebKit Review Bot
Comment 10 2012-02-07 19:56:49 PST
All reviewed patches have been landed. Closing bug.
Simon Fraser (smfr)
Comment 11 2012-02-08 00:42:52 PST
Comment on attachment 125958 [details] Patch I think I would have preferred the term "aligned" to "snapped".
Note You need to log in before you can comment on or make changes to this bug.