Bug 106271 - Sometimes RenderLayer::updateNeedsCompositedScrolling not called
Summary: Sometimes RenderLayer::updateNeedsCompositedScrolling not called
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xianzhu Wang
URL:
Keywords:
: 106915 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-01-07 15:52 PST by Xianzhu Wang
Modified: 2013-01-15 11:30 PST (History)
10 users (show)

See Also:


Attachments
Wrong version, please ignore. (4.80 KB, patch)
2013-01-07 17:45 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Patch (4.66 KB, patch)
2013-01-07 17:53 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Patch (5.49 KB, patch)
2013-01-08 13:30 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Patch (6.88 KB, patch)
2013-01-09 11:01 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Patch (7.19 KB, patch)
2013-01-09 11:09 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Patch (7.19 KB, patch)
2013-01-10 09:53 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Slightly simplified (7.72 KB, patch)
2013-01-11 16:31 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Patch (9.69 KB, patch)
2013-01-14 15:33 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
for landing (9.84 KB, patch)
2013-01-14 15:42 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xianzhu Wang 2013-01-07 15:52:30 PST
Suppose a RenderLayer meets the requirement of composited scrolling, updateNeedsCompositedScrolling() is supposed to be called when m_hasOutOfFlowPositionedDescendant changes. However, because the initial value of m_hasOutOfFlowPositionedDescendant is false, it won't change for the RenderLayer never has out-of-flow-positioned descendant; if there is no other events triggering updateNeedsCompositedScrolling(), m_needsCompositedScrolling will always be false, causing slow scrolling of the layer.
Comment 1 Xianzhu Wang 2013-01-07 17:45:03 PST
Created attachment 181612 [details]
Wrong version, please ignore.
Comment 2 Xianzhu Wang 2013-01-07 17:53:10 PST
Created attachment 181614 [details]
Patch
Comment 3 James Robinson 2013-01-07 17:58:30 PST
Comment on attachment 181614 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181614&action=review

> LayoutTests/ChangeLog:8
> +        Actually the test didn't reproduce the issue. However the test can ensure the composited scrolling status is correct and not causing slow scrolling.

