Bug 122565 – CoordinatedGraphics: Fix integer rounding when computing pixel alignment

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Correia (qrwteyrutiyoup)]

CoordinatedGraphics: Fix integer rounding when computing pixel alignment

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 213797 [details]
Patch

Proposed patch

[Noam Rosenthal]

Comment on attachment 213797 [details]
Patch

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

> Source/WebCore/ChangeLog:24
> +        In some situations, such as dealing with a mobile browser, which tries to
> +        calculate a scale factor to fit the contents to the viewport, we may get a
> +        small value for either width or height after scaling, which will cause the
> +        resulting FloatRect after being rounded to have this value be zero.
> +
> +        After converting back to the layer coordinates, we will still have this one
> +        value be zero, and that will cause problems in createTilesIfNeeded(), since
> +        in a previous step it will remove the layer's backing store, due to failing
> +        the check in layerShouldHaveBackingStore(), as the layer will be considered
> +        empty. At this point, we hit ASSERT(backingStore) in createTilesIfNeeded().
> +
> +        In this patch we replace the roundedIntRect() call with enclosingIntRect()
> +        in CoordinatedGraphicsLayer::computePixelAlignment().

Too much information here! All you're doing is changing to enclosingIntRect so that very small rectangles (after content scaling) don't appear to be empty, thus creating a crash later.

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #2)
> (From update of attachment 213797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213797&action=review
> 
> > Source/WebCore/ChangeLog:24
> > +        In some situations, such as dealing with a mobile browser, which tries to
> > +        calculate a scale factor to fit the contents to the viewport, we may get a
> > +        small value for either width or height after scaling, which will cause the
> > +        resulting FloatRect after being rounded to have this value be zero.
> > +
> > +        After converting back to the layer coordinates, we will still have this one
> > +        value be zero, and that will cause problems in createTilesIfNeeded(), since
> > +        in a previous step it will remove the layer's backing store, due to failing
> > +        the check in layerShouldHaveBackingStore(), as the layer will be considered
> > +        empty. At this point, we hit ASSERT(backingStore) in createTilesIfNeeded().
> > +
> > +        In this patch we replace the roundedIntRect() call with enclosingIntRect()
> > +        in CoordinatedGraphicsLayer::computePixelAlignment().
> 
> Too much information here! All you're doing is changing to enclosingIntRect so that very small rectangles (after content scaling) don't appear to be empty, thus creating a crash later.

Oops sorry. I was trying to explain the problem I observed, but I will use your words and make it way shorter.

[Noam Rosenthal]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 213797 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=213797&action=review
> > 
> > > Source/WebCore/ChangeLog:24
> > > +        In some situations, such as dealing with a mobile browser, which tries to
> > > +        calculate a scale factor to fit the contents to the viewport, we may get a
> > > +        small value for either width or height after scaling, which will cause the
> > > +        resulting FloatRect after being rounded to have this value be zero.
> > > +
> > > +        After converting back to the layer coordinates, we will still have this one
> > > +        value be zero, and that will cause problems in createTilesIfNeeded(), since
> > > +        in a previous step it will remove the layer's backing store, due to failing
> > > +        the check in layerShouldHaveBackingStore(), as the layer will be considered
> > > +        empty. At this point, we hit ASSERT(backingStore) in createTilesIfNeeded().
> > > +
> > > +        In this patch we replace the roundedIntRect() call with enclosingIntRect()
> > > +        in CoordinatedGraphicsLayer::computePixelAlignment().
> > 
> > Too much information here! All you're doing is changing to enclosingIntRect so that very small rectangles (after content scaling) don't appear to be empty, thus creating a crash later.
> 
> Oops sorry. I was trying to explain the problem I observed, but I will use your words and make it way shorter.

It's ok! Better have too much information than too little :)

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 213800 [details]
v2--comment=More succint ChangeLog.

[WebKit Commit Bot]

Comment on attachment 213800 [details]
v2--comment=More succint ChangeLog.

Clearing flags on attachment: 213800

Committed r157180: <http://trac.webkit.org/changeset/157180>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.