[webkit-reviews] review denied: [Bug 172019] elementFromPoint() should consider x and y to be in client (layout viewport) coordinates : [Attachment 314585] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Ali Juma
<ajuma at chromium.org>'s request for review:
Bug 172019: elementFromPoint() should consider x and y to be in client (layout
viewport) coordinates
https://bugs.webkit.org/show_bug.cgi?id=172019

Attachment 314585: Patch

https://bugs.webkit.org/attachment.cgi?id=314585&action=review




--- Comment #6 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 314585
  --> https://bugs.webkit.org/attachment.cgi?id=314585
Patch

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

> Source/WebCore/dom/TreeScope.cpp:307
> +	   documentScope().updateLayout();

Why is this documentScope().updateLayout(); only in the visual viewport code
path?

> Source/WebCore/dom/TreeScope.cpp:308
> +	   FloatPoint layoutViewportPoint =
view->clientToLayoutViewportPoint(clientPoint);

clientPoint should already be in layout viewport coordinates from the caller.

> Source/WebCore/page/FrameView.h:480
> +    FloatRect layoutViewportToAbsoluteRect(FloatRect) const;
> +    FloatPoint layoutViewportToAbsolutePoint(FloatPoint) const;
> +
> +    FloatPoint clientToLayoutViewportPoint(FloatPoint) const;

I don't think we should introduce these new conversion functions.
"layoutViewport" coordinates should just be client coordinates when visual
viewports are enabled, and callers can already convert to them via
documentToClientRect etc.

> Source/WebCore/rendering/RenderLayer.cpp:4934
> +	       hitTestArea.intersect(absoluteLayoutViewportRect);
> +	   } else
> +	      
hitTestArea.intersect(renderer().view().frameView().visibleContentRect(LegacyIO
SDocumentVisibleRect));

Neither of these make much sense for the case when visualOverflowRect() was
called; that's not in absolute coordinates.