ScrollingCoordinator::hasVisibleSlowRepaintFixedObject() should exclude out-of-view fixed position elements

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Xianzhu Wang]

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.

[Beth Dakin]

(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]

(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]

(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]

Created attachment 175100 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 175100 [details]
Patch

Attachment 175100 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14908379

[Build Bot]

Comment on attachment 175100 [details]
Patch

Attachment 175100 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14912434

[Build Bot]

Comment on attachment 175100 [details]
Patch

Attachment 175100 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14894416

[Xianzhu Wang]

Created attachment 175244 [details]
Add symbol

[Xianzhu Wang]

Created attachment 175255 [details]
For review

[Simon Fraser (smfr)]

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]

Created attachment 176104 [details]
avoid new field

[WebKit Review Bot]

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]

Created attachment 176320 [details]
Fix layout tests

[Xianzhu Wang]

Created attachment 176355 [details]
Patch

[Xianzhu Wang]

Simon, could you take another look? Thanks.

[Build Bot]

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]

Created attachment 176534 [details]
Fix layout tests on Mac

[Xianzhu Wang]

Simon, what do you think about this change?

[Adam Barth]

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]

(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]

(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]

Created attachment 177356 [details]
Rebase and adjust indent

[Xianzhu Wang]

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]

Created attachment 177506 [details]
Rebase again

[Build Bot]

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]

Created attachment 177534 [details]
Fix win build

[Build Bot]

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]

Created attachment 177603 [details]
Try WebKit2.def.in change

[Build Bot]

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]

Created attachment 177767 [details]
Yet another try of WebKit2.def.in change

[Build Bot]

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]

Created attachment 177789 [details]
Workaround Windows linker issue (bug 104136)

[Xianzhu Wang]

@Simon, would you please review the change, or suggest another reviewer? Thanks!

[Xianzhu Wang]

To ease review, I will split this change into multiple ones.

[Xianzhu Wang]

Created attachment 178122 [details]
Patch (independent with the blocking bugs)

[Xianzhu Wang]

(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 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 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)]

(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 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 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]

Created attachment 178247 [details]
New patch

[Xianzhu Wang]

(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 on attachment 178247 [details]
New patch

Attachment 178247 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15189388

[Xianzhu Wang]

Created attachment 178264 [details]
Fix mac build break

[Xianzhu Wang]

Simon, could you please take another look?

[Xianzhu Wang]

I think the current patch still has some problems. Will upload a new patch for review soon.

[Xianzhu Wang]

Created attachment 178710 [details]
Let RLC manage reasons of fixed pos layer not composited

[Xianzhu Wang]

Simon, the last patch is now ready for review. Could you please review it? Thanks.

[Xianzhu Wang]

Comment on attachment 178710 [details]
Let RLC manage reasons of fixed pos layer not composited

Sorry, found a problem again. Fixing.

[Xianzhu Wang]

Created attachment 178834 [details]
Let RLC manage reasons of fixedpos layer not composited

[James Robinson]

The test seems the same - what was the issue with the last iteration, what fixed it, and what test covers it?

[Xianzhu Wang]

(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]

(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]

Created attachment 178840 [details]
Update test to cover issue of a previous patch

[Eric Seidel (no email)]

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 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 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]

Created attachment 178928 [details]
Smaller patch

[James Robinson]

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]

Thanks James.
Will rebase after the patch of bug 104303 (in CQ now) landed to resolve the conflict.

[Xianzhu Wang]

Created attachment 178940 [details]
For landing

[WebKit Review Bot]

Comment on attachment 178940 [details]
For landing

Clearing flags on attachment: 178940

Committed r137409: <http://trac.webkit.org/changeset/137409>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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)]

I also dislike how requiresCompositingForPosition() has an out param named FixedPositionLayerNotCompositedReason. Who says only Fixed things get layers?

[Xianzhu Wang]

(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)]

(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]

(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.