Bug 48988 - Delegate scrolling via a separate method
Summary: Delegate scrolling via a separate method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-04 04:12 PDT by Kenneth Rohde Christiansen
Modified: 2015-02-13 12:01 PST (History)
6 users (show)

See Also:


Attachments
Patch (11.16 KB, patch)
2010-11-04 04:17 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch2 (11.89 KB, patch)
2010-11-04 06:47 PDT, Kenneth Rohde Christiansen
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2010-11-04 04:12:15 PDT
Instead of piggy-backing on the HostWindow::scroll method, we use a special delegateScrollRequested method. This means that we avoid calling additional scrollbar related code before the call to HostWindow::scroll, and that we do not need to do special casing in ChromeClient::scroll to figure out if it was a delegated scroll or not, thus imposing overhead on the normal case. This will also make the webkit2 impl more clear.
Comment 1 Kenneth Rohde Christiansen 2010-11-04 04:17:15 PDT
Created attachment 72926 [details]
Patch
Comment 2 Early Warning System Bot 2010-11-04 04:28:38 PDT
Attachment 72926 [details] did not build on qt:
Build output: http://queues.webkit.org/results/5099010
Comment 3 Kenneth Rohde Christiansen 2010-11-04 06:47:56 PDT
Created attachment 72933 [details]
Patch2
Comment 4 Adam Roben (:aroben) 2010-11-04 08:27:19 PDT
Comment on attachment 72933 [details]
Patch2

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

> WebCore/platform/ScrollView.cpp:340
> +    IntPoint newScrollPosition = scrollPoint.shrunkTo(maximumScrollPosition());
> +    newScrollPosition.clampNegativeToZero();
> +
> +#if ENABLE(TILED_BACKING_STORE)
>      if (delegatesScrolling()) {
> -        scrollContents(IntSize(scrollPoint.x(), scrollPoint.y()));
> +        hostWindow()->delegatedScrollRequested(IntSize(scrollPoint.x(), scrollPoint.y()));
>          return;
>      }
> -
> -    IntPoint newScrollPosition = scrollPoint.shrunkTo(maximumScrollPosition());
> -    newScrollPosition.clampNegativeToZero();
> +#endif

Looks like you removed the call to scrollContents entirely, even in the non-tiled case.
Comment 5 Kenneth Rohde Christiansen 2010-11-04 08:29:06 PDT
(In reply to comment #4)
> (From update of attachment 72933 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72933&action=review

> Looks like you removed the call to scrollContents entirely, even in the non-tiled case.

No, that is called later indirectly bye setting the scrollbars.
Comment 6 Dave Hyatt 2010-11-04 12:24:39 PDT
Comment on attachment 72933 [details]
Patch2

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

Looks good.

> WebCore/ChangeLog:9
> +        scrolling to the view. This is only used in conjunging with tiling,

"conjunction" is what you meant to say here I think.
Comment 7 Andreas Kling 2010-11-04 12:41:56 PDT
Committed r71352: <http://trac.webkit.org/changeset/71352>