Defer the real work in updateCompositingLayers until it's really needed.

Defer the real work in updateCompositingLayers until it's really needed.

Many types of change on a web page end up requiring that
compositing state gets updated. Currently this work is performed
immediately after layout, recalcStyle, and other entry points.

However, this work may happen surprisingly many times per actual
rendered frame, which is absolutely wasteful - nothing except the
most recent compositing state will matter when the frame finally
gets drawn. So, instead we can defer all compositing update work
until the compositor is ready to ask for updated compositing state.

BUG=229679, 302726
R=jamesr@chromium.org, vollick@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159961

[shawnsingh, 2013-10-08 11:18:15]

Ian, if you have time, could you please help me figure out why this craps out on
layout tests?  We'll be co-authors on the patch, then.

Please see https://code.google.com/p/chromium/issues/detail?id=302726 comment 60
for description of the status of this patch.

[jamesr, 2013-10-08 17:51:15]

https://codereview.chromium.org/26110004/diff/1/Source/web/PageWidgetDelegate...
File Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/26110004/diff/1/Source/web/PageWidgetDelegate...
Source/web/PageWidgetDelegate.cpp:84: // FIXME: is this a reasonable place for
this?
this is reasonable since we don't have a better entry point on WebWidget for
this.  We do call WebWidget::layout() from the chromium side sometimes when
we're not about to make a frame, but most of the time this is the right place to
do the update.

[Ian Vollick, 2013-10-08 18:32:21]

https://codereview.chromium.org/26110004/diff/1/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/26110004/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:396: // This way we only need to
compute all compositing state once for every frame drawn (if needed).
I love this. :)

https://codereview.chromium.org/26110004/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:415: // FIXME: why isn't
needHierarchyUpdate part of this?
Whoa. No good reason I can see. If we need to rebuild the composited layer tree,
we'd better not bail, right?

https://codereview.chromium.org/26110004/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:427: // FIXME, I think we can
remove this if-condition?
I don't think so. If the update root is non-NULL, then I think there be layers
in the tree we don't visit when computing compositing requirements (and will
therefore have stale state).

https://codereview.chromium.org/26110004/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:1946: // FIXME: this seems
bogus. If we don't know the layout position/size of the plugin yet, would't that
be handled elsewhere?
Totally fishy. Doesn't this mean that if we have a potentially composited
plugin, we'll _always_ need to recompute compositing requirements?

If, as you suggested offline, the original intent of this code had been to
indicate that we need to defer our determination here until post-layer, could we
remove this and the similar deferrals below (since they'll all happen
post-layout)?

https://codereview.chromium.org/26110004/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:1969: // FIXME: this seems
bogus. If we don't know the layout position/size of the frame yet, wouldn't that
be handled elsehwere?
Ditto.

https://codereview.chromium.org/26110004/diff/1/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayerCompositor.h (right):

https://codereview.chromium.org/26110004/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.h:203: void
setShouldReevaluateCompositingAfterLayout() {
m_needsToRecomputeCompositingRequirements = true; }
nit: should this function be renamed as well, then?

[Vangelis Kokkevis, 2013-10-08 23:22:33]

https://codereview.chromium.org/26110004/diff/1/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/26110004/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:1946: // FIXME: this seems
bogus. If we don't know the layout position/size of the plugin yet, would't that
be handled elsewhere?
On 2013/10/08 18:32:21, Ian Vollick wrote:
> Totally fishy. Doesn't this mean that if we have a potentially composited
> plugin, we'll _always_ need to recompute compositing requirements?
> 
> If, as you suggested offline, the original intent of this code had been to
> indicate that we need to defer our determination here until post-layer, could
we
> remove this and the similar deferrals below (since they'll all happen
> post-layout)?

I think the reason this code existed is that there was a special condition to
not switch to compositing mode for zero-sized plugins. Back in the day sites
were using Flash for audio and WebKit didn't want to incur the cost of switching
over to accelerated compositing for it. Maybe we could special-case it for
non-FCM or remove it altogether.

