The proposed scrollRecursively API has the following issues. 1.) Dependency on last mouse move position is error prone 2.) The name is confusing 3.) The documentation doesn't say what the return values signifies 4.) The test case doesn't test the negative scrolling case.
Created attachment 50236 [details] Proposed patch. Proposed patch attempting to address some of the issues with the new Qt 4.7 ScrollRecursively API
Comment on attachment 50236 [details] Proposed patch. > +void QWebFrame::scrollFrameWithParent(int dx, int dy, const QPoint& overflowHitTestPos) > +{ > + if (!d->scrollOverflow(dx, dy, overflowHitTestPos)) { I would change the name of the variable in the signature to pos instead of overflowHitTestPos.
Created attachment 50838 [details] Proposed patch This patch would remove the scrollRecursively method from the API and make it an internal method only.
(In reply to comment #3) > Created an attachment (id=50838) [details] > Proposed patch > > This patch would remove the scrollRecursively method from the API and make it > an internal method only. I will post another patch to remove the test case from tst_qwebframe.cpp and the call to sendScrollEvent.
Created attachment 51038 [details] Bug 35873 patch Since there was no response on the mailing list regarding this API it seems like there is no interest or reason to keep it for Qt 4.7. I propose to remove it from the publick API and make it an internal qt_unstable API for now.
Comment on attachment 51038 [details] Bug 35873 patch Clearing flags on attachment: 51038 Committed r56208: <http://trac.webkit.org/changeset/56208>
All reviewed patches have been landed. Closing bug.
Cherry-picked into qtwebkit-4.6 with commit 70ed003c946ad2fe327c08999a72375f2df98ca6 and 8b75e509d033d2df08b0fd0c80a5941cc7083731
Would it make sense to keep the test case for this (internal) api around ?
(In reply to comment #9) > Would it make sense to keep the test case for this (internal) api around ? I would think so
Laszlo, can we close this bug? ScrollRecursively is no longer part of the API. https://bugs.webkit.org/show_bug.cgi?id=36674 addresses some issues for the internal scroll recursively method so any comments can be resolved over there.
(In reply to comment #11) > Laszlo, can we close this bug? ScrollRecursively is no longer part of the API. Well, bug description says the following issues: >The proposed scrollRecursively API has the following issues. >1.) Dependency on last mouse move position is error prone That seems fixed, since POS is passed as parameter. > 2.) The name is confusing > 3.) The documentation doesn't say what the return values signifies bug 40569 seems about these two issues, right? > 4.) The test case doesn't test the negative scrolling case. This should be easily fixable. If you want to close this one, please file follow up bugs for the missing bits, and link them here for the record.
Main point for me are: * is any one working on this? If not, I can ... * why these method bodies are not using EventHandler::scrollRecursively which should work for any overflowed content, incluing inner frames and block elements (e.g. div)?