Only update needs composited scrolling when necessary.

Only update needs composited scrolling when necessary.

This CL is an update to Glenn's patch https://bugs.webkit.org/show_bug.cgi?id=109966.

As I made fixes to the composite patch, I made pretty significant changes to
the 'early-out' code. Please let me know what you think.

R=hartmanng@chromium.org,shawnsingh@chromium.org

BUG=177714

[vollick, 2013-03-30 18:38:06]

I am splitting this composite patch into two pieces
(https://codereview.appspot.com/7759045/). This is the first of the two pieces.
The second piece is here (https://codereview.appspot.com/8200043/).

Glenn, this piece is just my modifications to your early out patch. Please let
me know what you think.

[shawnsingh, 2013-04-01 23:12:50]

Round 1 of reviewing.  I just wanted to get these comments out there sooner,
while I take more time to look more closely at the dirty-bit logic

https://codereview.appspot.com/8166047/diff/1/Source/WebCore/dom/Document.cpp
File Source/WebCore/dom/Document.cpp (right):

https://codereview.appspot.com/8166047/diff/1/Source/WebCore/dom/Document.cpp...
Source/WebCore/dom/Document.cpp:138: #include "RenderLayer.h"
Maybe we should avoid adding this dependency.  It makes sense to me that
conceptually the Document needs to know about RenderView and RenderObject.
(though maybe even that can be reduced in the long-term, too).   Would it make
sense to move ScopedCompositedScrollingUpdater into its own file, even if it
still lives in rendering/ ?

https://codereview.appspot.com/8166047/diff/1/Source/WebCore/rendering/Render...
File Source/WebCore/rendering/RenderLayer.cpp (right):

https://codereview.appspot.com/8166047/diff/1/Source/WebCore/rendering/Render...
Source/WebCore/rendering/RenderLayer.cpp:147: static int gNumUpdaterInstances =
0;
At the very least, shouldn't we make this a private static member of
ScopedCompositedScrollingUpdater instead of global?  That would protect us from
unintended usage.  Perhaps we should call it a "semaphore", too.

A few more concerns, though:
(1) Are we able to guarantee we won't run into any static initialization races
(e.g. if anyone ends up initializing some RenderLayers or the dom/Document 
object statically)?

(2) And worse, what happens if we have multiple tabs on a single process?  Is
there some sort of "context" that isolates this data for a single RenderView
that i'm not aware of?

https://codereview.appspot.com/8166047/diff/1/Source/WebCore/rendering/Render...
Source/WebCore/rendering/RenderLayer.cpp:2095:
ScopedCompositedScrollingUpdater(this, UpdateNeedsCompositedScrolling);
I feel like this struct might be mangling two different roles, which obfuscates
the code a little:  (1) the role of a semaphore to track when it's reasonable to
actually invoke an update, and (2) the scheduling to perform an update.

Instead of this public constructor, I think what we want is a private schedule()
function on the ScopedCompositedScrollingUpdater.  And the associated renamings
that design clearer.

https://codereview.appspot.com/8166047/diff/1/Source/WebCore/rendering/Render...
Source/WebCore/rendering/RenderLayer.cpp:6085: if (!oldStyle)
I see this is the same pattern as existed already with
updateStackingContextsAfterStyleChange --  but it's not obvious to me that this
would work correctly on the very first update - i.e. there may not be an
oldStyle, but the visibility status is still not yet known based on the existing
style, and needs to be known?

[hartmanng, 2013-04-02 01:48:47]

Apart from Shawn's comments, I don't think I have much new insight to add, given
that most of this is based on code I wrote, and Ian and I discussed quite a bit.
Basically, the bulk of it looks good to me.

https://codereview.appspot.com/8166047/diff/1/Source/WebCore/dom/Document.cpp
File Source/WebCore/dom/Document.cpp (right):

https://codereview.appspot.com/8166047/diff/1/Source/WebCore/dom/Document.cpp...
Source/WebCore/dom/Document.cpp:138: #include "RenderLayer.h"
On 2013/04/01 23:12:50, shawnsingh wrote:
> Maybe we should avoid adding this dependency.  It makes sense to me that
> conceptually the Document needs to know about RenderView and RenderObject.
> (though maybe even that can be reduced in the long-term, too).   Would it make
> sense to move ScopedCompositedScrollingUpdater into its own file, even if it
> still lives in rendering/ ?

This might be a good solution. I personally have no problem with the RAII object
in this file (in fact, anything that helps improve correctness is good if you
ask me), but I can see WebKit reviewers potentially not liking this dependency.

https://codereview.appspot.com/8166047/diff/1/Source/WebCore/rendering/Render...
File Source/WebCore/rendering/RenderLayer.cpp (right):

https://codereview.appspot.com/8166047/diff/1/Source/WebCore/rendering/Render...
Source/WebCore/rendering/RenderLayer.cpp:147: static int gNumUpdaterInstances =
0;
On 2013/04/01 23:12:50, shawnsingh wrote:
> At the very least, shouldn't we make this a private static member of
> ScopedCompositedScrollingUpdater instead of global?  That would protect us
from
> unintended usage.  Perhaps we should call it a "semaphore", too.
> 
> A few more concerns, though:
> (1) Are we able to guarantee we won't run into any static initialization races
> (e.g. if anyone ends up initializing some RenderLayers or the dom/Document 
> object statically)?
> 
> (2) And worse, what happens if we have multiple tabs on a single process?  Is
> there some sort of "context" that isolates this data for a single RenderView
> that i'm not aware of?

+1 I didn't think of the multi-tab case.

https://codereview.appspot.com/8166047/diff/1/Source/WebCore/rendering/Render...
Source/WebCore/rendering/RenderLayer.cpp:6317: // is done). Perhaps we can
update the layout test instead?
Yeah, I think updating the layout test is definitely a better way to go.

[jchaffraix, 2013-04-03 20:33:06]

https://codereview.appspot.com/8166047/diff/7001/LayoutTests/compositing/over...
File
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling-after-style-change.html
(right):

https://codereview.appspot.com/8166047/diff/7001/LayoutTests/compositing/over...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling-after-style-change.html:127:
contentSuccessor.style.display = n ? 'block' : 'none';
I find the massive nested for loops to be difficult to understand.

I think a single list of what you want to test would be more readable and show
the intent more clearly. You can even use JSON object to make it super clear:

var tests = [ { 'width': '1px', 'height': '1px', 'overflowX': 'visible',
'overflowY': 'visible', 'display': 'block' }, { ... }, ... ];
for (i = 0; i < tests.len; ++i)
   writeResult(i, tests[i]);

https://codereview.appspot.com/8166047/diff/7001/LayoutTests/compositing/over...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling-after-style-change.html:129:
writeResult(count++, !!((i || j || n) && (k || m)));
Head explosion on the iteration count logic.

https://codereview.appspot.com/8166047/diff/7001/LayoutTests/compositing/over...
File LayoutTests/compositing/overflow/dynamic-composited-scrolling-status.html
(right):

https://codereview.appspot.com/8166047/diff/7001/LayoutTests/compositing/over...
LayoutTests/compositing/overflow/dynamic-composited-scrolling-status.html:39:
window.setTimeout(reportResult, 0);
Using setTimeout(..., 0) is a bad smell that something is wrong with your test
case.

If you need to force a layout, you can always force it synchronously using
document.body.offsetLeft which would remove the need for this change.

https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/dom/Document.cpp
File Source/WebCore/dom/Document.cpp (right):

https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/dom/Document....
Source/WebCore/dom/Document.cpp:138: #include "RenderLayer.h"
I am with Shawn here. We shouldn't add knowledge of RenderLayer outside
rendering as this is an internal detail and should stay this way.

https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
File Source/WebCore/rendering/RenderLayer.cpp (right):

https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderLayer.cpp:2087: void
RenderLayer::scheduleUpdateNeedsCompositedScrolling()
This function's name is a bit of a misnomer: scheduling means that we do the
update later when we actually will force it to be done now (or soonish).

https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderLayer.cpp:2121: if (!m_renderView)
Is it really possible not to have a RenderView?

https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderLayer.cpp:2124:
m_renderView->scopedCompositedScrollingUpdatersCount()++;
Most of the lines are: m_renderView->doSomething()

It really seems like we should move this code to RenderView, maybe including
ScopedCompositedScrollingUpdater (not sure if it would help though).

https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderLayer.cpp:2125: if (!layer ||
!layer->acceleratedCompositingForOverflowScrollEnabled())
You check for !layer above so it's unneeded to check it here again.

https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderLayer.cpp:2152: while (stackingContext &&
!stackingContext->isStackingContext())
Wouldn't that warrant an enclosingStackingContext() function?

This could simplify the code to:
if (RenderLayer* enclosingStackingContext = iter->enclosingStackingContext())
    stackingContexts.add(enclosingStackingContext);

https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderLayer.cpp:6099: if (isStackingContainer())
If you are checking isStackingContainer here, shouldn't you also check for
isStackingContainer above? This discrepancy seems fishy.

https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):

https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderLayerCompositor.cpp:2904:
ScopedCompositedScrollingUpdater
suspendCompositedScrollingUpdates(m_renderView);
I am really concerned about having so many call sites to patch. That makes it
easy to A) forget one or B) have one too many.

Ideally we would like to either do that lazily (ie it's transparent to the
caller) or have a single update phase followed by just getting the results,
ASSERT'ing it's not stale.

https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
File Source/WebCore/rendering/RenderView.h (right):

https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderView.h:231: unsigned&
scopedCompositedScrollingUpdatersCount() { return
m_scopedCompositedScrollingUpdatersCount; }
This is a violation of the encapsulation as you allow arbitrary callers to
modify the internal of this class.

The usual pattern is several methods which I would rather see here.

https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderView.h:232: HashSet<RenderLayer*>&
pendingCompositedScrollingUpdates() { return
m_pendingCompositedScrollingUpdates; }
Same comment here actually. Maybe moving most of the update code to RenderView
would alleviate the need for them?

[vollick, 2013-04-09 16:33:33]

Thanks for the review! I've posted a response and a new patch here
https://codereview.chromium.org/13913013/