ssia
Created attachment 222889 [details] Patch
Created attachment 222901 [details] Patch
Comment on attachment 222901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222901&action=review > Source/WebCore/platform/LayoutUnit.h:942 > +inline int snapSizeToPixel(LayoutUnit size, LayoutUnit location) I don't like "size" because we have IntSize/FloatSize with have widths and heights. > Source/WebCore/platform/LayoutUnit.h:948 > +inline float snapSizeToDevicePixel(LayoutUnit size, LayoutUnit location, float devicePixelRatio) Ditto. Do we ever use these on things that aren't rects? > Source/WebCore/platform/graphics/GraphicsContext.cpp:91 > + , m_paintPixelRatio(2) How can you assume 2 here? > Source/WebCore/platform/graphics/GraphicsContext.h:447 > + float paintPixelRatio() const { return m_paintPixelRatio; } We may have to rethink this name. Reading it in isolation here doesn't communicate what it's for.
(In reply to comment #3) > (From update of attachment 222901 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222901&action=review > > > Source/WebCore/platform/LayoutUnit.h:942 > > +inline int snapSizeToPixel(LayoutUnit size, LayoutUnit location) > > I don't like "size" because we have IntSize/FloatSize with have widths and heights. > > > Source/WebCore/platform/LayoutUnit.h:948 > > +inline float snapSizeToDevicePixel(LayoutUnit size, LayoutUnit location, float devicePixelRatio) > > Ditto. ok. > Do we ever use these on things that aren't rects? No. They are part of the rect snapping logic, so it does not make much sense to use them standalone. > > > Source/WebCore/platform/graphics/GraphicsContext.cpp:91 > > + , m_paintPixelRatio(2) > > How can you assume 2 here? I assumed we support iOS retina only. I could be mistaken here. > > > Source/WebCore/platform/graphics/GraphicsContext.h:447 > > + float paintPixelRatio() const { return m_paintPixelRatio; } > > We may have to rethink this name. Reading it in isolation here doesn't communicate what it's for. I had devicePixelRatioForPaiting perviously. Does that sound better?
Comment on attachment 222901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222901&action=review >>> Source/WebCore/platform/graphics/GraphicsContext.cpp:91 >>> + , m_paintPixelRatio(2) >> >> How can you assume 2 here? > > I assumed we support iOS retina only. I could be mistaken here. I don't think you should hardcode that assumption. >>> Source/WebCore/platform/graphics/GraphicsContext.h:447 >>> + float paintPixelRatio() const { return m_paintPixelRatio; } >> >> We may have to rethink this name. Reading it in isolation here doesn't communicate what it's for. > > I had devicePixelRatioForPaiting perviously. Does that sound better? pixelSnappingFactor()?
(In reply to comment #5) > (From update of attachment 222901 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222901&action=review > > >>> Source/WebCore/platform/graphics/GraphicsContext.cpp:91 > >>> + , m_paintPixelRatio(2) > >> > >> How can you assume 2 here? > > > > I assumed we support iOS retina only. I could be mistaken here. > > I don't think you should hardcode that assumption. Ok, I'll change that. > > >>> Source/WebCore/platform/graphics/GraphicsContext.h:447 > >>> + float paintPixelRatio() const { return m_paintPixelRatio; } > >> > >> We may have to rethink this name. Reading it in isolation here doesn't communicate what it's for. > > > > I had devicePixelRatioForPaiting perviously. Does that sound better? > > pixelSnappingFactor()? Much better. Thanks.
> > Do we ever use these on things that aren't rects? > No. They are part of the rect snapping logic, so it does not make much sense to use them standalone. hmm, actually we do use them in a few places outside of LayoutRect for example to snap offsetWidth/Height. This naming confused me too in the past, but I can't think of anything better now.
Created attachment 222911 [details] Patch
Comment on attachment 222911 [details] Patch Clearing flags on attachment: 222911 Committed r163265: <http://trac.webkit.org/changeset/163265>
All reviewed patches have been landed. Closing bug.
Comment on attachment 222911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222911&action=review > Source/WebCore/platform/LayoutUnit.h:940 > + return round((value.rawValue() * pixelSnappingFactor) / kEffectiveFixedPointDenominator) / pixelSnappingFactor; Needs to be roundf to avoid converting to a double, rounding, and then converting back to a float. > Source/WebCore/platform/LayoutUnit.h:945 > + return floor((value.rawValue() * pixelSnappingFactor) / kEffectiveFixedPointDenominator) / pixelSnappingFactor; Needs to be floorf to avoid converting to a double, rounding, and then converting back to a float. > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:143 > + ASSERT(abs(baseDeviceMatrix.a) == abs(baseDeviceMatrix.d)); The function abs takes an int, and I assume that conversion to integer isn’t what we want here. I suggest using fabs, even though it will result in converting float to double on 32-bit platforms, because this is just an assertion.
(In reply to comment #11) > (From update of attachment 222911 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222911&action=review > > > Source/WebCore/platform/LayoutUnit.h:940 > > + return round((value.rawValue() * pixelSnappingFactor) / kEffectiveFixedPointDenominator) / pixelSnappingFactor; > > Needs to be roundf to avoid converting to a double, rounding, and then converting back to a float. > > > Source/WebCore/platform/LayoutUnit.h:945 > > + return floor((value.rawValue() * pixelSnappingFactor) / kEffectiveFixedPointDenominator) / pixelSnappingFactor; > > Needs to be floorf to avoid converting to a double, rounding, and then converting back to a float. > > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:143 > > + ASSERT(abs(baseDeviceMatrix.a) == abs(baseDeviceMatrix.d)); > > The function abs takes an int, and I assume that conversion to integer isn’t what we want here. I suggest using fabs, even though it will result in converting float to double on 32-bit platforms, because this is just an assertion. Thanks. I'll fix them right away.
(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 222911 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=222911&action=review > > > > > Source/WebCore/platform/LayoutUnit.h:940 > > > + return round((value.rawValue() * pixelSnappingFactor) / kEffectiveFixedPointDenominator) / pixelSnappingFactor; > > > > Needs to be roundf to avoid converting to a double, rounding, and then converting back to a float. > > > > > Source/WebCore/platform/LayoutUnit.h:945 > > > + return floor((value.rawValue() * pixelSnappingFactor) / kEffectiveFixedPointDenominator) / pixelSnappingFactor; > > > > Needs to be floorf to avoid converting to a double, rounding, and then converting back to a float. > > > > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:143 > > > + ASSERT(abs(baseDeviceMatrix.a) == abs(baseDeviceMatrix.d)); > > > > The function abs takes an int, and I assume that conversion to integer isn’t what we want here. I suggest using fabs, even though it will result in converting float to double on 32-bit platforms, because this is just an assertion. > > Thanks. I'll fix them right away. bug 128075