Bug 107831 – [chromium] Make selection handling work in applyPageScaleInCompositor mode

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

[Chris Hopman]

Make selection handling work in applyPageScaleInCompositor mode

[Chris Hopman]

Created attachment 184508 [details]
Patch

[James Robinson]

I'm not terribly familiar with editing code.  What bug does this fix and what tests cover that bug?

[Ryosuke Niwa]

Comment on attachment 184508 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1386
> +    if (frame()->page()->settings()->applyPageScaleFactorInCompositor()) {
> +        unscaledBase.x /= view()->pageScaleFactor();
> +        unscaledBase.y /= view()->pageScaleFactor();
> +        unscaledExtent.x /= view()->pageScaleFactor();
> +        unscaledExtent.y /= view()->pageScaleFactor();
> +    }
> +    VisiblePosition basePosition = visiblePositionForWindowPoint(unscaledBase);
> +    VisiblePosition extentPosition = visiblePositionForWindowPoint(unscaledExtent);

There got to be a better way of achieving this. At minimum, we should have some helper function that automatically scale both x and y.

[Chris Hopman]

Created attachment 185621 [details]
Patch

[Chris Hopman]

Comment on attachment 184508 [details]
Patch

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

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1386
>> +    VisiblePosition extentPosition = visiblePositionForWindowPoint(unscaledExtent);
> 
> There got to be a better way of achieving this. At minimum, we should have some helper function that automatically scale both x and y.

Changed this to convert to IntPoint earlier and can then use IntPoint::scale. I was surprised that WebPoint had no way to scale, but I like this better anyway since I now don't need the conversion when calling windowToContents.

[Chris Hopman]

I changed this to only convert the points passed to ::selectRange and ::moveCaret[...], I'll handle the points sent back in selectionBounds separately. The issue here is that these functions take window points which, when applying the page scale factor in the compositor, need to be unscaled by the page scale factor when being passed in to WebKit.

As mentioned in https://bugs.webkit.org/show_bug.cgi?id=108386 I think we will have to move to having points be passed in to WebKit and returned from WebKit in document space so that we don't have to do this conversion from the two different "window" spaces.

[Ryosuke Niwa]

Comment on attachment 185621 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1386
> +    if (frame()->page()->settings()->applyPageScaleFactorInCompositor()) {
> +        unscaledExtent.scale(1 / view()->pageScaleFactor(), 1 / view()->pageScaleFactor());
> +        unscaledBase.scale(1 / view()->pageScaleFactor(), 1 / view()->pageScaleFactor());
> +    }

I think you should be calling visibleContentScaleFactor() instead since frame's page scaling factor is not necessarily equal to view's page scaling factor. See ScrollView::visibleContentRect for example.

[Alexandre Elias]

I think pageScaleFactor is correct as we're first converting from physical window point to CSS-space window point, and then to the frame's coordinate space.  But please try on a page with a frame to make sure.

[Chris Hopman]

(In reply to comment #8)
> I think pageScaleFactor is correct as we're first converting from physical window point to CSS-space window point, and then to the frame's coordinate space.  But please try on a page with a frame to make sure.

This works correctly within a frame.

[Alexandre Elias]

LGTM.  Adam?

For context, although we no longer this kind of conversion when going from WebCore to Javascript, we now need it for input-eventy things from coming from the browser to WebCore.

[Ryosuke Niwa]

Comment on attachment 185621 [details]
Patch

Okay. We need to come up with a better strategy in the long term though and it sounds like you already know what needs to happen.

[WebKit Review Bot]

Comment on attachment 185621 [details]
Patch

Clearing flags on attachment: 185621

Committed r141470: <http://trac.webkit.org/changeset/141470>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.