Bug 108311 – [chromium] Add WebFrame::visibleContentRect()

• Component: New Bugs
• Status: RESOLVED FIXED

[Tien-Ren Chen]

[chromium] Add convenience functions for geometry query

[Tien-Ren Chen]

Created attachment 185435 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

@jamesr: Would you be willing to review this patch?

[James Robinson]

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?

[Tien-Ren Chen]

(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.

[Tien-Ren Chen]

Created attachment 185607 [details]
Patch

[Tien-Ren Chen]

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().

[WebKit Review Bot]

Comment on attachment 185607 [details]
Patch

Clearing flags on attachment: 185607

Committed r141352: <http://trac.webkit.org/changeset/141352>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.