Bug 108386 – [Chromium] WebWidget::selectionBounds should return the bounds in document space

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Hopman]

[Chromium] WebWidget::selectionBounds should return the bounds in document space

[Chris Hopman]

Created attachment 185583 [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]

Does this fix a specific problem?

[Darin Fisher (:fishd, Google)]

Comment on attachment 185583 [details]
Patch

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

> Source/WebKit/chromium/public/WebWidget.h:204
> +    virtual bool documentSelectionBounds(WebRect& anchor, WebRect& focus) const { return false; }

nit: WebWidget as an interface generally isn't supposed to know about DOM-related concepts.
this method name seems to suggest that coordinates are returning in document space instead
of viewport space?  such a method probably belongs on WebView instead.

[Chris Hopman]

(In reply to comment #3)
> Does this fix a specific problem?

So here's the issue: when page scale is applied in the compositor, there are essentially 2 different window spaces, the scaled one and the unscaled one. WebKit deals with the unscaled and then when points or rects are passed between WebKit and the renderer, if they are passed in window space, they need to be scaled or unscaled.

It is unclear (at least to me) whose responsibility this is (i.e. at what boundary should it be done). Currently, it looks like we do this in WebKit (look for applyPageScaleFactorInCompositor in WebFrameImpl.cpp or WebViewImpl.cpp). I don't like this because it becomes confusing when a public API is called within WebKit. For example, WebWidget::selectionBounds() returns two rects. If WebKit applies the pagescale, then when selectionBounds is used in WebKit (for example WebViewImpl::computeSCaleAndScrollForFocusedNode) you have to know that the rects are not actually in WebKit's viewport space, they are in this other scaled space. And so now, we have this layer that needs to know all about an extra space.

My thought was that document space could be this common language that could be used across the boundary into and out of WebKit, so that neither side needed to know about the other's viewport space (and thus never had to distinguish between points in unscaled vs scaled viewport space).

I guess I can just switch to what we've already done in other places and make selectionBounds instead scale these rects when necessary (and its users in WebKit can be updated to know that the rects are scaled).

[James Robinson]

+cc aelias for the scaling questions

[Chris Hopman]

Created attachment 185849 [details]
Patch

[Chris Hopman]

(In reply to comment #7)
> Created an attachment (id=185849) [details]
> Patch

This second patch is following our current approach of having WebKit scale/unscale points when they are returned from/passed to the public API. This works for me and fixes the immediate problem, but I think longer-term we may want to reconsider how we are handling this issue.

What do you think?

[Alexandre Elias]

LGTM.

In the long run the correct solution is to push up all coordinates out of WebKit in absolute coordinates.  But that will be a more involved multi-sided fix and this quick fix doesn't make it harder to do that later.

[WebKit Review Bot]

Comment on attachment 185849 [details]
Patch

Clearing flags on attachment: 185849

Committed r141657: <http://trac.webkit.org/changeset/141657>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.