[shawnsingh, 2013-10-15 10:32:52]

If it's OK with reviewers, I would prefer to leave those "bogus" flags in the
code, to reduce the amount of change that happens in one patch.

The latest patch is almost done.  I am only aware of one last iframe layout test
bug, and all other issues I was aware of are debugged.

[shawnsingh, 2013-10-16 02:05:33]

jamesr@  can you please take a look at the most recent patch?   

Aside from iframe issues, I feel that everything else is in place.  Also I hope
there's a chance you can identify some iframe problems in my patch that I'm not
seeing and save me from hours more debugging =)  Thanks in advance.

[shawnsingh, 2013-10-16 06:52:19]

https://codereview.chromium.org/26110004/diff/17001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/26110004/diff/17001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:380: if (!isMainFrame() && frame
&& frame->contentRenderer() && frame->contentRenderer()->compositor())
checking !isMainFrame() is incorrect here.  The way I implemented it, this is
always asking the top-levelCompositor !isMainFrame(), and therefore would never
update children frames.  The correct condition is
frame->contentRenderer()->compositor()->isMainFrame().

I should probably make this function a private static helper to avoid this
mistake from happening again.

[Ian Vollick, 2013-10-17 01:30:25]

I'm still mulling over the frame tree updates in RLC, but the patch seems quite
reasonable. Here are a few minor comments.

https://codereview.chromium.org/26110004/diff/25001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/26110004/diff/25001/Source/core/page/Page.cpp...
Source/core/page/Page.cpp:235: return
scrollingCoordinator->mainThreadScrollingReasonsAsText();
Perhaps this could be passed m_mainFrame->view(). Lots of the other
ScrollingCoordinator fns take a FrameView. It could then call UCL.

https://codereview.chromium.org/26110004/diff/25001/Source/core/rendering/Com...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/26110004/diff/25001/Source/core/rendering/Com...
Source/core/rendering/CompositedLayerMapping.cpp:753:
m_owningLayer->positionOverflowControls();
What's the reason for this change?

[shawnsingh, 2013-10-17 01:33:21]

Dear Reviewers, PTAL.  This patch is getting close to done, now.

One issue in particular needs reviewer feedback please:

compositing/iframes/overlapped-nested-iframes.html

- for layout tests, children iframes may be in a state where layout is not yet
performed, but we are trying to finish all deferred compositing update for the
entire frame tree.  Should we be force invoking layout on-demand for frames that
need layout in that scenario?  For now, I opted to fix the failing layout test
by forcing compositing updates (and hence layout) on the child iframes.  When
trying the test manually, it seems to work find.  I wasn't sure how layout gets
synchronized across iframes.

[shawnsingh, 2013-10-17 01:44:04]

Thanks for the review so far...

https://codereview.chromium.org/26110004/diff/25001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/26110004/diff/25001/Source/core/page/Page.cpp...
Source/core/page/Page.cpp:235: return
scrollingCoordinator->mainThreadScrollingReasonsAsText();
On 2013/10/17 01:30:25, Ian Vollick wrote:
> Perhaps this could be passed m_mainFrame->view(). Lots of the other
> ScrollingCoordinator fns take a FrameView. It could then call UCL.

Can you please clarify - are you suggesting that I put the deferred
updateCompositingLayers() call inside of the scrolling coordinator instead?

https://codereview.chromium.org/26110004/diff/25001/Source/core/rendering/Com...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/26110004/diff/25001/Source/core/rendering/Com...
Source/core/rendering/CompositedLayerMapping.cpp:753:
m_owningLayer->positionOverflowControls();
On 2013/10/17 01:30:25, Ian Vollick wrote:
> What's the reason for this change?

Because we defer computing compositing, we may not decide to composite a layer
before updateLayerPositions() is called.  updateLayerPositions is where
composited scrollbars were being positioned, so in that scenario, positioning
composited scrollbars was getting skipped.  The CompositedLayerMapping here,
which manages those scrollbars, should be the one responsible for re-positioning
composited scrollbars.   Eventually we should be able to remove the code that
positions composited scrollbars from updateLayerPositions, but for now to be
safe, I left it there.

