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

Description Xianzhu Wang 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.
Comment 1 Beth Dakin 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?
Comment 2 Xianzhu Wang 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.
Comment 3 Beth Dakin 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.
Comment 4 Xianzhu Wang 2012-11-19 17:47:27 PST
Created attachment 175100 [details]
Patch
Comment 5 kov's GTK+ EWS bot 2012-11-19 18:16:38 PST
Comment on attachment 175100 [details]
Patch

Attachment 175100 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14908379
Comment 6 Build Bot 2012-11-19 18:47:28 PST
Comment on attachment 175100 [details]
Patch

Attachment 175100 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14912434
Comment 7 Build Bot 2012-11-19 19:09:18 PST
Comment on attachment 175100 [details]
Patch

Attachment 175100 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14894416
Comment 8 Xianzhu Wang 2012-11-20 10:41:35 PST
Created attachment 175244 [details]
Add symbol
Comment 9 Xianzhu Wang 2012-11-20 11:57:23 PST
Created attachment 175255 [details]
For review
Comment 10 Simon Fraser (smfr) 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.
Comment 11 Xianzhu Wang 2012-11-26 16:12:18 PST
Created attachment 176104 [details]
avoid new field
Comment 12 WebKit Review Bot 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
Comment 13 Xianzhu Wang 2012-11-27 12:13:14 PST
Created attachment 176320 [details]
Fix layout tests
Comment 14 Xianzhu Wang 2012-11-27 16:07:12 PST
Created attachment 176355 [details]
Patch
Comment 15 Xianzhu Wang 2012-11-27 16:43:06 PST
Simon, could you take another look? Thanks.
Comment 16 Build Bot 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
Comment 17 Xianzhu Wang 2012-11-28 11:44:40 PST
Created attachment 176534 [details]
Fix layout tests on Mac
Comment 18 Xianzhu Wang 2012-11-30 09:23:21 PST
Simon, what do you think about this change?
Comment 19 Adam Barth 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
Comment 20 Xianzhu Wang 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.
Comment 21 Xianzhu Wang 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.
Comment 22 Xianzhu Wang 2012-12-03 15:46:32 PST
Created attachment 177356 [details]
Rebase and adjust indent
Comment 23 Xianzhu Wang 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.
Comment 24 Xianzhu Wang 2012-12-04 10:15:33 PST
Created attachment 177506 [details]
Rebase again
Comment 25 Build Bot 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
Comment 26 Xianzhu Wang 2012-12-04 13:19:18 PST
Created attachment 177534 [details]
Fix win build
Comment 27 Build Bot 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
Comment 28 Xianzhu Wang 2012-12-04 16:41:26 PST
Created attachment 177603 [details]
Try WebKit2.def.in change
Comment 29 Build Bot 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
Comment 30 Xianzhu Wang 2012-12-05 09:03:45 PST
Created attachment 177767 [details]
Yet another try of WebKit2.def.in change
Comment 31 Build Bot 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
Comment 32 Xianzhu Wang 2012-12-05 10:30:20 PST
Created attachment 177789 [details]
Workaround Windows linker issue (bug 104136)
Comment 33 Xianzhu Wang 2012-12-05 12:11:51 PST
@Simon, would you please review the change, or suggest another reviewer? Thanks!
Comment 34 Xianzhu Wang 2012-12-06 10:56:41 PST
To ease review, I will split this change into multiple ones.
Comment 35 Xianzhu Wang 2012-12-06 17:45:56 PST
Created attachment 178122 [details]
Patch (independent with the blocking bugs)
Comment 36 Xianzhu Wang 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.
Comment 37 Simon Fraser (smfr) 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?
Comment 38 Xianzhu Wang 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.
Comment 39 Simon Fraser (smfr) 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.
Comment 40 Xianzhu Wang 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.
Comment 41 Xianzhu Wang 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.
Comment 42 Xianzhu Wang 2012-12-07 11:58:52 PST
Created attachment 178247 [details]
New patch
Comment 43 Xianzhu Wang 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.
Comment 44 Build Bot 2012-12-07 12:30:49 PST
Comment on attachment 178247 [details]
New patch

Attachment 178247 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15189388
Comment 45 Xianzhu Wang 2012-12-07 13:32:54 PST
Created attachment 178264 [details]
Fix mac build break
Comment 46 Xianzhu Wang 2012-12-10 15:05:34 PST
Simon, could you please take another look?
Comment 47 Xianzhu Wang 2012-12-10 15:29:47 PST
I think the current patch still has some problems. Will upload a new patch for review soon.
Comment 48 Xianzhu Wang 2012-12-10 21:50:20 PST
Created attachment 178710 [details]
Let RLC manage reasons of fixed pos layer not composited
Comment 49 Xianzhu Wang 2012-12-10 21:54:49 PST
Simon, the last patch is now ready for review. Could you please review it? Thanks.
Comment 50 Xianzhu Wang 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.
Comment 51 Xianzhu Wang 2012-12-11 10:32:32 PST
Created attachment 178834 [details]
Let RLC manage reasons of fixedpos layer not composited
Comment 52 James Robinson 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?
Comment 53 Xianzhu Wang 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.
Comment 54 Xianzhu Wang 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().
Comment 55 Xianzhu Wang 2012-12-11 11:16:37 PST
Created attachment 178840 [details]
Update test to cover issue of a previous patch
Comment 56 Eric Seidel (no email) 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.
Comment 57 James Robinson 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.
Comment 58 Xianzhu Wang 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
Comment 59 Xianzhu Wang 2012-12-11 17:20:54 PST
Created attachment 178928 [details]
Smaller patch
Comment 60 James Robinson 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
Comment 61 Xianzhu Wang 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.
Comment 62 Xianzhu Wang 2012-12-11 18:21:15 PST
Created attachment 178940 [details]
For landing
Comment 63 WebKit Review Bot 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>
Comment 64 WebKit Review Bot 2012-12-11 21:15:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 65 Simon Fraser (smfr) 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.
Comment 66 Simon Fraser (smfr) 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?
Comment 67 Xianzhu Wang 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.
Comment 68 Simon Fraser (smfr) 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.
Comment 69 Xianzhu Wang 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.