Bug 113263 – [BlackBerry] WebOverlay::pixelViewportRect() should return pixel viewport coordinates

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Arvid Nilsson]

But today returns window coordinates, which sometimes happen to coincide with the pixel viewport coordinates, sometimes not.

PR 312404

[Arvid Nilsson]

Created attachment 194974 [details]
Patch

[Mike Fenton]

Comment on attachment 194974 [details]
Patch

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

> Source/WebCore/platform/graphics/blackberry/LayerRenderer.cpp:573
> +    FloatRect result = toPixelCoordinates(rect, m_viewport, surfaceHeight);

A more meaningful variable than result could be helpful here.  pixelRect?

As well, if the delta calculation was put ahead the calculation, adjustment and return would all be nicely grouped.

> Source/WebCore/platform/graphics/blackberry/LayerRenderer.cpp:583
> +    FloatRect result = toPixelCoordinates(rect, m_viewport, surfaceHeight);

See other comment.

Could this function be
FloatRect pixelRect(toPixelViewCoordinates(rect);
pixelRect.scale(1 / m_scale);
return enclosingIntRect(pixelRect);

[Arvid Nilsson]

Comment on attachment 194974 [details]
Patch

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

>> Source/WebCore/platform/graphics/blackberry/LayerRenderer.cpp:573
>> +    FloatRect result = toPixelCoordinates(rect, m_viewport, surfaceHeight);
> 
> A more meaningful variable than result could be helpful here.  pixelRect?
> 
> As well, if the delta calculation was put ahead the calculation, adjustment and return would all be nicely grouped.

Good point

>> Source/WebCore/platform/graphics/blackberry/LayerRenderer.cpp:583
>> +    FloatRect result = toPixelCoordinates(rect, m_viewport, surfaceHeight);
> 
> See other comment.
> 
> Could this function be
> FloatRect pixelRect(toPixelViewCoordinates(rect);
> pixelRect.scale(1 / m_scale);
> return enclosingIntRect(pixelRect);

The reason I opted for some code duplication here was to get floating point precision, unfortunately toPixelViewportCoordinates returns an IntPoint. However, the rounding should be harmless, I'll go for your suggestion instead to keep it DRY.

[Arvid Nilsson]

Created attachment 195086 [details]
Patch

[Mike Fenton]

Comment on attachment 195086 [details]
Patch

Looks good to me.

[Rob Buis]

Comment on attachment 195086 [details]
Patch

LGTM.

[Arvid Nilsson]

Comment on attachment 195086 [details]
Patch

Thanks Rob and Jakob!

[Arvid Nilsson]

(In reply to comment #7)
> (From update of attachment 195086 [details])
> Thanks Rob and Jakob!

Err, I mean Rob and Mike.

[WebKit Review Bot]

Comment on attachment 195086 [details]
Patch

Clearing flags on attachment: 195086

Committed r146906: <http://trac.webkit.org/changeset/146906>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.