Bug 35873 - [Qt] New API scrollRecursively has several problems.
Summary: [Qt] New API scrollRecursively has several problems.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Joseph Ligman
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-03-08 10:52 PST by Joseph Ligman
Modified: 2010-11-18 09:17 PST (History)
8 users (show)

See Also:


Attachments
Proposed patch. (9.45 KB, patch)
2010-03-08 11:17 PST, Joseph Ligman
no flags Details | Formatted Diff | Diff
Proposed patch (11.30 KB, patch)
2010-03-16 14:23 PDT, Joseph Ligman
no flags Details | Formatted Diff | Diff
Bug 35873 patch (11.70 KB, patch)
2010-03-18 09:49 PDT, Joseph Ligman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Ligman 2010-03-08 10:52:18 PST
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.
Comment 1 Joseph Ligman 2010-03-08 11:17:31 PST
Created attachment 50236 [details]
Proposed patch.

Proposed patch attempting to address some of the issues with the new Qt 4.7 ScrollRecursively API
Comment 2 Laszlo Gombos 2010-03-15 10:28:24 PDT
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.
Comment 3 Joseph Ligman 2010-03-16 14:23:59 PDT
Created attachment 50838 [details]
Proposed patch

This patch would remove the scrollRecursively method from the API and make it an internal method only.
Comment 4 Joseph Ligman 2010-03-16 14:48:51 PDT
(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.
Comment 5 Joseph Ligman 2010-03-18 09:49:48 PDT
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 6 WebKit Commit Bot 2010-03-18 21:51:40 PDT
Comment on attachment 51038 [details]
Bug 35873 patch

Clearing flags on attachment: 51038

Committed r56208: <http://trac.webkit.org/changeset/56208>
Comment 7 WebKit Commit Bot 2010-03-18 21:51:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Simon Hausmann 2010-03-25 08:49:34 PDT
Cherry-picked into qtwebkit-4.6 with commit 70ed003c946ad2fe327c08999a72375f2df98ca6 and 8b75e509d033d2df08b0fd0c80a5941cc7083731
Comment 9 Laszlo Gombos 2010-03-30 09:12:39 PDT
Would it make sense to keep the test case for this (internal) api around ?
Comment 10 Kenneth Rohde Christiansen 2010-03-30 10:24:54 PDT
(In reply to comment #9)
> Would it make sense to keep the test case for this (internal) api around ?

I would think so
Comment 11 Joseph Ligman 2010-04-09 11:08:36 PDT
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.
Comment 12 Antonio Gomes 2010-07-25 21:37:09 PDT
(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.
Comment 13 Antonio Gomes 2010-07-25 21:41:48 PDT
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)?