Make selection handling work in applyPageScaleInCompositor mode
Created attachment 184508 [details] Patch
I'm not terribly familiar with editing code. What bug does this fix and what tests cover that bug?
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.
Created attachment 185621 [details] Patch
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.
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.
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.
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.
(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.
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.
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.
Comment on attachment 185621 [details] Patch Clearing flags on attachment: 185621 Committed r141470: <http://trac.webkit.org/changeset/141470>
All reviewed patches have been landed. Closing bug.