Do you know why this is?
Comment 4 Xianzhu Wang 2013-01-07 19:37:27 PST
(In reply to comment #3)
> (From update of attachment 181614 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181614&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        Actually the test didn't reproduce the issue. However the test can ensure the composited scrolling status is correct and not causing slow scrolling.
> 
> Do you know why this is?

Not yet. I could not reproduce it with a manual test. Just did a debug with the test case and found that updateNeedsCompositedScrolling() is always called from styleChanged() during page loading. The issue happens with gmail.com. Will try if dynamically created elements cause it.
Comment 5 Xianzhu Wang 2013-01-08 12:45:53 PST
Found another case: if a container becomes scrollable dynamically, sometimes the composited scrolling status is not updated.
Comment 6 Xianzhu Wang 2013-01-08 13:30:16 PST
Created attachment 181746 [details]
Patch
Comment 7 James Robinson 2013-01-08 14:21:49 PST
Comment on attachment 181746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181746&action=review

> Source/WebCore/rendering/RenderLayer.cpp:150
> +    , m_hasOutOfFlowPositionedDescendant(true) // Initially true to be consistent with the initial value of m_needsCompositedScrolling.

why would the initial value of this matter if the bit is initially dirty?  No code should be looking at the value of m_hasOutOfFlowPositionedDescendant while m_hasOutOfFlowPositionedDescendantDirty is true, right?
Comment 8 Xianzhu Wang 2013-01-08 14:33:27 PST
Comment on attachment 181746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181746&action=review

>> Source/WebCore/rendering/RenderLayer.cpp:150
>> +    , m_hasOutOfFlowPositionedDescendant(true) // Initially true to be consistent with the initial value of m_needsCompositedScrolling.
> 
> why would the initial value of this matter if the bit is initially dirty?  No code should be looking at the value of m_hasOutOfFlowPositionedDescendant while m_hasOutOfFlowPositionedDescendantDirty is true, right?

In RenderLayer::updateDescendantDependentFlags(), the oldHasOutOfFlowPositionedDescendant is checked against the new value to determine if updateNeedsCompositedScrolling should be called:
        if (oldHasOutOfFlowPositionedDescendant != m_hasOutOfFlowPositionedDescendant)
            updateNeedsCompositedScrolling();

Here oldHasOutOfFlowPositionedDescendant may be the dirty value before the update.
Comment 9 James Robinson 2013-01-08 14:36:54 PST
(In reply to comment #8)
> (From update of attachment 181746 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181746&action=review
> 
> >> Source/WebCore/rendering/RenderLayer.cpp:150
> >> +    , m_hasOutOfFlowPositionedDescendant(true) // Initially true to be consistent with the initial value of m_needsCompositedScrolling.
> > 
> > why would the initial value of this matter if the bit is initially dirty?  No code should be looking at the value of m_hasOutOfFlowPositionedDescendant while m_hasOutOfFlowPositionedDescendantDirty is true, right?
> 
> In RenderLayer::updateDescendantDependentFlags(), the oldHasOutOfFlowPositionedDescendant is checked against the new value to determine if updateNeedsCompositedScrolling should be called:
>         if (oldHasOutOfFlowPositionedDescendant != m_hasOutOfFlowPositionedDescendant)
>             updateNeedsCompositedScrolling();
> 
> Here oldHasOutOfFlowPositionedDescendant may be the dirty value before the update.

Seems like the better fix would be to call updateNeedsCompositedScrolling() whenever m_hasOutOfFlowPositionedDescendantDirty transitions from true->false.
Comment 10 Xianzhu Wang 2013-01-08 14:44:55 PST
Comment on attachment 181746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181746&action=review

>>>> Source/WebCore/rendering/RenderLayer.cpp:150
>>>> +    , m_hasOutOfFlowPositionedDescendant(true) // Initially true to be consistent with the initial value of m_needsCompositedScrolling.
>>> 
>>> why would the initial value of this matter if the bit is initially dirty?  No code should be looking at the value of m_hasOutOfFlowPositionedDescendant while m_hasOutOfFlowPositionedDescendantDirty is true, right?
>> 
>> In RenderLayer::updateDescendantDependentFlags(), the oldHasOutOfFlowPositionedDescendant is checked against the new value to determine if updateNeedsCompositedScrolling should be called:
>>         if (oldHasOutOfFlowPositionedDescendant != m_hasOutOfFlowPositionedDescendant)
>>             updateNeedsCompositedScrolling();
>> 
>> Here oldHasOutOfFlowPositionedDescendant may be the dirty value before the update.
> 
> Seems like the better fix would be to call updateNeedsCompositedScrolling() whenever m_hasOutOfFlowPositionedDescendantDirty transitions from true->false.

Wouldn't that cause unnecessary updates? e.g. m_hasOutOfFlowPositionedDescendantDirty changes from false->true then ->false while m_hasOutOfFlowPositionedDescendant hasn't changed.

Actually I even consider unconditional updateNeedsCompositedScrolling() because it is not costly :)

Ian, what's your opinion?
Comment 11 vollick 2013-01-08 19:52:07 PST
(In reply to comment #10)
> (From update of attachment 181746 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181746&action=review
> 
> >>>> Source/WebCore/rendering/RenderLayer.cpp:150
> >>>> +    , m_hasOutOfFlowPositionedDescendant(true) // Initially true to be consistent with the initial value of m_needsCompositedScrolling.
> >>> 
> >>> why would the initial value of this matter if the bit is initially dirty?  No code should be looking at the value of m_hasOutOfFlowPositionedDescendant while m_hasOutOfFlowPositionedDescendantDirty is true, right?
> >> 
> >> In RenderLayer::updateDescendantDependentFlags(), the oldHasOutOfFlowPositionedDescendant is checked against the new value to determine if updateNeedsCompositedScrolling should be called:
> >>         if (oldHasOutOfFlowPositionedDescendant != m_hasOutOfFlowPositionedDescendant)
> >>             updateNeedsCompositedScrolling();
> >> 
> >> Here oldHasOutOfFlowPositionedDescendant may be the dirty value before the update.
> > 
> > Seems like the better fix would be to call updateNeedsCompositedScrolling() whenever m_hasOutOfFlowPositionedDescendantDirty transitions from true->false.
> 
> Wouldn't that cause unnecessary updates? e.g. m_hasOutOfFlowPositionedDescendantDirty changes from false->true then ->false while m_hasOutOfFlowPositionedDescendant hasn't changed.
> 
> Actually I even consider unconditional updateNeedsCompositedScrolling() because it is not costly :)
> 
> Ian, what's your opinion?

I'm not against calling updateNeedsCompositedScrolling more often here (as you mentioned, it's cheap), and I like James's suggestion of doing the update every time we 'undirty' m_hasOutOfFlowPositionedDescendant.
Comment 12 Xianzhu Wang 2013-01-09 11:01:00 PST
Created attachment 181951 [details]
Patch
Comment 13 Simon Fraser (smfr) 2013-01-09 11:02:25 PST
Comment on attachment 181951 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181951&action=review

> Source/WebCore/ChangeLog:8
> +        Sometimes RenderLayer::updateNeedsCompositedScrolling is not called for a RenderLayer without out-of-flow-positioned descendant
> +        https://bugs.webkit.org/show_bug.cgi?id=106271
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: compositing/overflow/dynamic-composited-scrolling-status.html

Please describe the bug, and how you fixed it here.
Comment 14 Xianzhu Wang 2013-01-09 11:09:50 PST
Created attachment 181953 [details]
Patch
Comment 15 Xianzhu Wang 2013-01-10 09:33:10 PST
James, Simon and Ian, what do you think of the last patch?
Comment 16 vollick 2013-01-10 09:45:07 PST
Comment on attachment 181953 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181953&action=review

Just one nit.

> Source/WebCore/rendering/RenderLayer.h:477
> +    // FIXME: We should ASSERT(!m_hasOutOfFlowPositionedDescendant); here but we may hit the same bugs as visible content above.

m_hasOutOfFlowPositionedDescendantDirty?
Comment 17 Xianzhu Wang 2013-01-10 09:52:44 PST
Comment on attachment 181953 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181953&action=review

>> Source/WebCore/rendering/RenderLayer.h:477
>> +    // FIXME: We should ASSERT(!m_hasOutOfFlowPositionedDescendant); here but we may hit the same bugs as visible content above.
> 
> m_hasOutOfFlowPositionedDescendantDirty?

Done.
Comment 18 Xianzhu Wang 2013-01-10 09:53:18 PST
Created attachment 182161 [details]
Patch
Comment 19 Xianzhu Wang 2013-01-11 09:54:49 PST
Kindly ping reviewers...
Comment 20 Xianzhu Wang 2013-01-11 16:31:01 PST
Created attachment 182444 [details]
Slightly simplified
Comment 21 Xianzhu Wang 2013-01-13 19:04:18 PST
Kindly ping reviewers...
Comment 22 Xianzhu Wang 2013-01-14 13:53:47 PST
(In reply to comment #21)
> Kindly ping reviewers...

James and Simon?
Comment 23 Simon Fraser (smfr) 2013-01-14 14:10:13 PST
Comment on attachment 182444 [details]
Slightly simplified

View in context: https://bugs.webkit.org/attachment.cgi?id=182444&action=review

> Source/WebCore/rendering/RenderLayer.cpp:5547
> +    if (shouldBeScrollableArea != frameView->containsScrollableArea(this)) {
> +        if (shouldBeScrollableArea)
> +            frameView->addScrollableArea(this);
> +        else
> +            frameView->removeScrollableArea(this);

This is two hash lookups in either case. Would better if you could reduce it to one.
Comment 24 Xianzhu Wang 2013-01-14 15:33:21 PST
Created attachment 182638 [details]
Patch
Comment 25 Xianzhu Wang 2013-01-14 15:34:17 PST
Comment on attachment 182638 [details]
Patch

PTAL. I'm not sure about the style.
Comment 26 Simon Fraser (smfr) 2013-01-14 15:34:40 PST
Comment on attachment 182638 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182638&action=review

> Source/WebCore/page/FrameView.h:350
> +    bool addScrollableArea(ScrollableArea*);
> +    bool removeScrollableArea(ScrollableArea*);

Please add a comment saying what the return values mean.
Comment 27 Xianzhu Wang 2013-01-14 15:42:50 PST
Created attachment 182639 [details]
for landing
Comment 28 WebKit Review Bot 2013-01-14 17:15:11 PST
Comment on attachment 182639 [details]
for landing

Clearing flags on attachment: 182639

Committed r139691: <http://trac.webkit.org/changeset/139691>
Comment 29 WebKit Review Bot 2013-01-14 17:15:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 30 Sami Kyöstilä 2013-01-15 11:13:03 PST
*** Bug 106915 has been marked as a duplicate of this bug. ***