Bug 113263

Summary: [BlackBerry] WebOverlay::pixelViewportRect() should return pixel viewport coordinates
Product: WebKit Reporter: Arvid Nilsson <anilsson>
Component: WebKit BlackBerryAssignee: Arvid Nilsson <anilsson>
Status: RESOLVED FIXED    
Severity: Normal CC: anilsson, jpetsovits, mifenton, rwlbuis, tonikitoo, webkit.review.bot, yodai
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Arvid Nilsson 2013-03-25 18:45:09 PDT 
But today returns window coordinates, which sometimes happen to coincide with the pixel viewport coordinates, sometimes not.

PR 312404
Comment 1 Arvid Nilsson 2013-03-25 18:54:41 PDT 
Created attachment 194974 [details]
Patch
Comment 2 Mike Fenton 2013-03-26 07:36:38 PDT 
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);
Comment 3 Arvid Nilsson 2013-03-26 07:44:09 PDT 
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.
Comment 4 Arvid Nilsson 2013-03-26 07:50:35 PDT 
Created attachment 195086 [details]
Patch
Comment 5 Mike Fenton 2013-03-26 07:57:20 PDT 
Comment on attachment 195086 [details]
Patch

Looks good to me.
Comment 6 Rob Buis 2013-03-26 08:03:17 PDT 
Comment on attachment 195086 [details]
Patch

LGTM.
Comment 7 Arvid Nilsson 2013-03-26 10:43:12 PDT 
Comment on attachment 195086 [details]
Patch

Thanks Rob and Jakob!
Comment 8 Arvid Nilsson 2013-03-26 10:43:31 PDT 
(In reply to comment #7)
> (From update of attachment 195086 [details])
> Thanks Rob and Jakob!

Err, I mean Rob and Mike.
Comment 9 WebKit Review Bot 2013-03-26 10:46:52 PDT 
Comment on attachment 195086 [details]
Patch

Clearing flags on attachment: 195086

Committed r146906: <http://trac.webkit.org/changeset/146906>
Comment 10 WebKit Review Bot 2013-03-26 10:46:55 PDT 
All reviewed patches have been landed.  Closing bug.