Bug 78054

Created attachment 126130 [details] Patch

Comment on attachment 126130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126130&action=review > Source/WebCore/rendering/style/RenderStyle.cpp:889 > + RoundedRect roundedRect(pixelSnappedIntRect(borderRect)); This sort of thing makes me think that we should not allow transparent conversion from LayoutRect to IntRect/FloatRect, and that all callsites should have to choose one or the other. That might help make it clear when we're snapping vs. not.

Comment on attachment 126130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126130&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:713 > + viewRect.intersect(pixelSnappedIntRect(contentRect)); I would have done this when assigning the local, and made the local IntRect contentRect = pixelSnappedIntRect(m_renderer->absoluteClippedOverflowRect()). But this is part of my general feeling that only the layout code should deal with Layout*, and anyone ouside of layout will need to make an explicit choice if they want an int rect or float rect,a nd if they want it snapped or not. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2633 > + return m_renderer->document()->view()->contentsToScreen(pixelSnappedIntRect(ourrect)); I probably would have used a local here to avoid two separate snapped calls. > Source/WebCore/html/HTMLCanvasElement.cpp:287 > + context->drawImageBuffer(imageBuffer, ColorSpaceDeviceRGB, pixelSnappedIntRect(r), CompositeSourceOver, useLowQualityScale); This makes me wonder why we don't define pixelSnappedIntRect() on LayoutRect directly? (And until then on IntRect directly?) > Source/WebCore/html/shadow/TextControlInnerElements.cpp:553 > + IntRect rect = pixelSnappedIntRect(renderer()->view()->frameView()->contentsToWindow(renderer()->absoluteBoundingBoxRectIgnoringTransforms())); What a horribly complicated line.

Comment on attachment 126130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126130&action=review >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:713 >> + viewRect.intersect(pixelSnappedIntRect(contentRect)); > > I would have done this when assigning the local, and made the local IntRect contentRect = pixelSnappedIntRect(m_renderer->absoluteClippedOverflowRect()). But this is part of my general feeling that only the layout code should deal with Layout*, and anyone ouside of layout will need to make an explicit choice if they want an int rect or float rect,a nd if they want it snapped or not. Seems reasonable. >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2633 >> + return m_renderer->document()->view()->contentsToScreen(pixelSnappedIntRect(ourrect)); > > I probably would have used a local here to avoid two separate snapped calls. It's only one actual call, since only one of these is compiled depending on the platform. >> Source/WebCore/html/HTMLCanvasElement.cpp:287 >> + context->drawImageBuffer(imageBuffer, ColorSpaceDeviceRGB, pixelSnappedIntRect(r), CompositeSourceOver, useLowQualityScale); > > This makes me wonder why we don't define pixelSnappedIntRect() on LayoutRect directly? (And until then on IntRect directly?) We followed the enclosingIntRect convention by making this a global method. Neither seems clearer to me, so I'm happy to go the other way if you have strong feelings about it. >> Source/WebCore/html/shadow/TextControlInnerElements.cpp:553 >> + IntRect rect = pixelSnappedIntRect(renderer()->view()->frameView()->contentsToWindow(renderer()->absoluteBoundingBoxRectIgnoringTransforms())); > > What a horribly complicated line. :( >> Source/WebCore/rendering/style/RenderStyle.cpp:889 >> + RoundedRect roundedRect(pixelSnappedIntRect(borderRect)); > > This sort of thing makes me think that we should not allow transparent conversion from LayoutRect to IntRect/FloatRect, and that all callsites should have to choose one or the other. That might help make it clear when we're snapping vs. not. I agree!

Created attachment 126387 [details] Patch

Comment on attachment 126387 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126387&action=review I'm willing to aprove this and let you all move foward. But I think you/we shoudl focus on making the boundaries crystal clear. It's less important that we get each spot right, more than that it's easy to tell when a spot is right or wrong. Right now it's very difficult to tell what should be a layout rect vs. an IntRect because the rules are not clear (at least to me). We need to make them clear. NOt just for me, but for all the others who are going to touch this code and be confused as to what type of rect to use where. > Source/WebCore/page/FrameView.cpp:1777 > + unionedRect.unite(pixelSnappedIntRect(m_repaintRects[i])); Isn't a repaint rect a pixel rect by definition already? And thus an IntRect and not a LayoutRect? > Source/WebCore/rendering/RenderObject.cpp:968 > - graphicsContext->drawRect(IntRect(x1, y1, x2 - x1, y2 - y1)); > + graphicsContext->drawRect(pixelSnappedIntRect(LayoutRect(x1, y1, x2 - x1, y2 - y1))); Feels a little odd that we convert from int to layout to int here...

Committed r107461: <http://trac.webkit.org/changeset/107461>