[webkit-reviews] review denied: [Bug 179129] Web Automation: inViewCenterPoint should not include topContentInset when computed in viewport coordinates : [Attachment 325595] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Brian Burg
<bburg at apple.com>'s request for review:
Bug 179129: Web Automation: inViewCenterPoint should not include
topContentInset when computed in viewport coordinates
https://bugs.webkit.org/show_bug.cgi?id=179129

Attachment 325595: Patch

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




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

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

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:578
>      if (containerElement) {

The code above uses coreElement->clientRect(). Note the comment above it:

// Note that this is not web-exposed, and does not use the same coordinate
system as getBoundingClientRect() and friends.
IntRect Element::clientRect() const

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:580
>	       inViewCenter =
WebCore::IntPoint(coreFrameView->clientToDocumentPoint(clientCenterPoint.value(
)));

So i think clientToDocumentPoint is the wrong way to convert this point.

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:583
> +		   inViewCenter.value().moveBy(WebCore::IntPoint(0,
-coreFrameView->topContentInset()));

I never want to see this kind of manual coordinate conversion. We should always
go through FrameView helpers like "convert foo to bar".