Bug 82196 - Revert RenderTheme paint and layout functions to ints
Summary: Revert RenderTheme paint and layout functions to ints
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on:
Blocks: 60318
  Show dependency treegraph
 
Reported: 2012-03-26 06:31 PDT by Levi Weintraub
Modified: 2012-04-03 06:56 PDT (History)
4 users (show)

See Also:


Attachments
Patch (23.23 KB, patch)
2012-03-26 07:02 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (39.08 KB, patch)
2012-04-02 06:54 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch for landing (39.21 KB, patch)
2012-04-03 04:05 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2012-03-26 06:31:53 PDT
While We're converting WebCore layout to sub-pixels, we continue to paint RenderBoxes on pixel boundaries, and when possible we avoid exposing sub-pixel units to platform code. RenderTheme components are platform-specific, and therefor should be isolated from WebCore's sub-pixel units.
Comment 1 Levi Weintraub 2012-03-26 07:02:55 PDT
Created attachment 133798 [details]
Patch
Comment 2 Julien Chaffraix 2012-03-26 13:41:52 PDT
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 3 Eric Seidel (no email) 2012-03-27 12:28:36 PDT
Comment on attachment 133798 [details]
Patch

Looks like you have some unanswered questions from Julien.
Comment 4 Levi Weintraub 2012-03-27 14:01:40 PDT
(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.
Comment 5 Levi Weintraub 2012-04-02 06:54:00 PDT
Created attachment 135088 [details]
Patch
Comment 6 Emil A Eklund 2012-04-02 15:49:11 PDT
The updated patch addresses the previous comments and concerns, please take another look when you get a chance.
Comment 7 Julien Chaffraix 2012-04-02 16:57:08 PDT
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.
Comment 8 Emil A Eklund 2012-04-02 17:00:00 PDT
Thanks Julien!
Comment 9 Levi Weintraub 2012-04-03 03:56:44 PDT
(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!
Comment 10 Levi Weintraub 2012-04-03 04:05:26 PDT
Created attachment 135305 [details]
Patch for landing
Comment 11 WebKit Review Bot 2012-04-03 06:56:11 PDT
Comment on attachment 135305 [details]
Patch for landing

Clearing flags on attachment: 135305

Committed r113030: <http://trac.webkit.org/changeset/113030>
Comment 12 WebKit Review Bot 2012-04-03 06:56:16 PDT
All reviewed patches have been landed.  Closing bug.