[shawnsingh, 2013-10-17 01:52:08]

I think it should be possible to migrate all usage of updateCompositingLayers()
to simple setNeedsXXX type functions, so that
CompositingUpdateFinishAllDeferredWork is the only type of update that actually
calls updateCompositingLayers.  But I would prefer to do that in a separate
patch once the dust settles from this one.

[shawnsingh, 2013-10-17 03:29:55]

https://codereview.chromium.org/26110004/diff/42001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/26110004/diff/42001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:435:
finishCompositingUpdateForFrameTree(&m_renderView->frameView()->frame());
I debugged some more and determined that this needs to happen earlier in this
function, before a few of the early exits.  Otherwise the parent compositor may
never realize that it needed updating due to a descendant iframe compositor.

[caseq, 2013-10-17 12:37:38]

https://codereview.chromium.org/26110004/diff/50001/LayoutTests/http/tests/in...
File LayoutTests/http/tests/inspector/inspector-test.js (right):

https://codereview.chromium.org/26110004/diff/50001/LayoutTests/http/tests/in...
LayoutTests/http/tests/inspector/inspector-test.js:483: testRunner.display();
Why is this necessary? We used to invoke this only when this is actually
required. It's not that I oppose to doing this unconditionally, but it would be
nice to understand why this is required.

https://codereview.chromium.org/26110004/diff/50001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorLayerTreeAgent.cpp (right):

https://codereview.chromium.org/26110004/diff/50001/Source/core/inspector/Ins...
Source/core/inspector/InspectorLayerTreeAgent.cpp:201: *errorString = "No
CompositedLayerMapping for the provided node";
When are we supposed to hit this? enclosingCompositingLayer() returns only
layers for which isComposited() is true, and the latter just checks for the
presence for compositedLayerMapping, so we should never hit the second clause.
Hitting the first one means we traversed up till the root layer without
encountering a composited layer.
Is this for cases where we've done the style recalc and layout but not yet
updated the layer tree? I think we either need to force layer tree update in
this case, or rather make this function async and actually invoke the callback
only after we've rebuilt the layer tree. The former is cheaper to implement
though less efficient (which probably does not matter, as we throttle calls to
this function on the front-end). The latter is a bit more tricky, but I can help
with it if the "cheap" approach fails (it looks like
RenderLayerCompositor::updateCompositingLayers() bails out in case layout is not
yet done, so if we're unlucky to invoke getLayers() at the moment when layouit
is invalid, a call to updateCompositingLayers() will have no effect).

[Ian Vollick, 2013-10-17 15:24:23]

On 2013/10/17 01:52:08, shawnsingh wrote:
> I think it should be possible to migrate all usage of
updateCompositingLayers()
> to simple setNeedsXXX type functions, so that
> CompositingUpdateFinishAllDeferredWork is the only type of update that
actually
> calls updateCompositingLayers.  But I would prefer to do that in a separate
> patch once the dust settles from this one.

That would be fantastic!

https://codereview.chromium.org/26110004/diff/25001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/26110004/diff/25001/Source/core/page/Page.cpp...
Source/core/page/Page.cpp:235: return
scrollingCoordinator->mainThreadScrollingReasonsAsText();
On 2013/10/17 01:44:04, shawnsingh wrote:
> On 2013/10/17 01:30:25, Ian Vollick wrote:
> > Perhaps this could be passed m_mainFrame->view(). Lots of the other
> > ScrollingCoordinator fns take a FrameView. It could then call UCL.
> 
> Can you please clarify - are you suggesting that I put the deferred
> updateCompositingLayers() call inside of the scrolling coordinator instead?
Yeah. It's a minor point, but I thought it would be nice if Page could continue
to not know about the compositor.

[shawnsingh, 2013-10-17 19:00:04]

Replies inline below. Cheers!

https://codereview.chromium.org/26110004/diff/50001/LayoutTests/http/tests/in...
File LayoutTests/http/tests/inspector/inspector-test.js (right):

