Bug 82196

Created attachment 133798 [details] Patch

Comment on attachment 133798 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133798&action=review > Source/WebCore/ChangeLog:11 > + before passing coordinates to the external code. RenderTheme encompasses a set of objects whose > + rendering is influenced by the platform. This change reverts the interface between this platform > + code and WebCore to be integers. It looks like some platforms are not using int for native geometries (Mac for example uses float in NSSize) which makes me wonder if it's not desirable to keep the sub-pixel precision here. Your take looks like it's to just use device pixels (thus losing some precision), is it really ridiculous to keep the extended precision here? (I don't see a technical limitation here as FractionalUnit will be in platform/ so it's not a layering violation AFAICT) > Source/WebCore/ChangeLog:22 > + (WebCore::IntRect::pixelSnappedLocation): Temporary mirrors to the functions of the same name on > + FractionalLayoutRect. > + (WebCore::IntRect::pixelSnappedSize): Ditto. Let's mark those 2 as temporary (ie don't use and please remove when done with the transition). > Source/WebCore/ChangeLog:55 > + * rendering/RenderThemeMac.mm: > + (WebCore::RenderThemeMac::adjustRepaintRect): > + (WebCore::RenderThemeMac::inflateRect): > + (WebCore::RenderThemeMac::setControlSize): > + (WebCore::RenderThemeMac::paintCapsLockIndicator): > + (WebCore::RenderThemeMac::paintMenuList): > + (WebCore::RenderThemeMac::meterSizeForBounds): > + (WebCore::RenderThemeMac::setPopupButtonCellState): > + (WebCore::RenderThemeMac::paintSearchFieldCancelButton): > + (WebCore::RenderThemeMac::volumeSliderOffsetFromMuteButton): How about the other RenderThemes? > Source/WebCore/rendering/RenderThemeMac.h:48 > + virtual void adjustRepaintRect(const RenderObject*, IntRect&); Let's annotate those functions with OVERRIDE as we go (not repeated in the rest of RenderThemeMac).

Comment on attachment 133798 [details] Patch Looks like you have some unanswered questions from Julien.

(In reply to comment #3) > (From update of attachment 133798 [details]) > Looks like you have some unanswered questions from Julien. Totally accurate. I'm working on the best solution here.

Created attachment 135088 [details] Patch

The updated patch addresses the previous comments and concerns, please take another look when you get a chance.

Comment on attachment 135088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135088&action=review > Source/WebCore/ChangeLog:15 > + Some platforms, such as Mac, use sub-pixel units for layout and rendering, but it's still not > + desirable to pass sub-pixel values to these API's, because ultimately we'll render these objects > + at whole-pixel values to avoid anti-aliasing. I like your explanation, thanks for digging this. > Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:292 > + IntRect cancelButtonRect(cancelButtonObject->offsetFromAncestorContainer(inputRenderBox).width(), > + inputContentBox.y() + (inputContentBox.height() - cancelButtonSize + 1) / 2, > + cancelButtonSize, cancelButtonSize); I prefer when the arguments on the non-first row are aligned with the argument on the line above but our style guide is silent on that so it's up to you. > Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:335 > + IntRect magnifierRect(magnifierObject->offsetFromAncestorContainer(inputRenderBox).width(), > + inputContentBox.y() + (inputContentBox.height() - magnifierSize + 1) / 2, > + magnifierSize, magnifierSize); Ditto. > Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:368 > + IntRect magnifierRect(magnifierObject->offsetFromAncestorContainer(inputRenderBox).width(), > + inputContentBox.y() + (inputContentBox.height() - magnifierHeight + 1) / 2, > + magnifierWidth, magnifierHeight); Ditto.

Thanks Julien!

(In reply to comment #7) > (From update of attachment 135088 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135088&action=review > > > Source/WebCore/ChangeLog:15 > > + Some platforms, such as Mac, use sub-pixel units for layout and rendering, but it's still not > > + desirable to pass sub-pixel values to these API's, because ultimately we'll render these objects > > + at whole-pixel values to avoid anti-aliasing. > > I like your explanation, thanks for digging this. Thank you kindly :) > > > Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:292 > > + IntRect cancelButtonRect(cancelButtonObject->offsetFromAncestorContainer(inputRenderBox).width(), > > + inputContentBox.y() + (inputContentBox.height() - cancelButtonSize + 1) / 2, > > + cancelButtonSize, cancelButtonSize); > > I prefer when the arguments on the non-first row are aligned with the argument on the line above but our style guide is silent on that so it's up to you. I actually agree, but have been pressed for the former style in the past. I'll update the style and land. > > > Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:335 > > + IntRect magnifierRect(magnifierObject->offsetFromAncestorContainer(inputRenderBox).width(), > > + inputContentBox.y() + (inputContentBox.height() - magnifierSize + 1) / 2, > > + magnifierSize, magnifierSize); > > Ditto. > > > Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:368 > > + IntRect magnifierRect(magnifierObject->offsetFromAncestorContainer(inputRenderBox).width(), > > + inputContentBox.y() + (inputContentBox.height() - magnifierHeight + 1) / 2, > > + magnifierWidth, magnifierHeight); > > Ditto. Thanks Julien!

Created attachment 135305 [details] Patch for landing

Comment on attachment 135305 [details] Patch for landing Clearing flags on attachment: 135305 Committed r113030: <http://trac.webkit.org/changeset/113030>

All reviewed patches have been landed. Closing bug.