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.
Created attachment 85167 [details] Patch
Watcha think?
Comment on attachment 85167 [details] Patch It is an offset, IntSize is normally used for that. We are not scrolling to a point.
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?
Attachment 85167 [details] did not build on qt: Build output: http://queues.webkit.org/results/8116478
(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.
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.
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
(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 =).
> > 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.
r+ given this compiles on Qt that is :-)
(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.
You need to modify the WebPageProxy.messages.in file, or what it is called.
(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!
Created attachment 85233 [details] Patch
Comment on attachment 85233 [details] Patch Clearing flags on attachment: 85233 Committed r80716: <http://trac.webkit.org/changeset/80716>
All reviewed patches have been landed. Closing bug.