Summary: | [BlackBerry] Replace map{From,To}Transformed() with ViewportAccessor | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jakob Petsovits <jpetsovits> | ||||
Component: | WebKit BlackBerry | Assignee: | Jakob Petsovits <jpetsovits> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | allan.jensen, anilsson, commit-queue, mifenton, rwlbuis, tonikitoo, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Jakob Petsovits
2013-03-27 15:34:25 PDT
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. |