Bug 102543

Summary: ScrollingCoordinator::hasVisibleSlowRepaintFixedObject() should exclude out-of-view fixed position elements
Product: WebKit Reporter: Xianzhu Wang <wangxianzhu>
Component: Layout and RenderingAssignee: Xianzhu Wang <wangxianzhu>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, bdakin, dglazkov, enne, eric, gtk-ews, jamesr, ojan.autocc, ojan, shawnsingh, simon.fraser, skyostil, tonikitoo, trchen, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 104277, 104294, 104303, 104714    
Bug Blocks: 103470, 104724    
Attachments:
Description Flags
Patch
none
Add symbol
none
For review
none
avoid new field
none
Fix layout tests
none
Patch
none
Fix layout tests on Mac
none
Rebase and adjust indent
none
Rebase again
none
Fix win build
none
Try WebKit2.def.in change
none
Yet another try of WebKit2.def.in change
none
Workaround Windows linker issue (bug 104136)
none
Patch (independent with the blocking bugs)
none
New patch
none
Fix mac build break
none
Let RLC manage reasons of fixed pos layer not composited
none
Let RLC manage reasons of fixedpos layer not composited
none
Update test to cover issue of a previous patch
none
Smaller patch
none
For landing none

Xianzhu Wang
Reported 2012-11-16 11:40:58 PST
Bug 101303 resolved the issue on Mac only. On other platforms, as ScrollingCoordinator::hasNonLayerFixedObjects() is renamed to hasVisibleSlowRepaintFixedObjects(), it should exclude out-of-view fixed objects to avoid the issue described in bug 101303.
Attachments
Patch (24.94 KB, patch)
2012-11-19 17:47 PST, Xianzhu Wang
no flags
Add symbol (29.40 KB, patch)
2012-11-20 10:41 PST, Xianzhu Wang
no flags
For review (29.52 KB, patch)
2012-11-20 11:57 PST, Xianzhu Wang
no flags
avoid new field (28.78 KB, patch)
2012-11-26 16:12 PST, Xianzhu Wang
no flags
Fix layout tests (43.43 KB, patch)
2012-11-27 12:13 PST, Xianzhu Wang
no flags
Patch (43.51 KB, patch)
2012-11-27 16:07 PST, Xianzhu Wang
no flags
Fix layout tests on Mac (39.67 KB, patch)
2012-11-28 11:44 PST, Xianzhu Wang
no flags
Rebase and adjust indent (41.06 KB, patch)
2012-12-03 15:46 PST, Xianzhu Wang
no flags
Rebase again (40.41 KB, patch)
2012-12-04 10:15 PST, Xianzhu Wang
no flags
Fix win build (41.93 KB, patch)
2012-12-04 13:19 PST, Xianzhu Wang
no flags
Try WebKit2.def.in change (64.19 KB, patch)
2012-12-04 16:41 PST, Xianzhu Wang
no flags
Yet another try of WebKit2.def.in change (43.72 KB, patch)
2012-12-05 09:03 PST, Xianzhu Wang
no flags
Workaround Windows linker issue (bug 104136) (42.24 KB, patch)
2012-12-05 10:30 PST, Xianzhu Wang
no flags
Patch (independent with the blocking bugs) (20.12 KB, patch)
2012-12-06 17:45 PST, Xianzhu Wang
no flags
New patch (21.78 KB, patch)
2012-12-07 11:58 PST, Xianzhu Wang
no flags
Fix mac build break (23.70 KB, patch)
2012-12-07 13:32 PST, Xianzhu Wang
no flags
Let RLC manage reasons of fixed pos layer not composited (26.55 KB, patch)
2012-12-10 21:50 PST, Xianzhu Wang
no flags
Let RLC manage reasons of fixedpos layer not composited (26.73 KB, patch)
2012-12-11 10:32 PST, Xianzhu Wang
no flags
Update test to cover issue of a previous patch (26.84 KB, patch)
2012-12-11 11:16 PST, Xianzhu Wang
no flags
Smaller patch (13.62 KB, patch)
2012-12-11 17:20 PST, Xianzhu Wang
no flags
For landing (13.71 KB, patch)
2012-12-11 18:21 PST, Xianzhu Wang
no flags
Beth Dakin
Comment 1 2012-11-16 11:43:47 PST
(In reply to comment #0) > Bug 101303 resolved the issue on Mac only. On other platforms, as ScrollingCoordinator::hasNonLayerFixedObjects() is renamed to hasVisibleSlowRepaintFixedObjects(), it should exclude out-of-view fixed objects to avoid the issue described in bug 101303. No other ports actually use threaded scrolling, so is there any real benefit to this change?
Xianzhu Wang
Comment 2 2012-11-16 11:48:40 PST
(In reply to comment #1) > No other ports actually use threaded scrolling, so is there any real benefit to this change? It'll benefit Chromium which does accelerated scrolling in the compositor thread.
Beth Dakin
Comment 3 2012-11-16 11:49:12 PST
(In reply to comment #2) > (In reply to comment #1) > > No other ports actually use threaded scrolling, so is there any real benefit to this change? > > It'll benefit Chromium which does accelerated scrolling in the compositor thread. Got it.
Xianzhu Wang
Comment 4 2012-11-19 17:47:27 PST
kov's GTK+ EWS bot
Comment 5 2012-11-19 18:16:38 PST
Build Bot
Comment 6 2012-11-19 18:47:28 PST
Build Bot
Comment 7 2012-11-19 19:09:18 PST
Xianzhu Wang
Comment 8 2012-11-20 10:41:35 PST
Created attachment 175244 [details] Add symbol
Xianzhu Wang
Comment 9 2012-11-20 11:57:23 PST
Created attachment 175255 [details] For review
Simon Fraser (smfr)
Comment 10 2012-11-23 11:30:59 PST
Comment on attachment 175255 [details] For review View in context: https://bugs.webkit.org/attachment.cgi?id=175255&action=review > Source/WebCore/rendering/RenderLayerModelObject.h:65 > + bool m_isOutOfViewFixedPositioned : 1; I don't think we should waste a member variable for this state which only applies to position:fixed elements for some ports; it probably increases the size of the class, possibly by 4 bytes. Please measure.
Xianzhu Wang
Comment 11 2012-11-26 16:12:18 PST
Created attachment 176104 [details] avoid new field
WebKit Review Bot
Comment 12 2012-11-26 22:44:27 PST
Comment on attachment 176104 [details] avoid new field Attachment 176104 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15010045 New failing tests: compositing/geometry/fixed-position-composited-page-scale-down.html compositing/geometry/fixed-position-transform-composited-page-scale-down.html platform/chromium/virtual/softwarecompositing/geometry/fixed-position-composited-page-scale.html platform/chromium/virtual/softwarecompositing/geometry/fixed-position-transform-composited-page-scale-down.html compositing/geometry/fixed-position-composited-page-scale.html platform/chromium/virtual/softwarecompositing/geometry/fixed-position-composited-page-scale-down.html
Xianzhu Wang
Comment 13 2012-11-27 12:13:14 PST
Created attachment 176320 [details] Fix layout tests
Xianzhu Wang
Comment 14 2012-11-27 16:07:12 PST
Xianzhu Wang
Comment 15 2012-11-27 16:43:06 PST
Simon, could you take another look? Thanks.
Build Bot
Comment 16 2012-11-28 07:24:13 PST
Comment on attachment 176355 [details] Patch Attachment 176355 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15003868 New failing tests: compositing/geometry/fixed-position-composited-page-scale-scroll.html compositing/geometry/fixed-position-composited-page-scale.html compositing/geometry/fixed-position-composited-page-scale-down.html fast/events/overflow-scroll-fake-mouse-move.html
Xianzhu Wang
Comment 17 2012-11-28 11:44:40 PST
Created attachment 176534 [details] Fix layout tests on Mac
Xianzhu Wang
Comment 18 2012-11-30 09:23:21 PST
Simon, what do you think about this change?
Adam Barth
Comment 19 2012-12-03 13:52:21 PST
Comment on attachment 176534 [details] Fix layout tests on Mac View in context: https://bugs.webkit.org/attachment.cgi?id=176534&action=review > Source/WebCore/page/Page.cpp:260 > + if (Document* document = m_mainFrame->document()) This can't ever be 0. > LayoutTests/compositing/geometry/fixed-position-composited-page-scale-down.html:17 > + window.internals.settings.setEnableCompositingForFixedPosition(true); four-space indent please
Xianzhu Wang
Comment 20 2012-12-03 14:13:02 PST
(In reply to comment #19) Thanks for review. > (From update of attachment 176534 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176534&action=review > > > Source/WebCore/page/Page.cpp:260 > > + if (Document* document = m_mainFrame->document()) > > This can't ever be 0. > Are you sure about this? I saw many other similar checks in both Page.cpp and Frame.cpp.
Xianzhu Wang
Comment 21 2012-12-03 15:31:14 PST
(In reply to comment #19) > (From update of attachment 176534 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176534&action=review > > > Source/WebCore/page/Page.cpp:260 > > + if (Document* document = m_mainFrame->document()) > > This can't ever be 0. Is your point base on that the method is only called from Internals::mainThreadScrollingReasons()? It seems better to me to have the check in Page::mainThreadScrollingReasonsAsText() for safety (in case that it's called from other places) and consistency.
Xianzhu Wang
Comment 22 2012-12-03 15:46:32 PST
Created attachment 177356 [details] Rebase and adjust indent
Xianzhu Wang
Comment 23 2012-12-03 16:00:32 PST
Comment on attachment 176534 [details] Fix layout tests on Mac View in context: https://bugs.webkit.org/attachment.cgi?id=176534&action=review >>>> Source/WebCore/page/Page.cpp:260 >>>> + if (Document* document = m_mainFrame->document()) >>> >>> This can't ever be 0. >> >> Are you sure about this? I saw many other similar checks in both Page.cpp and Frame.cpp. > > Is your point base on that the method is only called from Internals::mainThreadScrollingReasons()? It seems better to me to have the check in Page::mainThreadScrollingReasonsAsText() for safety (in case that it's called from other places) and consistency. Just noticed that the last patch I uploaded is a version with the check removed. (I have no preference.) >> LayoutTests/compositing/geometry/fixed-position-composited-page-scale-down.html:17 >> + window.internals.settings.setEnableCompositingForFixedPosition(true); > > four-space indent please Done.
Xianzhu Wang
Comment 24 2012-12-04 10:15:33 PST
Created attachment 177506 [details] Rebase again
Build Bot
Comment 25 2012-12-04 10:43:16 PST
Comment on attachment 177506 [details] Rebase again Attachment 177506 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15119889
Xianzhu Wang
Comment 26 2012-12-04 13:19:18 PST
Created attachment 177534 [details] Fix win build
Build Bot
Comment 27 2012-12-04 13:45:32 PST
Comment on attachment 177534 [details] Fix win build Attachment 177534 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15154083
Xianzhu Wang
Comment 28 2012-12-04 16:41:26 PST
Created attachment 177603 [details] Try WebKit2.def.in change
Build Bot
Comment 29 2012-12-04 20:06:08 PST
Comment on attachment 177603 [details] Try WebKit2.def.in change Attachment 177603 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15153238
Xianzhu Wang
Comment 30 2012-12-05 09:03:45 PST
Created attachment 177767 [details] Yet another try of WebKit2.def.in change
Build Bot
Comment 31 2012-12-05 10:09:57 PST
Comment on attachment 177767 [details] Yet another try of WebKit2.def.in change Attachment 177767 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15147552
Xianzhu Wang
Comment 32 2012-12-05 10:30:20 PST
Created attachment 177789 [details] Workaround Windows linker issue (bug 104136)
Xianzhu Wang
Comment 33 2012-12-05 12:11:51 PST
@Simon, would you please review the change, or suggest another reviewer? Thanks!
Xianzhu Wang
Comment 34 2012-12-06 10:56:41 PST
To ease review, I will split this change into multiple ones.
Xianzhu Wang
Comment 35 2012-12-06 17:45:56 PST
Created attachment 178122 [details] Patch (independent with the blocking bugs)
Xianzhu Wang
Comment 36 2012-12-06 17:49:48 PST
(In reply to comment #35) > Created an attachment (id=178122) [details] > Patch (independent with the blocking bugs) We really want to fix this issue soon, so I created the last patch that resolves only part of the issue (the unscaled case) not depending on the other sub bugs.
Simon Fraser (smfr)
Comment 37 2012-12-06 17:59:52 PST
Comment on attachment 178122 [details] Patch (independent with the blocking bugs) View in context: https://bugs.webkit.org/attachment.cgi?id=178122&action=review > Source/WebCore/rendering/RenderLayer.cpp:3130 > + if (!isComposited() && isStackingContext() && m_renderer->style()->position() == FixedPosition && m_renderer->container() == m_renderer->view()) (isStackingContext() && m_renderer->style()->position() == FixedPosition && m_renderer->container() == m_renderer->view()) should be packaged up into a function that's called from both RLC and this code. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1984 > - if (!viewBounds.intersects(layerBounds)) > + if (!viewBounds.intersects(layerBounds)) { > + frameView->removeViewportConstrainedObject(renderer); > return false; > + } > } > > + frameView->addViewportConstrainedObject(renderer); I don't like the fact that this code is messing with the FrameView's viewportConstrainedObjects. I think you should track whether there are any ignorable Fixed objects in RLC. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1988 > + if (Settings* settings = m_renderView->document()->settings()) > + if (!settings->acceleratedCompositingForFixedPositionEnabled()) > + return false; Why is this test at the end?
Xianzhu Wang
Comment 38 2012-12-07 10:31:38 PST
Comment on attachment 178122 [details] Patch (independent with the blocking bugs) View in context: https://bugs.webkit.org/attachment.cgi?id=178122&action=review >> Source/WebCore/rendering/RenderLayer.cpp:3130 >> + if (!isComposited() && isStackingContext() && m_renderer->style()->position() == FixedPosition && m_renderer->container() == m_renderer->view()) > > (isStackingContext() && m_renderer->style()->position() == FixedPosition && m_renderer->container() == m_renderer->view()) should be packaged up into a function that's called from both RLC and this code. Actually the common part is only (isStackingContext() && m_renderer->style()->position() == FixedPosition). RLC needs to check the container separately as it need to check the NULL condition. Also I'm not sure how to name it. Does isFixedPositionAndStackingContext() look good? >> Source/WebCore/rendering/RenderLayerCompositor.cpp:1984 >> + frameView->addViewportConstrainedObject(renderer); > > I don't like the fact that this code is messing with the FrameView's viewportConstrainedObjects. I think you should track whether there are any ignorable Fixed objects in RLC. I think "ignorable" may mean different things. ScrollingCoordinator needs to exclude objects that are not actually viewport constrained in the viewportConstrainedObjects list when determine main thread scrolling. RenderLayer should not paint out-of-view fixed position layer when doing pre-painting. So I'm not sure if we can track ignorable fixed objects in RLC for the different needs. About viewportConstrainedObjects, I tried two methods: 1) Check if the object in FrameView::viewportConstrainedObjects() is actually viewport constrained in ScrollingCoordinator::hasVisibleSlowRepaintFixedObjects() (the previous patch) 2) Update FrameView::viewConstrainedObjects() by excluding non-viewport-constrained objects (the current patch) More information about method 2: in patch of bug 103470 (which needs to exclude another category of elements from viewportConstrainedObjects), skyostil@ suggested to let FrameView::viewportConstrainedObjects() return only the actual viewport constrained objects excluding both out-of-view ones and the ones under transformed container. This needs change of where FrameView's viewportConstrainedObjects are updated. I haven't found a proper place yet. RLC seems not a good place. Does method 1 look good to you? >> Source/WebCore/rendering/RenderLayerCompositor.cpp:1988 >> + return false; > > Why is this test at the end? I moved it here because in spite of acceleratedCompositingForFixedPositionEnabled, the invisible fixed position element should not be a viewport constrained object and should not cause slow scrolling. Won't need this if we don't call FrameView::add/removeViewportConstrainedObject() here.
Simon Fraser (smfr)
Comment 39 2012-12-07 10:41:52 PST
(In reply to comment #38) > (From update of attachment 178122 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178122&action=review > > >> Source/WebCore/rendering/RenderLayer.cpp:3130 > >> + if (!isComposited() && isStackingContext() && m_renderer->style()->position() == FixedPosition && m_renderer->container() == m_renderer->view()) > > > > (isStackingContext() && m_renderer->style()->position() == FixedPosition && m_renderer->container() == m_renderer->view()) should be packaged up into a function that's called from both RLC and this code. > > Actually the common part is only (isStackingContext() && m_renderer->style()->position() == FixedPosition). RLC needs to check the container separately as it need to check the NULL condition. > Also I'm not sure how to name it. Does isFixedPositionAndStackingContext() look good? isStackingContextFixedPosition()? > > >> Source/WebCore/rendering/RenderLayerCompositor.cpp:1984 > >> + frameView->addViewportConstrainedObject(renderer); > > > > I don't like the fact that this code is messing with the FrameView's viewportConstrainedObjects. I think you should track whether there are any ignorable Fixed objects in RLC. > > I think "ignorable" may mean different things. ScrollingCoordinator needs to exclude objects that are not actually viewport constrained in the viewportConstrainedObjects list when determine main thread scrolling. RenderLayer should not paint out-of-view fixed position layer when doing pre-painting. So I'm not sure if we can track ignorable fixed objects in RLC for the different needs. Doesn't the same apply to the FrameView list of viewport-constrained objects then? > About viewportConstrainedObjects, I tried two methods: > 1) Check if the object in FrameView::viewportConstrainedObjects() is actually viewport constrained in ScrollingCoordinator::hasVisibleSlowRepaintFixedObjects() (the previous patch) When does viewportConstrainedObjects() contain something that is not actually viewport-constrained? Out-of-view fixed things should still be considered viewport-constained, I think. Or, exclude them from FrameView::viewportConstrainedObjects(), but do so from RenderLayer or rendering code. Currently frameView->addViewportConstrainedObject(this); is called by RenderLayerModelObject::styleDidChange(), but at that point you don't know about layout, so you can't compare the bounds with the viewport. I think we need a general solution here that works independently of compositing. Ideally it would be smart enough to handle position:-webkit-sticky things that are scrolled out of view too. > 2) Update FrameView::viewConstrainedObjects() by excluding non-viewport-constrained objects (the current patch) But this patch does it via compositing code, which is wrong. So the main question here is should FrameView::viewportConstrainedObjects() become FrameView::visibleViewportConstrainedObjects(), and, if so, how do we maintain this list correctly. If not, we need to do visibility/transformed container checks elsewhere.
Xianzhu Wang
Comment 40 2012-12-07 11:10:47 PST
Comment on attachment 178122 [details] Patch (independent with the blocking bugs) View in context: https://bugs.webkit.org/attachment.cgi?id=178122&action=review >>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:1984 >>>> + frameView->addViewportConstrainedObject(renderer); >>> >>> I don't like the fact that this code is messing with the FrameView's viewportConstrainedObjects. I think you should track whether there are any ignorable Fixed objects in RLC. >> >> I think "ignorable" may mean different things. ScrollingCoordinator needs to exclude objects that are not actually viewport constrained in the viewportConstrainedObjects list when determine main thread scrolling. RenderLayer should not paint out-of-view fixed position layer when doing pre-painting. So I'm not sure if we can track ignorable fixed objects in RLC for the different needs. >> >> About viewportConstrainedObjects, I tried two methods: >> 1) Check if the object in FrameView::viewportConstrainedObjects() is actually viewport constrained in ScrollingCoordinator::hasVisibleSlowRepaintFixedObjects() (the previous patch) >> 2) Update FrameView::viewConstrainedObjects() by excluding non-viewport-constrained objects (the current patch) >> >> More information about method 2: in patch of bug 103470 (which needs to exclude another category of elements from viewportConstrainedObjects), skyostil@ suggested to let FrameView::viewportConstrainedObjects() return only the actual viewport constrained objects excluding both out-of-view ones and the ones under transformed container. This needs change of where FrameView's viewportConstrainedObjects are updated. I haven't found a proper place yet. RLC seems not a good place. >> >> Does method 1 look good to you? > > Doesn't the same apply to the FrameView list of viewport-constrained objects then? I'm not sure if I understand your question. However, after looking at how viewportConstrainedObjects are used in FrameView itself, I think we'd better exclude the objects from FrameView's viewportConstrainedObject (method 2) instead of excluding them in ScrollingCoordinator(method 1). I think this would also benefit other platforms to avoid non-viewport-constrained objects from downgrading the performance. Using method 2, if not calling add/removeViewportConstrainedObjects in RLC, we need either a) find better places to call add/removeViewportConstrainedObjects to exclude the non-viewport-constrained objects or b) maintain a set of non-viewport-constrained objects in RLC, and let FrameView exclude the objects from its own viewportConstrainedObjects a) seems better but haven't found a better place than RLC yet.
Xianzhu Wang
Comment 41 2012-12-07 11:12:52 PST
Comment on attachment 178122 [details] Patch (independent with the blocking bugs) View in context: https://bugs.webkit.org/attachment.cgi?id=178122&action=review >>>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:1984 >>>>> + frameView->addViewportConstrainedObject(renderer); >>>> >>>> I don't like the fact that this code is messing with the FrameView's viewportConstrainedObjects. I think you should track whether there are any ignorable Fixed objects in RLC. >>> >>> I think "ignorable" may mean different things. ScrollingCoordinator needs to exclude objects that are not actually viewport constrained in the viewportConstrainedObjects list when determine main thread scrolling. RenderLayer should not paint out-of-view fixed position layer when doing pre-painting. So I'm not sure if we can track ignorable fixed objects in RLC for the different needs. >>> >>> About viewportConstrainedObjects, I tried two methods: >>> 1) Check if the object in FrameView::viewportConstrainedObjects() is actually viewport constrained in ScrollingCoordinator::hasVisibleSlowRepaintFixedObjects() (the previous patch) >>> 2) Update FrameView::viewConstrainedObjects() by excluding non-viewport-constrained objects (the current patch) >>> >>> More information about method 2: in patch of bug 103470 (which needs to exclude another category of elements from viewportConstrainedObjects), skyostil@ suggested to let FrameView::viewportConstrainedObjects() return only the actual viewport constrained objects excluding both out-of-view ones and the ones under transformed container. This needs change of where FrameView's viewportConstrainedObjects are updated. I haven't found a proper place yet. RLC seems not a good place. >>> >>> Does method 1 look good to you? >> >> Doesn't the same apply to the FrameView list of viewport-constrained objects then? > > I'm not sure if I understand your question. However, after looking at how viewportConstrainedObjects are used in FrameView itself, I think we'd better exclude the objects from FrameView's viewportConstrainedObject (method 2) instead of excluding them in ScrollingCoordinator(method 1). I think this would also benefit other platforms to avoid non-viewport-constrained objects from downgrading the performance. > > Using method 2, if not calling add/removeViewportConstrainedObjects in RLC, we need either > a) find better places to call add/removeViewportConstrainedObjects to exclude the non-viewport-constrained objects > or > b) maintain a set of non-viewport-constrained objects in RLC, and let FrameView exclude the objects from its own viewportConstrainedObjects > > a) seems better but haven't found a better place than RLC yet. Sorry, I missed your additional comments which don't show in code review UI. Am reading them.
Xianzhu Wang
Comment 42 2012-12-07 11:58:52 PST
Created attachment 178247 [details] New patch
Xianzhu Wang
Comment 43 2012-12-07 12:04:00 PST
(In reply to comment #39) > (In reply to comment #38) > > (From update of attachment 178122 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=178122&action=review > > > > >> Source/WebCore/rendering/RenderLayer.cpp:3130 > > >> + if (!isComposited() && isStackingContext() && m_renderer->style()->position() == FixedPosition && m_renderer->container() == m_renderer->view()) > > > > > > (isStackingContext() && m_renderer->style()->position() == FixedPosition && m_renderer->container() == m_renderer->view()) should be packaged up into a function that's called from both RLC and this code. > > > > Actually the common part is only (isStackingContext() && m_renderer->style()->position() == FixedPosition). RLC needs to check the container separately as it need to check the NULL condition. > > Also I'm not sure how to name it. Does isFixedPositionAndStackingContext() look good? > > isStackingContextFixedPosition()? > Combined more conditions into a new function RenderLayerCompositor::isFixedPositionCanBeComposited(). > > > > >> Source/WebCore/rendering/RenderLayerCompositor.cpp:1984 > > >> + frameView->addViewportConstrainedObject(renderer); > > > > > > I don't like the fact that this code is messing with the FrameView's viewportConstrainedObjects. I think you should track whether there are any ignorable Fixed objects in RLC. > > > > I think "ignorable" may mean different things. ScrollingCoordinator needs to exclude objects that are not actually viewport constrained in the viewportConstrainedObjects list when determine main thread scrolling. RenderLayer should not paint out-of-view fixed position layer when doing pre-painting. So I'm not sure if we can track ignorable fixed objects in RLC for the different needs. > > Doesn't the same apply to the FrameView list of viewport-constrained objects then? > > > About viewportConstrainedObjects, I tried two methods: > > 1) Check if the object in FrameView::viewportConstrainedObjects() is actually viewport constrained in ScrollingCoordinator::hasVisibleSlowRepaintFixedObjects() (the previous patch) > > When does viewportConstrainedObjects() contain something that is not actually viewport-constrained? Out-of-view fixed things should still be considered viewport-constained, I think. Or, exclude them from FrameView::viewportConstrainedObjects(), but do so from RenderLayer or rendering code. Currently frameView->addViewportConstrainedObject(this); is called by RenderLayerModelObject::styleDidChange(), but at that point you don't know about layout, so you can't compare the bounds with the viewport. > > I think we need a general solution here that works independently of compositing. Ideally it would be smart enough to handle position:-webkit-sticky things that are scrolled out of view too. > > > 2) Update FrameView::viewConstrainedObjects() by excluding non-viewport-constrained objects (the current patch) > > But this patch does it via compositing code, which is wrong. > > So the main question here is should FrameView::viewportConstrainedObjects() become FrameView::visibleViewportConstrainedObjects(), and, if so, how do we maintain this list correctly. If not, we need to do visibility/transformed container checks elsewhere. Kept viewConstrainedObjects unchanged. Now checks visibility in RenderLayer::paintLayer and ScrollingCoordinator::hasVisibleSlowRepaintFixedObjects(). Will also check transformed container in ScrollingCoordinator in bug 103470.
Build Bot
Comment 44 2012-12-07 12:30:49 PST
Xianzhu Wang
Comment 45 2012-12-07 13:32:54 PST
Created attachment 178264 [details] Fix mac build break
Xianzhu Wang
Comment 46 2012-12-10 15:05:34 PST
Simon, could you please take another look?
Xianzhu Wang
Comment 47 2012-12-10 15:29:47 PST
I think the current patch still has some problems. Will upload a new patch for review soon.
Xianzhu Wang
Comment 48 2012-12-10 21:50:20 PST
Created attachment 178710 [details] Let RLC manage reasons of fixed pos layer not composited
Xianzhu Wang
Comment 49 2012-12-10 21:54:49 PST
Simon, the last patch is now ready for review. Could you please review it? Thanks.
Xianzhu Wang
Comment 50 2012-12-11 10:26:43 PST
Comment on attachment 178710 [details] Let RLC manage reasons of fixed pos layer not composited Sorry, found a problem again. Fixing.
Xianzhu Wang
Comment 51 2012-12-11 10:32:32 PST
Created attachment 178834 [details] Let RLC manage reasons of fixedpos layer not composited
James Robinson
Comment 52 2012-12-11 10:57:33 PST
The test seems the same - what was the issue with the last iteration, what fixed it, and what test covers it?
Xianzhu Wang
Comment 53 2012-12-11 11:01:55 PST
(In reply to comment #52) > The test seems the same - what was the issue with the last iteration, what fixed it, and what test covers it? The issue was that m_fixedPositionLayerNotCompositedMap.clear() is misplaced. Will update the test to cover that.
Xianzhu Wang
Comment 54 2012-12-11 11:13:51 PST
(In reply to comment #52) > The test seems the same - what was the issue with the last iteration, what fixed it, and what test covers it? The problems mentioned in #47 were about code structure. The method RenderLayer to determine if it should be painted was tricky, and it seems not good to expose unclear RenderLayerCompositor::isFixedPositionCanBeComposited().
Xianzhu Wang
Comment 55 2012-12-11 11:16:37 PST
Created attachment 178840 [details] Update test to cover issue of a previous patch
Eric Seidel (no email)
Comment 56 2012-12-11 13:28:51 PST
Comment on attachment 178840 [details] Update test to cover issue of a previous patch View in context: https://bugs.webkit.org/attachment.cgi?id=178840&action=review > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:402 > +String ScrollingCoordinator::mainThreadScrollingReasonsAsText(MainThreadScrollingReasons reasons) This refactoring appears to be largely unrelated to the rest of the patch. If you are feeling blocked on this patch, you might consider doing this code-motion in a separate patch. Just moving where this string building happens (and removing the trailing comma and updating the one affected layout test) is a trivial patch which even I (or other non-layer-saavy folks) could review in a separate bug.
James Robinson
Comment 57 2012-12-11 14:54:56 PST
Comment on attachment 178840 [details] Update test to cover issue of a previous patch View in context: https://bugs.webkit.org/attachment.cgi?id=178840&action=review > Source/WebCore/rendering/RenderLayer.cpp:3152 > + } else if (compositor()->fixedPositionLayerNotCompositedReason(this) == RenderLayerCompositor::LayerBoundsOutOfView) { > + // Don't paint out-of-view fixed position layers (when doing prepainting) because they will never be visible > + // unless their position or viewport size is changed. > + return; Could you put this in a separate patch? It's a different thing and not really directly related to the ScrollingCoordinator behavior change, although it's using the same infrastructure.
Xianzhu Wang
Comment 58 2012-12-11 15:01:11 PST
Comment on attachment 178840 [details] Update test to cover issue of a previous patch View in context: https://bugs.webkit.org/attachment.cgi?id=178840&action=review >> Source/WebCore/rendering/RenderLayer.cpp:3152 >> + return; > > Could you put this in a separate patch? It's a different thing and not really directly related to the ScrollingCoordinator behavior change, although it's using the same infrastructure. Filed bug 104724
Xianzhu Wang
Comment 59 2012-12-11 17:20:54 PST
Created attachment 178928 [details] Smaller patch
James Robinson
Comment 60 2012-12-11 17:34:29 PST
Comment on attachment 178928 [details] Smaller patch We discussed the placement of the visibility checks and it seems like since it depends on composited bounds doing it in RenderLayerCompositor and querying the result later is the right thing to do. I think we could tweak exactly how that state is maintained and carried forward, but this seems like a good start. R=me
Xianzhu Wang
Comment 61 2012-12-11 17:37:09 PST
Thanks James. Will rebase after the patch of bug 104303 (in CQ now) landed to resolve the conflict.
Xianzhu Wang
Comment 62 2012-12-11 18:21:15 PST
Created attachment 178940 [details] For landing
WebKit Review Bot
Comment 63 2012-12-11 21:15:29 PST
Comment on attachment 178940 [details] For landing Clearing flags on attachment: 178940 Committed r137409: <http://trac.webkit.org/changeset/137409>
WebKit Review Bot
Comment 64 2012-12-11 21:15:38 PST
All reviewed patches have been landed. Closing bug.
Simon Fraser (smfr)
Comment 65 2012-12-15 14:45:50 PST
The DescendantOfTransformedElement enum added here is not accurate. The container for a Fixed element may be another Fixed element. It should be renamed.
Simon Fraser (smfr)
Comment 66 2012-12-15 14:47:04 PST
I also dislike how requiresCompositingForPosition() has an out param named FixedPositionLayerNotCompositedReason. Who says only Fixed things get layers?
Xianzhu Wang
Comment 67 2012-12-17 12:06:09 PST
(In reply to comment #66) > I also dislike how requiresCompositingForPosition() has an out param named FixedPositionLayerNotCompositedReason. Who says only Fixed things get layers? For now RLC seems to assume requiresCompositingForPosition() is for fixed things only: In RenderLayerCompositor::reasonForCompositing(): if (requiresCompositingForPosition(renderer, layer)) return "position: fixed"; So how about renaming requiresCompositingForPosition to requiresCompositingForFixedPosition? However FixedPositionLayerNotCompositedReason is also an output parameter in needsToBeComposited() and requiresCompositingLayer(), so would it be better to make the parameter more generic? For example, make the value a bit field enum (or struct) and any requiresCompositingForXXX() function can add bits to it, so FixedPositionLayerNotCompositedReason might be LayerCompositingStatusFlags. An related idea is to record the info in RenderLayer with a bit field instead of in the map in RLC. Just tested on 64bit system, sizeof(RenderLayer)=288 and there are at least 16bit for additional bit fields without increasing the size. On 32bit system, sizeof(RenderLayer)=204 and adding 1 bit will increase the size to 208.
Simon Fraser (smfr)
Comment 68 2012-12-17 14:51:40 PST
(In reply to comment #67) > (In reply to comment #66) > > I also dislike how requiresCompositingForPosition() has an out param named FixedPositionLayerNotCompositedReason. Who says only Fixed things get layers? > > For now RLC seems to assume requiresCompositingForPosition() is for fixed things only: > In RenderLayerCompositor::reasonForCompositing(): > if (requiresCompositingForPosition(renderer, layer)) > return "position: fixed"; > > So how about renaming requiresCompositingForPosition to requiresCompositingForFixedPosition? > > However FixedPositionLayerNotCompositedReason is also an output parameter in needsToBeComposited() and requiresCompositingLayer(), so would it be better to make the parameter more generic? For example, make the value a bit field enum (or struct) and any requiresCompositingForXXX() function can add bits to it, so FixedPositionLayerNotCompositedReason might be LayerCompositingStatusFlags. > > An related idea is to record the info in RenderLayer with a bit field instead of in the map in RLC. Just tested on 64bit system, sizeof(RenderLayer)=288 and there are at least 16bit for additional bit fields without increasing the size. On 32bit system, sizeof(RenderLayer)=204 and adding 1 bit will increase the size to 208. The reason I made the comment is because I'm adding support for position:sticky here, so it's "viewport-constrained" things now.
Xianzhu Wang
Comment 69 2012-12-18 14:41:47 PST
(In reply to comment #68) > The reason I made the comment is because I'm adding support for position:sticky here, so it's "viewport-constrained" things now. Would you like to change the names or refactor in your position:sticky change, or me to change of the names? I think at least I should correct the name DescendantOfTransformedElement and related previously existing comments. I'd do that soon.
Note You need to log in before you can comment on or make changes to this bug.