[chromium] Transform mouse/gesture event coordinates to account for pinch-zoom in compositor.
Created attachment 170962 [details] Patch
Patch for discussion. (Does not compile without changes to CC to expose the implTransform.) This patch addresses the coordinate transform issue for mouse and gesture events when a non-identity zoom transform is in effect. I'd like some feedback on the approach please: 1) Would it be better to push the computation of the transformed point back into cc (LaterTreeHost)? Then the Web* interface doesn't need to know about the implTransform at all (it could be called something like "adjustEventLocationForZoom"). 2) I was surprised I couldn't just invert the implTransform without having to modify it first. Since the modification only applies to the (hidden) zoom translation, there wasn't any obvious way around it. Tests to come.
Comment on attachment 170962 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170962&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:667 > + m.setM41(m.m41() / m.m11()); // Translation components seem to have had scale applied to them, Does this mean the implTransform is being computed wrong? > Source/WebKit/chromium/src/WebViewImpl.cpp:670 > + return m.inverse().mapPoint(location); You want projectPoint() when you're using the inverse transform instead of mapPoint()
(In reply to comment #2) > Patch for discussion. (Does not compile without changes to CC to expose the implTransform.) > > This patch addresses the coordinate transform issue for mouse and gesture events when a non-identity zoom transform is in effect. I'd like some feedback on the approach please: > > 1) Would it be better to push the computation of the transformed point back into cc (LaterTreeHost)? Then the Web* interface doesn't need to know about the implTransform at all (it could be called something like "adjustEventLocationForZoom"). Personally, I'd like this more as we have things like MathUtil there, which no longer exist on this side of the webkit boundary. > 2) I was surprised I couldn't just invert the implTransform without having to modify it first. Since the modification only applies to the (hidden) zoom translation, there wasn't any obvious way around it. > > Tests to come.
(In reply to comment #3) > (From update of attachment 170962 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170962&action=review > > > Source/WebKit/chromium/src/WebViewImpl.cpp:667 > > + m.setM41(m.m41() / m.m11()); // Translation components seem to have had scale applied to them, > > Does this mean the implTransform is being computed wrong? Maybe, or it may mean I'm missing something about what implTransform is supposed to do. > > Source/WebKit/chromium/src/WebViewImpl.cpp:670 > > + return m.inverse().mapPoint(location); > > You want projectPoint() when you're using the inverse transform instead of mapPoint() I'll try that and see if it fixes the problem above.
(In reply to comment #4) > (In reply to comment #2) > > Patch for discussion. (Does not compile without changes to CC to expose the implTransform.) > > > > This patch addresses the coordinate transform issue for mouse and gesture events when a non-identity zoom transform is in effect. I'd like some feedback on the approach please: > > > > 1) Would it be better to push the computation of the transformed point back into cc (LaterTreeHost)? Then the Web* interface doesn't need to know about the implTransform at all (it could be called something like "adjustEventLocationForZoom"). > > Personally, I'd like this more as we have things like MathUtil there, which no longer exist on this side of the webkit boundary. I'd vote for the LayerTreeHost approach. Ideally the WebKit/WebCore should know nothing about the page scale. It feels weird to me that the embedder (either the compositor or RenderViewImpl) applies the scale when rendering, while input events are inverted-scaled in WebViewImpl.
(In reply to comment #6) > I'd vote for the LayerTreeHost approach. Ideally the WebKit/WebCore should know nothing about the page scale. It feels weird to me that the embedder (either the compositor or RenderViewImpl) applies the scale when rendering, while input events are inverted-scaled in WebViewImpl. +1 to this
(In reply to comment #6) > (In reply to comment #4) > > (In reply to comment #2) > > > Patch for discussion. (Does not compile without changes to CC to expose the implTransform.) > > > > > > This patch addresses the coordinate transform issue for mouse and gesture events when a non-identity zoom transform is in effect. I'd like some feedback on the approach please: > > > > > > 1) Would it be better to push the computation of the transformed point back into cc (LaterTreeHost)? Then the Web* interface doesn't need to know about the implTransform at all (it could be called something like "adjustEventLocationForZoom"). > > > > Personally, I'd like this more as we have things like MathUtil there, which no longer exist on this side of the webkit boundary. > > I'd vote for the LayerTreeHost approach. Ideally the WebKit/WebCore should know nothing about the page scale. It feels weird to me that the embedder (either the compositor or RenderViewImpl) applies the scale when rendering, while input events are inverted-scaled in WebViewImpl. Agreed!
Created attachment 171272 [details] Patch
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 on attachment 171272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171272&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:2006 > + mouseEvent = *static_cast<const WebMouseEvent*>(&inputEvent); why all the * and & back and forth? Why not just: WebInputEvent inputEventTransformed = inputEvent; mouseEvent = static_cast<WebMouseEvent>(inputEvent); etc?
Comment on attachment 171272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171272&action=review > Source/Platform/chromium/public/WebLayerTreeView.h:36 > +#include "WebTransformationMatrix.h" nit: unused header
(In reply to comment #11) > (From update of attachment 171272 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171272&action=review > > > Source/WebKit/chromium/src/WebViewImpl.cpp:2006 > > + mouseEvent = *static_cast<const WebMouseEvent*>(&inputEvent); > > why all the * and & back and forth? > > Why not just: > WebInputEvent inputEventTransformed = inputEvent; > mouseEvent = static_cast<WebMouseEvent>(inputEvent); > etc? Won't this slice off the extra fields (x, y, globalX, etc.) in the derived classes WebMouseEvent, WebGestureEvent ? Will remove the extra header (oops!).
Comment on attachment 171272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171272&action=review >>> Source/WebKit/chromium/src/WebViewImpl.cpp:2006 >>> + mouseEvent = *static_cast<const WebMouseEvent*>(&inputEvent); >> >> why all the * and & back and forth? >> >> Why not just: >> WebInputEvent inputEventTransformed = inputEvent; >> mouseEvent = static_cast<WebMouseEvent>(inputEvent); >> etc? > > Won't this slice off the extra fields (x, y, globalX, etc.) in the derived classes WebMouseEvent, WebGestureEvent ? > > Will remove the extra header (oops!). Ah I see, okay! I guess this is just awkward since it's being given, and passing on, a reference instead of a pointer.
(In reply to comment #14) > (From update of attachment 171272 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171272&action=review > > >>> Source/WebKit/chromium/src/WebViewImpl.cpp:2006 > >>> + mouseEvent = *static_cast<const WebMouseEvent*>(&inputEvent); > >> > >> why all the * and & back and forth? > >> > >> Why not just: > >> WebInputEvent inputEventTransformed = inputEvent; > >> mouseEvent = static_cast<WebMouseEvent>(inputEvent); > >> etc? > > > > Won't this slice off the extra fields (x, y, globalX, etc.) in the derived classes WebMouseEvent, WebGestureEvent ? > > > > Will remove the extra header (oops!). > > Ah I see, okay! I guess this is just awkward since it's being given, and passing on, a reference instead of a pointer. Yeah, the const-references can be a bit hard if you actually need to transform a value ...
Created attachment 171329 [details] Patch
Comment on attachment 171329 [details] Patch Attachment 171329 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14641301 New failing tests: fast/frames/iframe-window-focus.html fast/frames/frame-programmatic-noresize.html fast/frames/frame-deep-nested-resize.html fast/frames/frame-with-noresize-can-be-resized-after-setting-noResize-to-false.html fast/frames/frame-set-rotation-hit.html fast/frames/frame-dead-region.html fast/frames/frame-inherit-noresize-from-frameset.html fast/frames/hover-timer-crash.html fast/frames/frame-with-noresize-can-be-resized-after-removal-of-noresize.html fast/frames/frames-with-frameborder-zero-can-be-resized.html fast/frames/frame-set-scaling-hit.html
Created attachment 171438 [details] Patch
This patch fixes null pointer dereference on m_layerTreeView.
Comment on attachment 171438 [details] Patch R=me
Comment on attachment 171438 [details] Patch Clearing flags on attachment: 171438 Committed r132946: <http://trac.webkit.org/changeset/132946>
All reviewed patches have been landed. Closing bug.