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.
Created attachment 195418 [details] Patch
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!
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.
Comment on attachment 195418 [details] Patch LGTM.
Comment on attachment 195418 [details] Patch Clearing flags on attachment: 195418 Committed r147744: <http://trac.webkit.org/changeset/147744>
All reviewed patches have been landed. Closing bug.