Summary: | [chromium] Add WebFrame::visibleContentRect() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tien-Ren Chen <trchen> | ||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, jamesr, tkent+wkapi, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Tien-Ren Chen
2013-01-30 01:38:07 PST
Created attachment 185435 [details]
Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. @jamesr: Would you be willing to review this patch? Comment on attachment 185435 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185435&action=review > Source/WebKit/chromium/public/WebFrame.h:172 > + // Returns the visible content rect (minus scrollbars, in absolute coordinate) blank line before new functions how does this interact with contentsSize() + scrollOffset() ? > Source/WebKit/chromium/src/WebViewImpl.cpp:4378 > + physicalWindowRect.scale(deviceScaleFactor() * pageScaleFactor()); I don't understand this function. device and page scale factors are exposed through the WebKit API, why do we need another WebKit API entry point just to multiply them together? (In reply to comment #4) > (From update of attachment 185435 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185435&action=review > > > Source/WebKit/chromium/public/WebFrame.h:172 > > + // Returns the visible content rect (minus scrollbars, in absolute coordinate) > > blank line before new functions > > how does this interact with contentsSize() + scrollOffset() ? Nothing to do with contentsSize. It is the current visible area of the frame, in absolute coordinate. In current implementation it would be the same value as IntRect(scrollOffset, ceil(webView()->size() / pageScaleFactor)) > > Source/WebKit/chromium/src/WebViewImpl.cpp:4378 > > + physicalWindowRect.scale(deviceScaleFactor() * pageScaleFactor()); > > I don't understand this function. device and page scale factors are exposed through the WebKit API, why do we need another WebKit API entry point just to multiply them together? Because it is much less error-prone and easier to understand this way. Saying "foo = bar * deviceScaleFactor * pageScaleFactor;" can be understood by the compiler, but doesn't really translate well to English. While saying "foo = WebViewImpl->physicalWindowFromClient(bar);" even your grandmother can understand what are you trying to do here. Consider the more complex case, what if we need to convert between absolute and client coordinates? Inner and outer viewport coordinates? Should we add or substract the offset? Translate before scale or after? Doing this all the times kills lots of brain cells. Created attachment 185607 [details]
Patch
Per offline discussion, we decided not to over-grow WebView API with bunch of conversion functions, but there is probably no alternative for WebFrame::visibleContentRect(). Comment on attachment 185607 [details] Patch Clearing flags on attachment: 185607 Committed r141352: <http://trac.webkit.org/changeset/141352> All reviewed patches have been landed. Closing bug. |