Bug 99104 – [subpixel] Change intrinsicSize to LayoutUnit

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Emil A Eklund]

Change RenderReplaced and intrinsicSize to LayoutUnit to avoid rounding problems when zooming/scaling.

[Emil A Eklund]

Created attachment 168286 [details]
Patch

[Levi Weintraub]

Comment on attachment 168286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168286&action=review

> Source/WebCore/loader/cache/CachedImage.h:31
> +#include "SVGImageCache.h"
> +

I'm guessing this isn't necessary.

> Source/WebCore/rendering/InlineFlowBox.cpp:631
> +        LayoutUnit baselinePosition = curr->baselinePosition(baselineType);
> +        if (curr->renderer()->isReplaced())
> +            baselinePosition = baselinePosition.floor();

It seems like the changes here and in RenderBox could be landed, along with a lot of rebaselines, first. This could help shrink the size of this patch to a more reasonable size.

> Source/WebCore/rendering/RenderBox.cpp:3321
> -    computedValues.m_position = logicalLeftPos.round();
> +    computedValues.m_position = logicalLeftPos;

I'd love to see the rationale for this in the changelog.

[Emil A Eklund]

Good idea, I've broken out the InlineFlowBox and RenderBox changes into a separate change, see bug 99108.

[Emil A Eklund]

Created attachment 168454 [details]
Patch

[WebKit Review Bot]

Comment on attachment 168454 [details]
Patch

Attachment 168454 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14266407

New failing tests:
fast/reflections/reflection-with-zoom.html

[Emil A Eklund]

Created attachment 168740 [details]
Patch

[Eric Seidel (no email)]

Attachment 168740 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/chromium-linux/compositing/overflow/scroll-ancestor-update-expected.png:0:  Have to enable auto props in the subversion config file (/Users/eseidel/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/Users/eseidel/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 168740 [details]
Patch

Attachment 168740 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14389009

New failing tests:
svg/zoom/page/zoom-svg-through-object-with-percentage-size.xhtml
svg/zoom/page/zoom-svg-through-object-with-absolute-size-2.xhtml
fast/reflections/reflection-with-zoom.html

[WebKit Review Bot]

Comment on attachment 168740 [details]
Patch

Attachment 168740 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14390035

New failing tests:
svg/zoom/page/zoom-svg-through-object-with-percentage-size.xhtml
svg/zoom/page/zoom-svg-through-object-with-absolute-size-2.xhtml
fast/reflections/reflection-with-zoom.html

[Emil A Eklund]

Created attachment 171661 [details]
Patch

[Dominik Röttsches (drott)]

Any chance this fixes bug 67712?

[Emil A Eklund]

(In reply to comment #11)
> Any chance this fixes bug 67712?

Sadly it does not. To fix that bug we'll have to change our image scaling to take the pixel snapped (as opposed to rounded) size into account.

[Emil A Eklund]

Ping?

[Levi Weintraub]

Comment on attachment 171661 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171661&action=review

Overall this looks good, except you use LayoutUnits/Rects/Sizes and FractionalLayoutUnits/Rects/Sizes inconsistently through this patch. Let's keep the "Fractional" part out for consistency and give this another try.

> Source/WebCore/html/ImageDocument.cpp:158
> +        // At a zoom level of 1 the image is guaranteed to have an integer size.

This sounds like an assert to me :)

> Source/WebCore/loader/cache/CachedImage.cpp:268
> +    ASSERT(multiplier != 1.0f || (imageSize.width().fraction() == 0.0f && imageSize.height().fraction() == 0.0f));

Actually, looks like you've got it covered!

> Source/WebCore/platform/graphics/FractionalLayoutSize.h:75
> -    void scale(float scale)
> +    void scale(float widthScale, float heightScale)

Why not just add this two-argument version?

> Source/WebCore/rendering/RenderReplaced.h:49
> -    virtual int minimumReplacedHeight() const { return 0; }
> +    virtual LayoutUnit minimumReplacedHeight() const { return 0; }

LayoutUnit()?

> Source/WebCore/rendering/RenderVideo.cpp:84
> +    float scaleFactor = style()->effectiveZoom();
> +    size.scale(scaleFactor, scaleFactor);

This would be easier with the single-argument version, no?

> Source/WebCore/rendering/RenderVideo.cpp:98
> +FractionalLayoutSize RenderVideo::calculateIntrinsicSize()

Why FrationalLayoutSize instead of LayoutSize?

> Source/WebCore/rendering/style/StyleGeneratedImage.cpp:70
> -    IntSize size = imageSize(renderer, 1);
> +    IntSize size = flooredIntSize(imageSize(renderer, 1));

I think another comment about the flooring is warranted.

[Emil A Eklund]

Created attachment 171832 [details]
Patch

[Emil A Eklund]

(In reply to comment #14)
Made the changes you suggested, PTAL.

[Levi Weintraub]

Comment on attachment 171832 [details]
Patch

Perfect. Thanks!

[WebKit Review Bot]

Comment on attachment 171832 [details]
Patch

Clearing flags on attachment: 171832

Committed r133172: <http://trac.webkit.org/changeset/133172>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Emil A Eklund]

*** Bug 91007 has been marked as a duplicate of this bug. ***