Bug 87774 - REGRESSION (r105515): reflection masks are truncated at zoom levels < 1
Summary: REGRESSION (r105515): reflection masks are truncated at zoom levels < 1
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: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-05-29 13:40 PDT by Simon Fraser (smfr)
Modified: 2012-06-02 13:43 PDT (History)
7 users (show)

See Also:


Attachments
Testcase (353 bytes, text/html)
2012-05-29 13:41 PDT, Simon Fraser (smfr)
no flags Details
Patch (310.58 KB, patch)
2012-06-01 12:47 PDT, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2012-05-29 13:40:41 PDT
r105515 broke reflections when using text zoom to zoom out to a level < 1. The reflection mask is truncated.
Comment 1 Simon Fraser (smfr) 2012-05-29 13:41:05 PDT
Created attachment 144612 [details]
Testcase
Comment 2 Simon Fraser (smfr) 2012-05-29 13:42:48 PDT
<rdar://problem/11387506>
Comment 3 Simon Fraser (smfr) 2012-05-31 15:38:23 PDT
Niko: ping
Comment 4 Beth Dakin 2012-05-31 15:44:26 PDT
It seems like RenderBoxModelObject::calculateImageIntrinsicDimensions() does not always scale by the effectiveZoom so it doesn't make sense to always divide the zoom out. That's what happening in this test case. 

That is what is happening for the attached reduction. We return right away without scaling the dimensions:

IntSize RenderBoxModelObject::calculateImageIntrinsicDimensions(StyleImage* image, const IntSize& positioningAreaSize) const
{
    // A generated image without a fixed size, will always return the container size as intrinsic size.
    if (image->isGeneratedImage() && image->usesImageContainerSize())
        return IntSize(positioningAreaSize.width(), positioningAreaSize.height());

…

But there are other cases that don't scale too.
Comment 5 Beth Dakin 2012-06-01 12:47:53 PDT
Created attachment 145361 [details]
Patch

Here's one way to fix this.
Comment 6 Tim Horton 2012-06-01 12:52:56 PDT
Comment on attachment 145361 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        * rendering/RenderBoxModelObject.cpp:

Additional newline here.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1034
> +    IntSize imageIntrinsicSize = calculateImageIntrinsicDimensions(image, positioningAreaSize, true);

Maybe an enum instead of an incomprehensible bool argument?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1251
> -    int imageWidth = imageSize.width() / style->effectiveZoom();
> -    int imageHeight = imageSize.height() / style->effectiveZoom();
> +    int imageWidth = imageSize.width();
> +    int imageHeight = imageSize.height();

Have you quadruple checked that this doesn't break the apple.com top bar again (a la https://bugs.webkit.org/show_bug.cgi?id=76249)?

> LayoutTests/ChangeLog:5
> +        -and corresponding-

I don't know that the -and corresponding- is necessary, but OK :D
Comment 7 Simon Fraser (smfr) 2012-06-01 12:53:26 PDT
Comment on attachment 145361 [details]
Patch

r+ but I agree with Tim!
Comment 8 Beth Dakin 2012-06-01 12:57:48 PDT
(In reply to comment #6)
> (From update of attachment 145361 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145361&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        * rendering/RenderBoxModelObject.cpp:
> 
> Additional newline here.
> 

Added.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1034
> > +    IntSize imageIntrinsicSize = calculateImageIntrinsicDimensions(image, positioningAreaSize, true);
> 
> Maybe an enum instead of an incomprehensible bool argument?
> 

I knew someone was going to suggest this. And I only didn't do it that way from the start because I suck so much at naming things. How about:

enum ShouldScaleByEffectiveZoom { ShouldScale, ShouldNotScale }

Thoughts? Suggestions?

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1251
> > -    int imageWidth = imageSize.width() / style->effectiveZoom();
> > -    int imageHeight = imageSize.height() / style->effectiveZoom();
> > +    int imageWidth = imageSize.width();
> > +    int imageHeight = imageSize.height();
> 
> Have you quadruple checked that this doesn't break the apple.com top bar again (a la https://bugs.webkit.org/show_bug.cgi?id=76249)?
> 

I have! I ran lots of layout tests and pixel tests too.

> > LayoutTests/ChangeLog:5
> > +        -and corresponding-
> 
> I don't know that the -and corresponding- is necessary, but OK :D

It's my thang! Just ask mitzpettel. :-P
Comment 9 Beth Dakin 2012-06-01 13:21:07 PDT
Fixed: http://trac.webkit.org/changeset/119274
Comment 10 Nikolas Zimmermann 2012-06-02 13:40:47 PDT
(In reply to comment #3)
> Niko: ping
Sorry for the late answer - I was busy with internal work this week.
Beth, thanks a bunch for looking into this!
Comment 11 Beth Dakin 2012-06-02 13:43:38 PDT
(In reply to comment #10)
> (In reply to comment #3)
> > Niko: ping
> Sorry for the late answer - I was busy with internal work this week.
> Beth, thanks a bunch for looking into this!

No worries, I understand. 

And, you're welcome! :-)