Bug 69963 – InjectedBundleHitTestResult::imageRect() should return rect in WKView coordinates

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jeff Miller]

Since WebKit2 clients only know about the WKView coordinate space, this rectangle is only useful if it's in that space.

[Radar WebKit Bug Importer]

<rdar://problem/10275376>

[Jeff Miller]

Created attachment 110747 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 110747 [details]
Patch

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

Is it possible to write a test for this?

> Source/WebKit2/ChangeLog:7
> +        InjectedBundleHitTestResult::imageRect() should return rect in WKView coordinates
> +        https://bugs.webkit.org/show_bug.cgi?id=69963
> +        
> +        Use the confusingly-named ScrollView::contentsToWindow() function to convert the image rect
> +        from frame to WKView coordinates.

Can you give a little justification as to why WKView coordinates are better than whatever coordinates we were previously using?

[Sam Weinig]

In the immortal words of Adam Roben, "this should have an API test."

[Adam Roben (:aroben)]

(In reply to comment #4)
> In the immortal words of Adam Roben, "this should have an API test."

Actually, my words were:

(In reply to comment #3)
> Is it possible to write a test for this?

[Jeff Miller]

Comment on attachment 110747 [details]
Patch

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

>> Source/WebKit2/ChangeLog:7
>> +        from frame to WKView coordinates.
> 
> Can you give a little justification as to why WKView coordinates are better than whatever coordinates we were previously using?

The justification is in the comment in the code, but I'll elaborate in the ChangeLog.

[Simon Fraser (smfr)]

Comment on attachment 110747 [details]
Patch

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

>>> Source/WebKit2/ChangeLog:7
>>> +        from frame to WKView coordinates.
>> 
>> Can you give a little justification as to why WKView coordinates are better than whatever coordinates we were previously using?
> 
> The justification is in the comment in the code, but I'll elaborate in the ChangeLog.

"ToWindow" really is window coords, not view coords. The coords will be flipped in WK1.

[Darin Adler]

Comment on attachment 110747 [details]
Patch

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

>>>> Source/WebKit2/ChangeLog:7
>>>> +        from frame to WKView coordinates.
>>> 
>>> Can you give a little justification as to why WKView coordinates are better than whatever coordinates we were previously using?
>> 
>> The justification is in the comment in the code, but I'll elaborate in the ChangeLog.
> 
> "ToWindow" really is window coords, not view coords. The coords will be flipped in WK1.

This is injected bundle code in WebKit2, so I don’t see how WebKit1 is relevant.

[Jeff Miller]

Comment on attachment 110747 [details]
Patch

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

>>>>> Source/WebKit2/ChangeLog:7
>>>>> +        from frame to WKView coordinates.
>>>> 
>>>> Can you give a little justification as to why WKView coordinates are better than whatever coordinates we were previously using?
>>> 
>>> The justification is in the comment in the code, but I'll elaborate in the ChangeLog.
>> 
>> "ToWindow" really is window coords, not view coords. The coords will be flipped in WK1.
> 
> This is injected bundle code in WebKit2, so I don’t see how WebKit1 is relevant.

WebKit1 isn't relevant, but the first part of Simon's comment is correct.  After talking to Simon more, I'm working on a patch to add member functions to Widget and ScrollView to convert between frame view and root view coordinate systems, since there's no way to do this currently. InjectedBundleHitTestResult::imageRect() can use one of these functions, and I also need to write a WK2 API test (as Sam and Adam pointed out).

[Jeff Miller]

I wrote bug 70136 to cover writing the test for this, since I need to land this patch soon.

[Jeff Miller]

Created attachment 111062 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 111062 [details]
Patch

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

> Source/WebCore/platform/Widget.cpp:106
> +IntRect Widget::convertFromRootView(const IntRect& rootRect) const
> +{
> +    if (const ScrollView* parentScrollView = parent()) {
> +        IntRect parentRect = parentScrollView->convertFromRootView(rootRect);
> +        return convertFromContainingView(parentRect);
> +    }
> +    return rootRect;
> +}
> +
> +IntRect Widget::convertToRootView(const IntRect& localRect) const
> +{
> +    if (const ScrollView* parentScrollView = parent()) {
> +        IntRect parentRect = convertToContainingView(localRect);
> +        return parentScrollView->convertToRootView(parentRect);
> +    }
> +    return localRect;
> +}
> +
> +IntPoint Widget::convertFromRootView(const IntPoint& rootPoint) const
> +{
> +    if (const ScrollView* parentScrollView = parent()) {
> +        IntPoint parentPoint = parentScrollView->convertFromRootView(rootPoint);
> +        return convertFromContainingView(parentPoint);
> +    }
> +    return rootPoint;
> +}
> +
> +IntPoint Widget::convertToRootView(const IntPoint& localPoint) const
> +{
> +    if (const ScrollView* parentScrollView = parent()) {
> +        IntPoint parentPoint = convertToContainingView(localPoint);
> +        return parentScrollView->convertToRootView(parentPoint);
> +    }
> +    return localPoint;
> +}

How about calling these from the existing toWindow/fromWindow calls?

[Jeff Miller]

Comment on attachment 111062 [details]
Patch

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

>> Source/WebCore/platform/Widget.cpp:106
>> +}
> 
> How about calling these from the existing toWindow/fromWindow calls?

Good idea, but I'd rather not perturb existing code in this patch. I will file a followup bug to track this.

[Jeff Miller]

Committed r97514: <http://trac.webkit.org/changeset/97514>

[Jeff Miller]

I wrote bug 70152 to track Simon's suggestion.