https://codereview.chromium.org/26110004/diff/50001/LayoutTests/http/tests/in...
LayoutTests/http/tests/inspector/inspector-test.js:483: testRunner.display();
On 2013/10/17 12:37:38, caseq wrote:
> Why is this necessary? We used to invoke this only when this is actually
> required. It's not that I oppose to doing this unconditionally, but it would
be
> nice to understand why this is required.

Without testRunner.display() here, compositing was not getting updated.  As a
result, composited layers were not setup the way that tests were expecting.  A
lighter-weight option would be to use
window.internals.forceCompositingUpdate(document) but it seemed that testRunner
usage was the pattern in this test already.

https://codereview.chromium.org/26110004/diff/50001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorLayerTreeAgent.cpp (right):

https://codereview.chromium.org/26110004/diff/50001/Source/core/inspector/Ins...
Source/core/inspector/InspectorLayerTreeAgent.cpp:201: *errorString = "No
CompositedLayerMapping for the provided node";
On 2013/10/17 12:37:38, caseq wrote:
> When are we supposed to hit this? enclosingCompositingLayer() returns only
> layers for which isComposited() is true, and the latter just checks for the
> presence for compositedLayerMapping, so we should never hit the second clause.
> Hitting the first one means we traversed up till the root layer without
> encountering a composited layer.
> Is this for cases where we've done the style recalc and layout but not yet
> updated the layer tree? I think we either need to force layer tree update in
> this case, or rather make this function async and actually invoke the callback
> only after we've rebuilt the layer tree. The former is cheaper to implement
> though less efficient (which probably does not matter, as we throttle calls to
> this function on the front-end). The latter is a bit more tricky, but I can
help
> with it if the "cheap" approach fails (it looks like
> RenderLayerCompositor::updateCompositingLayers() bails out in case layout is
not
> yet done, so if we're unlucky to invoke getLayers() at the moment when layouit
> is invalid, a call to updateCompositingLayers() will have no effect).

Yeah - as you suspected, this issue occurs when compositing is not updated yet. 
You're also right that we may never be hitting the second condition on the if
statement - I need to double-check this.

When I tried forcing a compositing update here, it was crashing deep in
updateCompositingLayers().  It did not early exit because layout was incomplete.
 The scenario is that layout is done, but compositing is not yet updated, so no
compositedLayerMappings are yet allocated, not even for the root.  I will try to
understand more detail about why forcing an update here was not working.

[shawnsingh, 2013-10-17 22:14:08]

PTAL - This patch should be ready to go!!!  Pending reviewer feedback, of
course.

It passes all tests locally for me, and manual testing still works fine as far
as I can tell.

(1) The layout test crash was due to a bug in ASSERT code in RenderGeometryMap. 
It was actually passing in release, I just didn't try that and realize that.  So
it does work for now to force an update in the Inspector.  The fix for the bogus
ASSERT is to skip the ASSERT when m_mapping.size() is zero.

(2) The Page.cpp layering violation - I opted to move the re-layout and force
compositing update backwards into Internals.cpp, rather than forwards into
ScrollingCoordinator.  hope you like it.

[caseq, 2013-10-18 05:39:56]

https://codereview.chromium.org/26110004/diff/62001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorLayerTreeAgent.cpp (right):

