Bug 113449 – [BlackBerry] Replace map{From,To}Transformed() with ViewportAccessor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Jakob Petsovits]

The patch below removes the WebPage/BackingStoreClient family of coordinate transformation functions, which were widely disliked for their naming, with ViewportAccessor API that had previously been introduced for limited use cases. This takes care of the remaining call sites and leaves us with a single API to use.

[Jakob Petsovits]

Created attachment 195418 [details]
Patch

[Arvid Nilsson]

Comment on attachment 195418 [details]
Patch

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

I really like this patch, great cleanup!

> Source/WebKit/blackberry/Api/BackingStore.cpp:1246
> +            : viewportAccessor->roundFromDocumentContents(viewportAccessor->documentContentsRect(), currentScale);

This looks like it's actually fixing a bug - before, transformedContentsSize() would be called, which always uses the saved scale. But the code is now updated to use the currentScale stored in the active geometry, which could be different.

> Source/WebKit/blackberry/Api/WebPage.cpp:2895
> +

What about IntPoint ViewportAccessor::roundedPixelContents(const FloatPoint& point) const, which does exactly this?

> Source/WebKit/blackberry/Api/WebPage.cpp:3897
> +        IntPoint documentContentsPoint = m_webkitThreadViewportAccessor->documentContentsFromViewport(mouseEvent.position());

Just a quick observation - I really like how the viewport accessor uses the "resultFromInput" naming convention which make the code more readable instead of "inputToResult" which is kind of backwards.

> Source/WebKit/blackberry/Api/WebPage.cpp:4472
> +            const IntRect tRect = viewportAccessor->roundToDocumentFromPixelContents(WebCore::FloatRect(blockRect));

Whoa, I thought we didn't use "const" for locals in the WebKit code.

> Source/WebKit/blackberry/Api/WebPage.cpp:4522
> +    const FloatPoint topLeftPoint = newBlockRect.location();

Whoa, there's that const again!

[Jakob Petsovits]

Comment on attachment 195418 [details]
Patch

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

Thanks for the review, Arvid!

>> Source/WebKit/blackberry/Api/WebPage.cpp:2895
>> +
> 
> What about IntPoint ViewportAccessor::roundedPixelContents(const FloatPoint& point) const, which does exactly this?

I was thinking about that, but the thing with roundedPixelContents() is really that we shouldn't apply it to a document coordinate offset. roundedDocumentContents() goes the other way and I didn't want to change the behavior in subtle ways (for the record, I don't think the rounding direction matters in this case) so I put the rounding in here directly to preserve the previous calculation without applying a "wrong" function to the point.

[Rob Buis]

Comment on attachment 195418 [details]
Patch

LGTM.

[WebKit Commit Bot]

Comment on attachment 195418 [details]
Patch

Clearing flags on attachment: 195418

Committed r147744: <http://trac.webkit.org/changeset/147744>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.