Summary: | Add pixelSnappedX/Y/Width/Height methods | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Emil A Eklund <eae> | ||||||||
Component: | Layout and Rendering | Assignee: | 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
Emil A Eklund
2012-02-07 14:16:52 PST
Created attachment 125945 [details]
Patch
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. Created attachment 125949 [details]
Patch
> 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. 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? 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. Created attachment 125958 [details]
Patch
Comment on attachment 125958 [details]
Patch
OK.
Comment on attachment 125958 [details] Patch Clearing flags on attachment: 125958 Committed r107032: <http://trac.webkit.org/changeset/107032> All reviewed patches have been landed. Closing bug. Comment on attachment 125958 [details]
Patch
I think I would have preferred the term "aligned" to "snapped".
|