Bug 108386

Summary: [Chromium] WebWidget::selectionBounds should return the bounds in document space
Product: WebKit Reporter: Chris Hopman <cjhopman>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aelias, dglazkov, fishd, jamesr, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Hopman 2013-01-30 14:36:54 PST
[Chromium] WebWidget::selectionBounds should return the bounds in document space
Comment 1 Chris Hopman 2013-01-30 14:43:53 PST
Created attachment 185583 [details]
Patch
Comment 2 WebKit Review Bot 2013-01-30 14:47:20 PST
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.
Comment 3 Adam Barth 2013-01-30 23:50:44 PST
Does this fix a specific problem?
Comment 4 Darin Fisher (:fishd, Google) 2013-01-31 00:41:32 PST
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.
Comment 5 Chris Hopman 2013-01-31 10:51:42 PST
(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).
Comment 6 James Robinson 2013-01-31 10:54:47 PST
+cc aelias for the scaling questions
Comment 7 Chris Hopman 2013-01-31 13:38:09 PST
Created attachment 185849 [details]
Patch
Comment 8 Chris Hopman 2013-01-31 13:39:59 PST
(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?
Comment 9 Alexandre Elias 2013-01-31 14:59:48 PST
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.
Comment 10 WebKit Review Bot 2013-02-01 16:01:05 PST
Comment on attachment 185849 [details]
Patch

Clearing flags on attachment: 185849

Committed r141657: <http://trac.webkit.org/changeset/141657>
Comment 11 WebKit Review Bot 2013-02-01 16:01:11 PST
All reviewed patches have been landed.  Closing bug.