Bug 35873 – [Qt] New API scrollRecursively has several problems.

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Joseph Ligman]

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.

[Joseph Ligman]

Created attachment 50236 [details]
Proposed patch.

Proposed patch attempting to address some of the issues with the new Qt 4.7 ScrollRecursively API

[Laszlo Gombos]

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.

[Joseph Ligman]

Created attachment 50838 [details]
Proposed patch

This patch would remove the scrollRecursively method from the API and make it an internal method only.

[Joseph Ligman]

(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.

[Joseph Ligman]

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.

[WebKit Commit Bot]

Comment on attachment 51038 [details]
Bug 35873 patch

Clearing flags on attachment: 51038

Committed r56208: <http://trac.webkit.org/changeset/56208>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Cherry-picked into qtwebkit-4.6 with commit 70ed003c946ad2fe327c08999a72375f2df98ca6 and 8b75e509d033d2df08b0fd0c80a5941cc7083731

[Laszlo Gombos]

Would it make sense to keep the test case for this (internal) api around ?

[Kenneth Rohde Christiansen]

(In reply to comment #9)
> Would it make sense to keep the test case for this (internal) api around ?

I would think so

[Joseph Ligman]

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.

[Antonio Gomes]

(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.

[Antonio Gomes]

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)?