Bug 56011 – Tiled backing store's delegated scroll request uses incorrect convention

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Gustavo Noronha (kov)]

While poking at the tiled backing store code I noticed that delegated scroll requests use a delta, but the only place that initiates these requests receives an absolute-relative-to-content point.

[Gustavo Noronha (kov)]

Created attachment 85167 [details]
Patch

[Gustavo Noronha (kov)]

Watcha think?

[Kenneth Rohde Christiansen]

Comment on attachment 85167 [details]
Patch

It is an offset, IntSize is normally used for that. We are not scrolling to a point.

[Kenneth Rohde Christiansen]

Comment on attachment 85167 [details]
Patch

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

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:435
> +    QPoint currentPosition(m_webPage->mainFrame()->scrollPosition());

Where was this done before?

[Early Warning System Bot]

Attachment 85167 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8116478

[Gustavo Noronha (kov)]

(In reply to comment #3)
> (From update of attachment 85167 [details])
> It is an offset, IntSize is normally used for that. We are not scrolling to a point.

If that's the case, should ScrollView::setScrollPoint be renamed/changed, or should we convert its point to an offset before calling the delegatedScrollRequested method?

(In reply to comment #4)
> (From update of attachment 85167 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85167&action=review
> 
> > Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:435
> > +    QPoint currentPosition(m_webPage->mainFrame()->scrollPosition());
> 
> Where was this done before?

I'm not sure I understand the question, sorry =(. The conversion of the point received by setScrollPoint into an offset is not done anywhere that I could find right now.

[Kenneth Rohde Christiansen]

I think that you are right, it is a scrollToPosition(). I just checked our code.

But then I do not understand why you have to make that change to the Qt code.

[Kenneth Rohde Christiansen]

Comment on attachment 85167 [details]
Patch

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

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:436
> -    emit m_webPage->scrollRequested(delta.width(), delta.height(), QRect(QPoint(0, 0), m_webPage->viewportSize()));
> +    QPoint currentPosition(m_webPage->mainFrame()->scrollPosition());
> +    emit m_webPage->scrollRequested(point.x() - currentPosition.x(), point.y() - currentPosition.y(), QRect(QPoint(0, 0), m_webPage->viewportSize()));

This change

[Gustavo Noronha (kov)]

(In reply to comment #8)
> (From update of attachment 85167 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85167&action=review
> 
> > Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:436
> > -    emit m_webPage->scrollRequested(delta.width(), delta.height(), QRect(QPoint(0, 0), m_webPage->viewportSize()));
> > +    QPoint currentPosition(m_webPage->mainFrame()->scrollPosition());
> > +    emit m_webPage->scrollRequested(point.x() - currentPosition.x(), point.y() - currentPosition.y(), QRect(QPoint(0, 0), m_webPage->viewportSize()));
> 
> This change

Because of this:

http://trac.webkit.org/browser/trunk/Source/WebKit/qt/Api/qwebpage.cpp
4099	    \fn void QWebPage::scrollRequested(int dx, int dy, const QRect& rectToScroll)
4100	
4101	    This signal is emitted whenever the content given by \a rectToScroll needs
4102	    to be scrolled \a dx and \a dy downwards and no view was set.

The docs imply to me that QWebPage::scrollRequested wants a delta, so I convert the point to the delta to match the expectations. I'm not sure about how the API is used today so perhaps it's the docs that need to be changed to say it's given a point =).

[Kenneth Rohde Christiansen]

> 
> Because of this:
> 
> http://trac.webkit.org/browser/trunk/Source/WebKit/qt/Api/qwebpage.cpp
> 4099        \fn void QWebPage::scrollRequested(int dx, int dy, const QRect& rectToScroll)
> 4100    
> 4101        This signal is emitted whenever the content given by \a rectToScroll needs
> 4102        to be scrolled \a dx and \a dy downwards and no view was set.
> 
> The docs imply to me that QWebPage::scrollRequested wants a delta, so I convert the point to the delta to match the expectations. I'm not sure about how the API is used today so perhaps it's the docs that need to be changed to say it's given a point =).

Ah this is for the WebKit1 port. Then that seems fine. I think we should rename this method for WebKit2 to scrollPositionRequested and use absolute values, otherwise we will have to convert twice for no sake.

[Kenneth Rohde Christiansen]

r+ given this compiles on Qt that is :-)

[Gustavo Noronha (kov)]

(In reply to comment #11)
> r+ given this compiles on Qt that is :-)

Thanks =) about that... 

../../../Source/WebKit2/WebProcess/WebPage/WebPage.cpp:794: error: no matching function for call to ‘Messages::WebPageProxy::PageDidRequestScroll::PageDidRequestScroll(const WebCore::IntPoint&)’
generated/WebPageProxyMessages.h:532: note: candidates are: Messages::WebPageProxy::PageDidRequestScroll::PageDidRequestScroll(const WebCore::IntSize&)

That seems to be caused by a file that should have been regenerated not being regenerated =( any clues on how to fix that? I tried a local build of qtwebkit but it failed for a different, unrelated reason, still looking at it.

[Kenneth Rohde Christiansen]

You need to modify the WebPageProxy.messages.in file, or what it is called.

[Gustavo Noronha (kov)]

(In reply to comment #13)
> You need to modify the WebPageProxy.messages.in file, or what it is called.

Awesome, thanks =) I'll upload the new patch to get the ews to test it again, but it built fine locally!

[Gustavo Noronha (kov)]

Created attachment 85233 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 85233 [details]
Patch

Clearing flags on attachment: 85233

Committed r80716: <http://trac.webkit.org/changeset/80716>

[Gustavo Noronha (kov)]

All reviewed patches have been landed.  Closing bug.