Bug 72059 - [chromium] scrolling is broken with compositor thread
Summary: [chromium] scrolling is broken with compositor thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hin-Chung Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-10 14:22 PST by Hin-Chung Lam
Modified: 2011-11-11 04:16 PST (History)
4 users (show)

See Also:


Attachments
Patch (7.36 KB, patch)
2011-11-10 14:44 PST, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (2.90 KB, patch)
2011-11-10 15:01 PST, Hin-Chung Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hin-Chung Lam 2011-11-10 14:22:26 PST
Scrolling is currently broken because of this http://trac.webkit.org/changeset/99844

The above patch tries to subtract scroll delta after commit has completed on the main thread. However the subtraction happens on a wrong layer. It subtracts scroll delta from the root layer but instead scroll delta is coming from the scroll layer.
Comment 1 Hin-Chung Lam 2011-11-10 14:44:32 PST
Created attachment 114578 [details]
Patch
Comment 2 James Robinson 2011-11-10 14:51:03 PST
Comment on attachment 114578 [details]
Patch

You should apply the scroll deltas to CCLayerTreeHostImpl::m_scrollLayer on the impl side, not the root layer.
Comment 3 Adrienne Walker 2011-11-10 14:57:25 PST
There's also still the other option I suggested for the previous patch, of just storing the old scroll delta in a separate CCLayerImpl member variable that you can clear during commit without having to look up layers.

In other words, during the begin frame where we were previously clearing scroll delta:

layer->setSentScrollDelta(layer->sentScrollDelta() + layerScrollDelta());
layer->setScrollDelta(IntSize());

Then during commit, you could clear sentScrollDelta.  And, during calculateLayersAndVisibility, you could consider scrollDelta() and sentScrollDelta() together when calculating layer positions.

It's just a suggestion, but it might be a simpler approach than all of this.
Comment 4 Hin-Chung Lam 2011-11-10 15:01:35 PST
Created attachment 114583 [details]
Patch
Comment 5 James Robinson 2011-11-10 15:05:23 PST
Comment on attachment 114583 [details]
Patch

This seems reasonable for now. Enne's approach does seem better overall.
Comment 6 Hin-Chung Lam 2011-11-10 15:07:06 PST
I've tested the patch I just uploaded. This applies the scroll delta on the scroll layer that we know.

I think a simple way of doing this would be just pushing the position with respect to the container's visible rectangle.. that way there's no need to use scroll delta to position this layer.
Comment 7 Hin-Chung Lam 2011-11-11 04:16:13 PST
Comment on attachment 114583 [details]
Patch

Clearing flags on attachment: 114583

Committed r99942: <http://trac.webkit.org/changeset/99942>
Comment 8 Hin-Chung Lam 2011-11-11 04:16:20 PST
All reviewed patches have been landed.  Closing bug.