Factor GraphicsLayerUpdater out of RenderLayerCompositor

Factor GraphicsLayerUpdater out of RenderLayerCompositor

This CL moves some code relating to walking the RenderLayer tree and updating
GraphicsLayers out of RenderLayerCompositor. This code didn't interact with any
of the state in RenderLayerCompositor, so it's easy to untangle.

I've also taken the opportunity to remove a pretty gross hack using static
variables to communicate data across the recursive tree walk. Now we just use
normal member variables in the stack object that manages the tree walk.

This CL shouldn't change any behavior.

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

[esprehn, 2014-03-01 09:37:06]

lgtm, couple random nits, The only important one is what is
parentFrameContentLayers? The name doesn't make it sound like it has side
effects, it sounds like some kind of traversal method.

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/GraphicsLayerUpdater.cpp (right):

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/GraphicsLayerUpdater.cpp:2: * Copyright (C)
2009, 2010 Apple Inc. All rights reserved.
ditto, shorter copyright

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/GraphicsLayerUpdater.cpp:51: void
GraphicsLayerUpdater::rebuildTree(RenderLayer* layer, Vector<GraphicsLayer*>&
childLayersOfEnclosingLayer, int depth)
Can we pass a RenderLayer& ?

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/GraphicsLayerUpdater.cpp:135: void
GraphicsLayerUpdater::updateRecursive(RenderLayer* layer)
same

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/GraphicsLayerUpdater.cpp:174: bool
GraphicsLayerUpdater::parentFrameContentLayers(RenderPart* renderer)
This should have a verb name. parentFrameContentLayers doesn't sound like a verb
,what does it do?

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/GraphicsLayerUpdater.h (right):

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/GraphicsLayerUpdater.h:4: *
This is a brand new file, lets use the new copyright! :)

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/GraphicsLayerUpdater.h:48: static bool
parentFrameContentLayers(RenderPart*);
Should it also take a RenderPart& ? Also parentFrameContentLayers is super weird
for a boolean returning method.

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/GraphicsLayerUpdater.h:51: void
update(RenderLayer*);
RenderLayer& ?

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:507:
updater.updateRecursive(updateRoot);
We often just do

AlgorithmObject(...).updateRecursive(updateRoot)

if you don't want to do two lines. Either way is fine.

[abarth-chromium, 2014-03-01 09:38:30]

On 2014/03/01 09:37:06, esprehn wrote:
> lgtm, couple random nits, The only important one is what is
> parentFrameContentLayers? The name doesn't make it sound like it has side
> effects, it sounds like some kind of traversal method.

Yeah, that thing is crazy.  I was going to study it more in the future.

[abarth-chromium, 2014-03-01 09:40:38]

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/GraphicsLayerUpdater.cpp (right):

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/GraphicsLayerUpdater.cpp:51: void
GraphicsLayerUpdater::rebuildTree(RenderLayer* layer, Vector<GraphicsLayer*>&
childLayersOfEnclosingLayer, int depth)
On 2014/03/01 09:37:07, esprehn wrote:
> Can we pass a RenderLayer& ?

Sure.

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/GraphicsLayerUpdater.cpp:174: bool
GraphicsLayerUpdater::parentFrameContentLayers(RenderPart* renderer)
On 2014/03/01 09:37:07, esprehn wrote:
> what does it do?

I haven't figured that out yet.

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/GraphicsLayerUpdater.h (right):

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/GraphicsLayerUpdater.h:4: *
On 2014/03/01 09:37:07, esprehn wrote:
> This is a brand new file, lets use the new copyright! :)

It contains code from an old file.  No fair laundering copyright.

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/GraphicsLayerUpdater.h:48: static bool
parentFrameContentLayers(RenderPart*);
On 2014/03/01 09:37:07, esprehn wrote:
> Should it also take a RenderPart& ? Also parentFrameContentLayers is super
weird
> for a boolean returning method.

This function makes zero sense.  I can leave it in the old location if you
prefer.

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/GraphicsLayerUpdater.h:51: void
update(RenderLayer*);
On 2014/03/01 09:37:07, esprehn wrote:
> RenderLayer& ?

Sure.

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/185293002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:507:
updater.updateRecursive(updateRoot);
On 2014/03/01 09:37:07, esprehn wrote:
> We often just do
> 
> AlgorithmObject(...).updateRecursive(updateRoot)
> 
> if you don't want to do two lines. Either way is fine.

Will do

[esprehn, 2014-03-01 09:45:37]

Let's leave that function with the crazy name, we can leave it in the new
file too. Can you add a FIXME to figure it out then?

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[abarth-chromium, 2014-03-01 09:52:24]

I put the function back where it came from.  Let's keep the crazy all together. 
:)

I did add a comment though.

[abarth-chromium, 2014-03-01 09:52:28]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-03-01 09:52:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/185293002/20001

[commit-bot: I haz the power, 2014-03-01 13:47:49]

Change committed as 168244