Summary: | Sometimes RenderLayer::updateNeedsCompositedScrolling not called | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Xianzhu Wang <wangxianzhu> | ||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Xianzhu Wang <wangxianzhu> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | aelias, eric, jamesr, klobag, ojan.autocc, simon.fraser, skyostil, tonikitoo, vollick, webkit.review.bot | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Xianzhu Wang
2013-01-07 15:52:30 PST
Created attachment 181612 [details]
Wrong version, please ignore.
Created attachment 181614 [details]
Patch
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? (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. Found another case: if a container becomes scrollable dynamically, sometimes the composited scrolling status is not updated. Created attachment 181746 [details]
Patch
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 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. (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 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? (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. Created attachment 181951 [details]
Patch
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. Created attachment 181953 [details]
Patch
James, Simon and Ian, what do you think of the last patch? 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 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. Created attachment 182161 [details]
Patch
Kindly ping reviewers... Created attachment 182444 [details]
Slightly simplified
Kindly ping reviewers... (In reply to comment #21) > Kindly ping reviewers... James and Simon? 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. Created attachment 182638 [details]
Patch
Comment on attachment 182638 [details]
Patch
PTAL. I'm not sure about the style.
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. Created attachment 182639 [details]
for landing
Comment on attachment 182639 [details] for landing Clearing flags on attachment: 182639 Committed r139691: <http://trac.webkit.org/changeset/139691> All reviewed patches have been landed. Closing bug. *** Bug 106915 has been marked as a duplicate of this bug. *** |