WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
78862
Register scrolling layers with ScrollingCoordinator
https://bugs.webkit.org/show_bug.cgi?id=78862
Summary
Register scrolling layers with ScrollingCoordinator
Sami Kyostila
Reported
2012-02-16 17:20:27 PST
Touch overflow scrolling elements (-webkit-overflow-scrolling: touch) should be split into layers and registered with the ScrollCoordinator so they can be scrolled efficiently without repainting the contents.
Attachments
Patch
(42.41 KB, patch)
2012-06-08 14:05 PDT
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Patch
(43.40 KB, patch)
2012-06-13 12:22 PDT
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Patch
(50.65 KB, patch)
2012-06-15 12:45 PDT
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Patch
(55.46 KB, patch)
2012-06-18 11:31 PDT
,
Sami Kyostila
no flags
Details
Formatted Diff
Diff
Patch
(74.37 KB, patch)
2012-06-20 07:25 PDT
,
Sami Kyöstilä
no flags
Details
Formatted Diff
Diff
Patch
(85.09 KB, patch)
2012-06-20 10:54 PDT
,
Sami Kyöstilä
no flags
Details
Formatted Diff
Diff
Patch
(24.97 KB, patch)
2012-07-26 11:26 PDT
,
Sami Kyöstilä
no flags
Details
Formatted Diff
Diff
Diff with 91117
(36.22 KB, patch)
2012-08-10 08:38 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Patch including 91117
(67.50 KB, patch)
2012-08-10 08:41 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Patch
(71.33 KB, patch)
2012-08-10 13:58 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-06
(367.26 KB, application/zip)
2012-08-10 18:13 PDT
,
WebKit Review Bot
no flags
Details
Patch
(29.97 KB, patch)
2012-08-30 11:33 PDT
,
Sami Kyöstilä
no flags
Details
Formatted Diff
Diff
Patch
(12.12 KB, patch)
2012-09-03 07:28 PDT
,
Sami Kyöstilä
no flags
Details
Formatted Diff
Diff
Show Obsolete
(11)
View All
Add attachment
proposed patch, testcase, etc.
Antonio Gomes
Comment 1
2012-05-22 13:36:15 PDT
Are you merging the code from iOS5 branch into trunk? Or actually re-working it or working from scratch?
Antonio Gomes
Comment 2
2012-05-22 13:58:58 PDT
(In reply to
comment #1
)
> Are you merging the code from iOS5 branch into trunk? Or actually re-working it or working from scratch?
I am asking because I have done that for the BlackBerry port myself, but it was not a clean backport...
Sami Kyostila
Comment 3
2012-05-23 03:57:56 PDT
It's really more of a reimplementation than a straight port. I agree that the iOS5 branch isn't really usable as such.
Sami Kyostila
Comment 4
2012-06-08 14:05:17 PDT
Created
attachment 146645
[details]
Patch First version with the main pieces of functionality in place. Does not calculate per-layer non-fast scrollable regions, but that could be left for a follow-up patch.
Antonio Gomes
Comment 5
2012-06-09 07:22:49 PDT
Comment on
attachment 146645
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=146645&action=review
> Source/WebCore/ChangeLog:12 > + Touch overflow scrolling elements should be split into layers > +
https://bugs.webkit.org/show_bug.cgi?id=78862
> + > + Reviewed by NOBODY (OOPS!). > + > + When a regular overflow element is scrolled, we repaint the entire area > + it occupies in the view from scratch. Since this painting is done in the > + main thread, it can be blocked by, e.g., JavaScript execution, further > + limiting performance. > +
I think it is valid and fair to mention that it was based on the original iOS5 branch implementation. I know it is a re-work, but it had a starting point..
> Source/WebCore/rendering/RenderLayer.cpp:4862 > + if (hasOverflow && isVisibleToHitTest > +#if ENABLE(OVERFLOW_SCROLLING) > + && !hasAcceleratedScrolling() > +#endif > + )
the blackberry port relies on all of the scrollable elements being tracked by the FrameView, in order to for example just skip a bunch of logic if there is no scrollable element (other than the main frame).No matter if it has "accelerated scrolling" or not. Could you explain why this change specifically?
Sami Kyostila
Comment 6
2012-06-11 10:38:32 PDT
Comment on
attachment 146645
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=146645&action=review
Something I failed to mention earlier is that I'm still working on layout tests for these changes.
>> Source/WebCore/ChangeLog:12 >> + > > I think it is valid and fair to mention that it was based on the original iOS5 branch implementation. I know it is a re-work, but it had a starting point..
You are absolutely right. I did not mean to pass this off as original work.
>> Source/WebCore/rendering/RenderLayer.cpp:4862 >> + ) > > the blackberry port relies on all of the scrollable elements being tracked by the FrameView, in order to for example just skip a bunch of logic if there is no scrollable element (other than the main frame).No matter if it has "accelerated scrolling" or not. > > Could you explain why this change specifically?
My motivation was to avoid the scrollable area from becoming part of the non-fast scrollable region (see ScrollingCoordinator.cpp) which would prevent it from being independently scrolled by the threaded compositor. Completely removing the scrollable layer from the scrollable set is a bit drastic, so perhaps it would be better to move this logic to ScrollingCoordinator instead.
Sami Kyostila
Comment 7
2012-06-13 12:22:45 PDT
Created
attachment 147387
[details]
Patch Updated to match new interface from
bug 73350
. Did not fix FrameView scrollable are registry yet.
Sami Kyostila
Comment 8
2012-06-15 12:45:19 PDT
Created
attachment 147879
[details]
Patch - Keep all ScrollableAreas -- fast scrollable or not -- in the FrameView's scrollable area set. - Add layout tests to verify graphics layer configuration and stacking context behavior with overflow:hidden.
Sami Kyostila
Comment 9
2012-06-15 12:51:34 PDT
The test coverage for the prepainting part of this patch isn't as good as I'd like yet. I'm thinking of somehow capturing the GraphicsLayer invalidations from DRT.
Sami Kyostila
Comment 10
2012-06-18 11:31:13 PDT
Created
attachment 148135
[details]
Patch - Added new test to verify scrolling tree using the framework from
bug 89216
.
Adam Barth
Comment 11
2012-06-18 15:43:54 PDT
Who should review this patch? @jamesr?
Sami Kyöstilä
Comment 12
2012-06-20 07:25:40 PDT
Created
attachment 148562
[details]
Patch - Cleaned up most of the #ifdef spaghetti. - Added test to verify scroll content layer alignment with borders. - Added test to check outline fallback.
Sami Kyöstilä
Comment 13
2012-06-20 10:54:34 PDT
Created
attachment 148604
[details]
Patch - Added overflow clipping test. Does not cover all the cases yet but it's a start. - Removed scrollingTreeAsText() test for now.
Simon Fraser (smfr)
Comment 14
2012-06-24 14:38:54 PDT
Comment on
attachment 148604
[details]
Patch I have some review feedback, but need a few days to look at the patch. Please wait for my feedback.
Simon Fraser (smfr)
Comment 15
2012-06-24 14:55:50 PDT
Sami: please cc: me on any future bugs that involve changes to RenderLayerCompositor/RenderLayerBacking/GraphicsLayer. Thanks.
Simon Fraser (smfr)
Comment 16
2012-06-26 19:17:02 PDT
Comment on
attachment 148604
[details]
Patch I plan to upsteam some iOS code that has a better implementation in RenderLayerBacking etc. Please give me a few days to do that.
Sami Kyöstilä
Comment 17
2012-07-02 02:53:45 PDT
(In reply to
comment #16
)
> I plan to upsteam some iOS code that has a better implementation in RenderLayerBacking etc. Please give me a few days to do that.
Okay, thanks Simon!
Adam Barth
Comment 18
2012-07-12 08:46:45 PDT
> I plan to upsteam some iOS code that has a better implementation in RenderLayerBacking etc. Please give me a few days to do that.
@smfr: Did this end up happening? This patch accounts for a moderate amount of the remaining chromium-android fork and I'd like to make sure we don't stall out here. Thanks!
Simon Fraser (smfr)
Comment 19
2012-07-12 08:49:52 PDT
Sorry I haven't had time yet.
Adam Barth
Comment 20
2012-07-12 09:24:02 PDT
> Sorry I haven't had time yet.
Would it be appropriate to proceed with this patch and then plan to merge with your work when you have time to contribute it? I don't fully understand the relation between the patches, so it's hard for me to know what the best course of action is here.
Simon Fraser (smfr)
Comment 21
2012-07-12 10:31:10 PDT
Unfortunately my patch will directly conflict with some of these changes (and addresses various issues that I think the current patch has). I'll try to get to it early next week.
Sami Kyöstilä
Comment 22
2012-07-12 10:33:24 PDT
(In reply to
comment #21
)
> Unfortunately my patch will directly conflict with some of these changes (and addresses various issues that I think the current patch has). I'll try to get to it early next week.
Thanks for the update. Is there another bug open for your patch, or were you going to land it against this one?
Simon Fraser (smfr)
Comment 23
2012-07-12 10:40:59 PDT
I'll do it via
bug 91117
.
Adam Barth
Comment 24
2012-07-12 10:46:48 PDT
Thanks Simon.
Adam Barth
Comment 25
2012-07-20 10:43:07 PDT
> I'll try to get to it early next week.
Simon, I'm sorry to keep nagging you about this issue, but you've been blocking this patch for upwards of a month now. This bug is blocking us from upstreaming the chromium-android port and is one of the larger diffs left in WebCore, as described in <
http://lists.webkit.org/pipermail/webkit-dev/2012-July/021516.html
>. How do we make progress here?
Simon Fraser (smfr)
Comment 26
2012-07-20 10:57:29 PDT
This is top of my list to do after some internal stuff.
Adam Barth
Comment 27
2012-07-20 11:00:14 PDT
> This is top of my list to do after some internal stuff.
Thanks Simon.
Antonio Gomes
Comment 28
2012-07-24 11:18:21 PDT
(In reply to
comment #27
)
> > This is top of my list to do after some internal stuff. > > Thanks Simon.
Just to make it clearer to me, once
bug 91117
is done, there should still work to be done here?
Simon Fraser (smfr)
Comment 29
2012-07-24 13:11:33 PDT
ENABLE(OVERFLOW_SCROLLING) is a really bad name for this flag. overflows already scroll. It should have been ENABLE(COMPOSITED_OVERFLOW_SCROLLING) or something.
Simon Fraser (smfr)
Comment 30
2012-07-24 13:14:59 PDT
Also, I think it's fine to only protected the CSS parser code with #if ENABLE(OVERFLOW_SCROLLING). That just needs to hide the new CSS property. The rest of the code doesn't need #ifdefs.
Antonio Gomes
Comment 31
2012-07-25 06:13:37 PDT
(In reply to
comment #28
)
> (In reply to
comment #27
) > > > This is top of my list to do after some internal stuff. > > > > Thanks Simon. > > Just to make it clearer to me, once
bug 91117
is done, there should still work to be done here?
Ok, given the patch Simon uploaded to
bug 91117
, the answer seems 'yes'. But it also seems it overlaps or even overwrites most of the changes the patch in this bug does in RenderLayer and RenderLayerBacking ...
Antonio Gomes
Comment 32
2012-07-25 06:39:13 PDT
The original iOS5 branch also had a related setting that I found useful, and maybe we should consider having them here: void setAlwaysUseAcceleratedOverflowScroll(bool flag) bool alwaysUseAcceleratedOverflowScroll() const
Sami Kyöstilä
Comment 33
2012-07-25 08:27:40 PDT
(In reply to
comment #31
)
> Ok, given the patch Simon uploaded to
bug 91117
, the answer seems 'yes'. But it also seems it overlaps or even overwrites most of the changes the patch in this bug does in RenderLayer and RenderLayerBacking ...
Right, as far as I can tell the following things are still needed on top of
bug 91117
: - Letting ScrollingCoordinator know about scrollable layers. - Avoiding repainting layer contents after scrolling. - Possibly avoiding overflow clipping when calculating paint rects (e.g., RenderObject::computeRectForRepaint() in this patch).
Simon Fraser (smfr)
Comment 34
2012-07-25 09:15:11 PDT
(In reply to
comment #32
)
> The original iOS5 branch also had a related setting that I found useful, and maybe we should consider having them here: > > void setAlwaysUseAcceleratedOverflowScroll(bool flag) > bool alwaysUseAcceleratedOverflowScroll() const
I don't think you want those. The idea is they make every overflow:scroll use composting and create stacking context, which can break web pages.
Simon Fraser (smfr)
Comment 35
2012-07-25 09:16:08 PDT
(In reply to
comment #33
)
> (In reply to
comment #31
) > > Ok, given the patch Simon uploaded to
bug 91117
, the answer seems 'yes'. But it also seems it overlaps or even overwrites most of the changes the patch in this bug does in RenderLayer and RenderLayerBacking ... > > Right, as far as I can tell the following things are still needed on top of
bug 91117
: > > - Letting ScrollingCoordinator know about scrollable layers. > - Avoiding repainting layer contents after scrolling.
Yes on these two.
> - Possibly avoiding overflow clipping when calculating paint rects (e.g., RenderObject::computeRectForRepaint() in this patch).
I don't think you need any changes here; the repaintContainner logic should take case of this.
Antonio Gomes
Comment 36
2012-07-25 09:49:15 PDT
(In reply to
comment #34
)
> (In reply to
comment #32
) > > The original iOS5 branch also had a related setting that I found useful, and maybe we should consider having them here: > > > > void setAlwaysUseAcceleratedOverflowScroll(bool flag) > > bool alwaysUseAcceleratedOverflowScroll() const > > I don't think you want those. The idea is they make every overflow:scroll use composting and create stacking context, which can break web pages.
would you have example of broken web pages with that overly enabled?
Sami Kyöstilä
Comment 37
2012-07-26 11:26:03 PDT
Created
attachment 154694
[details]
Patch Work-in-progress patch against 91117 - Register scrolling layers with ScrollingCoordinator. - Implement bounds origin in Chromium compositor. - Keep scrolling contents layer under overflow controls. - Don't reparent overflow controls inside scrolling content. - Promote composited scrolling layers to composited layers. - Include composited scrolling layers in memory usage estimate. - Avoid repaints when scrolling (not working yet).
Adrienne Walker
Comment 38
2012-07-26 11:35:05 PDT
Comment on
attachment 154694
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=154694&action=review
> Source/WebCore/platform/graphics/chromium/LayerChromium.h:120 > + const FloatPoint& boundsOrigin() const { return m_boundsOrigin; } > + void setBoundsOrigin(const FloatPoint& origin) { m_boundsOrigin = origin; }
What does bounds origin mean? Forgive my skepticism about adding another member, but are you sure this translation can't be combined into one of position, anchor, transform, sublayerTransform, or scrollPosition at the LayerChromium level?
Sami Kyöstilä
Comment 39
2012-07-26 11:37:48 PDT
(In reply to
comment #38
)
> What does bounds origin mean? > > Forgive my skepticism about adding another member, but are you sure this translation can't be combined into one of position, anchor, transform, sublayerTransform, or scrollPosition at the LayerChromium level?
As far as I can tell it could be baked into sublayerTransform, so I don't think the compositor actually needs to care about it. I'm with you on not adding any more degrees of freedom to the layer transforms.
Sami Kyöstilä
Comment 40
2012-07-26 11:42:58 PDT
(In reply to
comment #35
)
> > - Possibly avoiding overflow clipping when calculating paint rects (e.g., RenderObject::computeRectForRepaint() in this patch). > > I don't think you need any changes here; the repaintContainner logic should take case of this.
I tried to make this work but looks like the repaints are still getting clipped to the visible part of the overflow contents. For example: 1. Make an overflow div with lots of text. 2. Select a part of the text at the beginning and scroll to the end. 3. Deselect and scroll back to the beginning. => The text is still selected, because the deselect repaint got clipped. I don't quite understand how this should work without turning off the overflow clipping in RenderObject et al. Did I miss something?
Adrienne Walker
Comment 41
2012-07-26 11:48:02 PDT
(In reply to
comment #40
)
> I don't quite understand how this should work without turning off the overflow clipping in RenderObject et al. Did I miss something?
For what it's worth, Chromium explicitly doesn't clip repaint rects for frame scrolling so that we can know which parts of the layer are invalidated even if they're not visible and don't have to invalidate on scroll. See FrameView::clipsRepaints().
Simon Fraser (smfr)
Comment 42
2012-07-26 11:52:58 PDT
(In reply to
comment #38
)
> (From update of
attachment 154694
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=154694&action=review
> > > Source/WebCore/platform/graphics/chromium/LayerChromium.h:120 > > + const FloatPoint& boundsOrigin() const { return m_boundsOrigin; } > > + void setBoundsOrigin(const FloatPoint& origin) { m_boundsOrigin = origin; } > > What does bounds origin mean? > > Forgive my skepticism about adding another member, but are you sure this translation can't be combined into one of position, anchor, transform, sublayerTransform, or scrollPosition at the LayerChromium level?
We used boundsOrigin because it plays nicely with UIScrollViews on iOS.
James Robinson
Comment 43
2012-07-27 13:50:27 PDT
(In reply to
comment #42
)
> (In reply to
comment #38
) > > (From update of
attachment 154694
[details]
[details]) > > View in context:
https://bugs.webkit.org/attachment.cgi?id=154694&action=review
> > > > > Source/WebCore/platform/graphics/chromium/LayerChromium.h:120 > > > + const FloatPoint& boundsOrigin() const { return m_boundsOrigin; } > > > + void setBoundsOrigin(const FloatPoint& origin) { m_boundsOrigin = origin; } > > > > What does bounds origin mean? > > > > Forgive my skepticism about adding another member, but are you sure this translation can't be combined into one of position, anchor, transform, sublayerTransform, or scrollPosition at the LayerChromium level? > > We used boundsOrigin because it plays nicely with UIScrollViews on iOS.
I don't understand what UIScrollViews has to do with adding a new property to LayerChromium.
vollick
Comment 44
2012-08-10 08:38:06 PDT
Created
attachment 157739
[details]
Diff with 91117 This patch set removes boundsOrigin from LayerChromium and below and instead bakes the value into the sublayerTransform.
vollick
Comment 45
2012-08-10 08:41:16 PDT
Created
attachment 157740
[details]
Patch including 91117 This attachment also contains code from 91117 (so that it can apply for the ews bots).
WebKit Review Bot
Comment 46
2012-08-10 08:43:56 PDT
Please wait for approval from
abarth@webkit.org
,
dglazkov@chromium.org
,
fishd@chromium.org
,
jamesr@chromium.org
or
tkent@chromium.org
before submitting, as this patch contains changes to the Chromium public API. See also
https://trac.webkit.org/wiki/ChromiumWebKitAPI
.
WebKit Review Bot
Comment 47
2012-08-10 09:47:35 PDT
Comment on
attachment 157740
[details]
Patch including 91117
Attachment 157740
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/13479010
Simon Fraser (smfr)
Comment 48
2012-08-10 11:04:04 PDT
(In reply to
comment #44
)
> Created an attachment (id=157739) [details] > Diff with 91117 > > This patch set removes boundsOrigin from LayerChromium and below and instead > bakes the value into the sublayerTransform.
How does that work if the overflow element also has perspective style?
vollick
Comment 49
2012-08-10 13:58:10 PDT
Created
attachment 157805
[details]
Patch Adding missing file: WebScrollDelegate.h.
WebKit Review Bot
Comment 50
2012-08-10 18:13:47 PDT
Comment on
attachment 157805
[details]
Patch
Attachment 157805
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/13464933
New failing tests: fast/css/resize-corner-tracking-transformed.html compositing/geometry/clip.html compositing/overflow/overflow-scroll.html
WebKit Review Bot
Comment 51
2012-08-10 18:13:55 PDT
Created
attachment 157848
[details]
Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
vollick
Comment 52
2012-08-13 11:34:48 PDT
The layout test failures appear to be due to 91117, so this should still be safe to review.
Sami Kyöstilä
Comment 53
2012-08-16 03:15:37 PDT
Comment on
attachment 157805
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=157805&action=review
> Source/WebKit/chromium/features.gypi:182 > + 'ENABLE_OVERFLOW_SCROLLING=1',
I don't think we want to enable this unconditionally just yet -- see, e.g.,
http://code.google.com/p/chromium/issues/detail?id=128325
for the kinds of problems it can cause.
Antonio Gomes
Comment 54
2012-08-16 12:53:17 PDT
Comment on
attachment 157805
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=157805&action=review
> Source/WebCore/rendering/RenderLayerBacking.cpp:673 > + // FIXME: Should use DontSetNeedsDisplay to avoid repainting the entire layer. > + m_scrollingContentsLayer->setOffsetFromRenderer(scrollingContentsOffset, GraphicsLayer::SetNeedsDisplay);
why the fixme instead of using it right way?
Sami Kyöstilä
Comment 55
2012-08-17 02:36:20 PDT
(In reply to
comment #54
)
> > + // FIXME: Should use DontSetNeedsDisplay to avoid repainting the entire layer. > > + m_scrollingContentsLayer->setOffsetFromRenderer(scrollingContentsOffset, GraphicsLayer::SetNeedsDisplay); > > why the fixme instead of using it right way?
The problem is that with the current patch, doing this produces a lot of rendering corruption which we need to fix first. I think we'll want to get
bug 91117
landed first to provide the scrolling layer infrastructure and then use this bug to make prepainting et al work.
James Robinson
Comment 56
2012-08-27 16:57:53 PDT
(In reply to
comment #55
)
> (In reply to
comment #54
) > > > + // FIXME: Should use DontSetNeedsDisplay to avoid repainting the entire layer. > > > + m_scrollingContentsLayer->setOffsetFromRenderer(scrollingContentsOffset, GraphicsLayer::SetNeedsDisplay); > > > > why the fixme instead of using it right way? > > The problem is that with the current patch, doing this produces a lot of rendering corruption which we need to fix first. I think we'll want to get
bug 91117
landed first to provide the scrolling layer infrastructure and then use this bug to make prepainting et al work.
91117 has landed now - what's next for this patch?
James Robinson
Comment 57
2012-08-27 17:02:02 PDT
Comment on
attachment 157805
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=157805&action=review
This needs a rebase now that 91117 has landed
>> Source/WebCore/rendering/RenderLayerBacking.cpp:673 >> + m_scrollingContentsLayer->setOffsetFromRenderer(scrollingContentsOffset, GraphicsLayer::SetNeedsDisplay); > > why the fixme instead of using it right way?
Now that
bug 91117
has landed I'd like to see an attempt at this.
Sami Kyöstilä
Comment 58
2012-08-28 03:46:51 PDT
(In reply to
comment #57
)
> > why the fixme instead of using it right way? > > Now that
bug 91117
has landed I'd like to see an attempt at this.
Agreed, I'm on it. The way I see it there are two things we need: telling ScrollingCoordinator about the scrolling layers and implementing prepainting so that we don't need to fully invalidate the layers as they scroll. I've updated the title to better match the scope -- please let me know if want this bug split further.
James Robinson
Comment 59
2012-08-29 14:41:18 PDT
(In reply to
comment #58
)
> (In reply to
comment #57
) > > > why the fixme instead of using it right way? > > > > Now that
bug 91117
has landed I'd like to see an attempt at this. > > Agreed, I'm on it. The way I see it there are two things we need: telling ScrollingCoordinator about the scrolling layers and implementing prepainting so that we don't need to fully invalidate the layers as they scroll. > > I've updated the title to better match the scope -- please let me know if want this bug split further.
It's hard to say when to split. I think you should definitely split cross-platform changes from chromium-only changes, and then split as things get unwieldy.
Sami Kyöstilä
Comment 60
2012-08-30 11:22:14 PDT
(In reply to
comment #59
)
> It's hard to say when to split. I think you should definitely split cross-platform changes from chromium-only changes, and then split as things get unwieldy.
Ok. I'll use this bug for the registration part since it's (mostly) Chromium and will open another bug for prepainting.
Sami Kyöstilä
Comment 61
2012-08-30 11:33:02 PDT
Created
attachment 161523
[details]
Patch
James Robinson
Comment 62
2012-08-30 15:08:54 PDT
Comment on
attachment 161523
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=161523&action=review
> Source/Platform/ChangeLog:3 > + Register scrolling layers with ScrollingCoordinator
this is an ~80% chromium-specific change with some changes in cross-platform code like RenderLayerBacking/etc. Please split the chromium parts out into a different patch so it can be reviewed separately.
> Source/Platform/chromium/public/WebLayer.h:177 > + virtual void setScrollDelegate(WebLayerScrollDelegate*) = 0;
document this please - in particular what the rules are for the lifetime of the passed in pointer (outlives the WebLayer, I'm guessing?) and whether you can have multiple (I'm guessing no?)
> Source/Platform/chromium/public/WebLayerScrollDelegate.h:32 > +class WebLayerScrollDelegate {
s/Delegate/Client/, please. we shouldn't be using 'delegate' in the WebKit API
> Source/Platform/chromium/public/WebLayerScrollDelegate.h:35 > +};
declared d'tor please (protected most likely since you shouldn't call delete on a client ptr)
> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:33 > +#include "LayerChromium.h"
this is a really bad dependency - you can't depend on LayerChromium outside of the compositor implementation
> Source/WebCore/platform/ScrollableArea.h:173 > + virtual bool usesCompositedScrolling() const { return false; }
why is this outside of USE(ACCELERATED_COMPOSITING) ?
Sami Kyöstilä
Comment 63
2012-09-03 07:13:26 PDT
Comment on
attachment 161523
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=161523&action=review
>> Source/Platform/ChangeLog:3 >> + Register scrolling layers with ScrollingCoordinator > > this is an ~80% chromium-specific change with some changes in cross-platform code like RenderLayerBacking/etc. Please split the chromium parts out into a different patch so it can be reviewed separately.
Sure, here's the Chromium part:
https://bugs.webkit.org/show_bug.cgi?id=95679
>> Source/Platform/chromium/public/WebLayer.h:177 >> + virtual void setScrollDelegate(WebLayerScrollDelegate*) = 0; > > document this please - in particular what the rules are for the lifetime of the passed in pointer (outlives the WebLayer, I'm guessing?) and whether you can have multiple (I'm guessing no?)
Good idea. I've added a comment block.
>> Source/Platform/chromium/public/WebLayerScrollDelegate.h:32 >> +class WebLayerScrollDelegate { > > s/Delegate/Client/, please. we shouldn't be using 'delegate' in the WebKit API
Done. FWIW, this was modeled after WebAnimationDelegate.
>> Source/Platform/chromium/public/WebLayerScrollDelegate.h:35 >> +}; > > declared d'tor please (protected most likely since you shouldn't call delete on a client ptr)
Done.
>> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:33 >> +#include "LayerChromium.h" > > this is a really bad dependency - you can't depend on LayerChromium outside of the compositor implementation
Sorry, I added this include by mistake; there isn't an actual dependency here.
>> Source/WebCore/platform/ScrollableArea.h:173 >> + virtual bool usesCompositedScrolling() const { return false; } > > why is this outside of USE(ACCELERATED_COMPOSITING) ?
This used to be so because RenderLayer exposes usesCompositedScrolling() both with and without USE(ACCELERATED_COMPOSITING). I've now rearranged things so that RenderLayer uses OVERRIDE for usesCompositedScrolling() based on the build configuration.
Sami Kyöstilä
Comment 64
2012-09-03 07:28:34 PDT
Created
attachment 161924
[details]
Patch Cross-platform patch.
James Robinson
Comment 65
2012-09-04 11:30:05 PDT
Comment on
attachment 161924
[details]
Patch R=me
WebKit Review Bot
Comment 66
2012-09-04 11:53:37 PDT
Comment on
attachment 161924
[details]
Patch Clearing flags on attachment: 161924 Committed
r127480
: <
http://trac.webkit.org/changeset/127480
>
WebKit Review Bot
Comment 67
2012-09-04 11:53:46 PDT
All reviewed patches have been landed. Closing bug.
Antonio Gomes
Comment 68
2012-09-06 08:22:50 PDT
(In reply to
comment #57
)
> (From update of
attachment 157805
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=157805&action=review
> > This needs a rebase now that 91117 has landed > > >> Source/WebCore/rendering/RenderLayerBacking.cpp:673 > >> + m_scrollingContentsLayer->setOffsetFromRenderer(scrollingContentsOffset, GraphicsLayer::SetNeedsDisplay); > > > > why the fixme instead of using it right way? > > Now that
bug 91117
has landed I'd like to see an attempt at this.
If you referred to an attempt to fix the FIXME I pointed out above, if did not happen afaik.
Sami Kyöstilä
Comment 69
2012-09-07 02:31:29 PDT
(In reply to
comment #68
)
> If you referred to an attempt to fix the FIXME I pointed out above, if did not happen afaik.
That's right, I've opened
https://bugs.webkit.org/show_bug.cgi?id=96087
to track that.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug