[webkit-reviews] review granted: [Bug 31879] Optimize the hierarchy rebuilding of compositing layers : [Attachment 43856] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted Simon Fraser (smfr) <simon.fraser at apple.com>'s
request for review:
Bug 31879: Optimize the hierarchy rebuilding of compositing layers
https://bugs.webkit.org/show_bug.cgi?id=31879

Attachment 43856: Patch
https://bugs.webkit.org/attachment.cgi?id=43856&action=review

------- Additional Comments from mitz at webkit.org
> +	   (WebCore::RenderLayerCompositor::rebuildCompositingLayerTree):
Changed to
> +	   collect child layers into Vectors of GraphicsLayers, which can be
set as
> +	   layer childen in one go.

Typo: childen.

> +void GraphicsLayer::setChildren(const Vector<GraphicsLayer*>& newChildren)

Perhaps you should use the GraphicsLayerList type here (move its definition if
needed).

> +    for (size_t i = 0; i < listSize; ++i)
> +	   addChild(newChildren.at(i));

Usually at() is used with a pointer to a vector. Since you have a reference,
please use [] notation.

> +    GraphicsLayer::setChildren(children);
> +    noteLayerPropertyChanged(ChildrenChanged);

Would it be more efficient if setChildren() returned a bool saying whether it
had actually done anything, so you could skip notLayerPropertyChanged() if it
hadn’t? Or is it very uncommon for setChildren() to be called with the same
children vector?

>  
> -    // Now create and parent the compositing layers.
> -    {
> +    if (needLayerRebuild) {
> +	   // Now updated and parent the compositing layers.
>	   CompositingState compState(updateRoot);
> -	   rebuildCompositingLayerTree(updateRoot, compState,
needLayerRebuild);
> +	   GraphicsLayerList childList;
> +	   rebuildCompositingLayerTree(updateRoot, compState, childList);
> +
> +	   // Host the document layer in the RenderView's root layer.
> +	   if (updateRoot == rootRenderLayer() && !childList.isEmpty())
> +	       m_rootPlatformLayer->setChildren(childList);
> +    } else {
> +	   // We just need to do a geometry update.
> +	   updateLayerTreeGeometry(updateRoot);
>      }
>      
>  #if PROFILE_LAYER_REBUILD
> @@ -596,11 +604,13 @@ bool
RenderLayerCompositor::canAccelerateVideoRendering(RenderVideo* o) const
>  }
>  #endif
>  
> -void RenderLayerCompositor::rebuildCompositingLayerTree(RenderLayer* layer,
struct CompositingState& compositingState, bool updateHierarchy)
> +void RenderLayerCompositor::rebuildCompositingLayerTree(RenderLayer* layer,
const CompositingState& compositingState, GraphicsLayerList&
enclosingChildList)

In the declaration, you called the last parameter “childList”. I now understand
the “enclosing” part, but I don’t think the name is very good. “Enclosing”
doesn’t refer to the children in the list. For one, I’d drop the word “list”
and work “GraphicsLayer” into the name. I think this argument is a “list of
child GraphicsLayer of the enclosing compositing layer”. Maybe
“childGraphicsLayersOfEnclosingLayer”?

> +    GraphicsLayerList layerChildren;

Please move this definition down to where it’s first used.

> +// This just updates layer geometry without doing any reparenting.

Can you say something like “changing the hierarchy” instead of “doing any
reparenting”?

I think this is good as-is, so r+, but feel free to post an updated version!