https://codereview.chromium.org/26110004/diff/62001/Source/core/inspector/Ins...
Source/core/inspector/InspectorLayerTreeAgent.cpp:202: if
(!enclosingLayer->enclosingCompositingLayer() ||
!enclosingLayer->enclosingCompositingLayer()->compositedLayerMapping()) {
I think this is never supposed to be true now that the layer tree should be up
to date at this point. I suggest we either drop this or perhaps drop the second
clause and change the first one to an assert.

[shawnsingh, 2013-10-18 12:05:58]

Looks like the trybots have gotten slow/hung again.  The try runs on patch #11
were pretty successful.

Latest patch #12 addresses caseq's comments.

jamesr, vollick, can both of you please take a look?  I'm hoping to be able to
land by later today (friday) or monday...

[jamesr, 2013-10-18 15:56:42]

https://codereview.chromium.org/26110004/diff/86001/Source/core/rendering/Ren...
File Source/core/rendering/RenderGeometryMap.cpp (right):

https://codereview.chromium.org/26110004/diff/86001/Source/core/rendering/Ren...
Source/core/rendering/RenderGeometryMap.cpp:124: //    if
(roundedIntPoint(rendererMappedResult) != roundedIntPoint(result))
please remove before landing

https://codereview.chromium.org/26110004/diff/86001/Source/core/rendering/Ren...
Source/core/rendering/RenderGeometryMap.cpp:173: //        if
(enclosingIntRect(rendererMappedResult) !=
enclosingIntRect(FloatQuad(result).boundingBox()))
ditto

https://codereview.chromium.org/26110004/diff/86001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/26110004/diff/86001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:375: // Update compositing for
current frame after all descedant frames are updated.
descedant -> descendant

https://codereview.chromium.org/26110004/diff/86001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.h (right):

https://codereview.chromium.org/26110004/diff/86001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.h:246: static void
finishCompositingUpdateForFrameTree(Frame*);
why is this static?  anything that has a Frame* can walk to its FrameView to get
to its RenderLayerCompositor instance

[shawnsingh, 2013-10-18 18:37:29]

New patch PTAL. Response to your question below, other feedback addressed in new
patch.   Thanks for reviewing.

https://codereview.chromium.org/26110004/diff/86001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.h (right):

https://codereview.chromium.org/26110004/diff/86001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.h:246: static void
finishCompositingUpdateForFrameTree(Frame*);
On 2013/10/18 15:56:42, jamesr wrote:
> why is this static?  anything that has a Frame* can walk to its FrameView to
get
> to its RenderLayerCompositor instance

I made this static because earlier I was accidentally using isMainFrame()
instead of compositor->isMainFrame.  This was a source of many of my iframe
failures, and it made sense to prevent an error like that from happening again
by making it static.   (I mentioned this in Comment #7)

I can change this if you want.  We could make it a non-member static if we make
isMainFrame public, for example.

[jamesr, 2013-10-18 19:16:03]

Ah, I see.  I think we'll have to rewrite all these isMainFrame() checks soon
anyway for OOPIF.

Source/web/ lgtm and the overall idea looks good too, although Ian (or someone
else) should double-check the /core/ changes as well.

[Ian Vollick, 2013-10-18 19:23:26]

This lgtm as well, but I'd love it if someone more familiar with the frame tree
could comment on that code in RLC.

https://codereview.chromium.org/26110004/diff/86001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/26110004/diff/86001/Source/core/page/Page.cpp...
Source/core/page/Page.cpp:53: #include "core/rendering/RenderLayerCompositor.h"
We don't need this, right?

[jamesr, 2013-10-18 19:24:57]

https://codereview.chromium.org/26110004/diff/136001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/26110004/diff/136001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:370: void
RenderLayerCompositor::finishCompositingUpdateForFrameTree(Frame* frame)
the tree walk here lgtm as far as I can tell.

[shawnsingh, 2013-10-18 19:36:47]

Thanks for everyone's help.  Fixed the #include, and now I'm going to land
manually and watch the tree.  Every time I sync there's rebase conflicts so I
would like to land while I just rebased.

On 2013/10/18 19:24:57, jamesr wrote:
>
https://codereview.chromium.org/26110004/diff/136001/Source/core/rendering/Re...
> File Source/core/rendering/RenderLayerCompositor.cpp (right):
> 
>
https://codereview.chromium.org/26110004/diff/136001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayerCompositor.cpp:370: void
> RenderLayerCompositor::finishCompositingUpdateForFrameTree(Frame* frame)
> the tree walk here lgtm as far as I can tell.

[shawnsingh, 2013-10-18 19:38:22]

Committed patchset #14 manually as r159961 (presubmit successful).