Summary: | ScrollingCoordinator::hasVisibleSlowRepaintFixedObject() should exclude out-of-view fixed position elements | ||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Xianzhu Wang <wangxianzhu> | ||||||||||||||||||||||||||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | 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
Xianzhu Wang
2012-11-16 11:40:58 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? (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. (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. Created attachment 175100 [details]
Patch
Comment on attachment 175100 [details] Patch Attachment 175100 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14908379 Comment on attachment 175100 [details] Patch Attachment 175100 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14912434 Comment on attachment 175100 [details] Patch Attachment 175100 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14894416 Created attachment 175244 [details]
Add symbol
Created attachment 175255 [details]
For review
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. Created attachment 176104 [details]
avoid new field
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 Created attachment 176320 [details]
Fix layout tests
Created attachment 176355 [details]
Patch
Simon, could you take another look? Thanks. 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 Created attachment 176534 [details]
Fix layout tests on Mac
Simon, what do you think about this change? 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 (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. (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. Created attachment 177356 [details]
Rebase and adjust indent
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. Created attachment 177506 [details]
Rebase again
Comment on attachment 177506 [details] Rebase again Attachment 177506 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15119889 Created attachment 177534 [details]
Fix win build
Comment on attachment 177534 [details] Fix win build Attachment 177534 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15154083 Created attachment 177603 [details]
Try WebKit2.def.in change
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 Created attachment 177767 [details]
Yet another try of WebKit2.def.in change
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 Created attachment 177789 [details] Workaround Windows linker issue (bug 104136) @Simon, would you please review the change, or suggest another reviewer? Thanks! To ease review, I will split this change into multiple ones. Created attachment 178122 [details]
Patch (independent with the blocking bugs)
(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. 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? 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. (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. 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. 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. Created attachment 178247 [details]
New patch
(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. Comment on attachment 178247 [details] New patch Attachment 178247 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15189388 Created attachment 178264 [details]
Fix mac build break
Simon, could you please take another look? I think the current patch still has some problems. Will upload a new patch for review soon. Created attachment 178710 [details]
Let RLC manage reasons of fixed pos layer not composited
Simon, the last patch is now ready for review. Could you please review it? Thanks. Comment on attachment 178710 [details]
Let RLC manage reasons of fixed pos layer not composited
Sorry, found a problem again. Fixing.
Created attachment 178834 [details]
Let RLC manage reasons of fixedpos layer not composited
The test seems the same - what was the issue with the last iteration, what fixed it, and what test covers it? (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. (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(). Created attachment 178840 [details]
Update test to cover issue of a previous patch
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. 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. 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 Created attachment 178928 [details]
Smaller patch
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
Thanks James. Will rebase after the patch of bug 104303 (in CQ now) landed to resolve the conflict. Created attachment 178940 [details]
For landing
Comment on attachment 178940 [details] For landing Clearing flags on attachment: 178940 Committed r137409: <http://trac.webkit.org/changeset/137409> All reviewed patches have been landed. Closing bug. The DescendantOfTransformedElement enum added here is not accurate. The container for a Fixed element may be another Fixed element. It should be renamed. I also dislike how requiresCompositingForPosition() has an out param named FixedPositionLayerNotCompositedReason. Who says only Fixed things get layers? (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. (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. (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. |