Bug 23357

Created attachment 26775 [details] Patch, changelog

Comment on attachment 26775 [details] Patch, changelog Issues: Should use the 2-clause bsd license. Should avoid the "in" prefix for parameters Multiline if-statements need braces. +#include "RenderLayerBacking.h" This include should be the first include after "config.h" +struct LayerHitTestData { + LayerHitTestData(RenderLayer* inRootLayer, const HitTestRequest& inRequest, HitTestResult& inResult, const IntRect& hitTestRect) + : m_rootLayer(inRootLayer) + , m_request(inRequest) + , m_hitTestRect(hitTestRect) + , m_result(inResult) The initialization list should be indented further and the "in" prefixes are not necessary. + if (renderer()->node()->isHTMLElement() && renderer()->node()->hasID()) + m_graphicsLayer->setName(renderer()->renderName() + String(" ") + ((HTMLElement*)renderer()->node())->id()); Please use a c++ style cast +float RenderLayerBacking::compositingOpacity(float rendererOpacity) const +{ + float finalOpacity = rendererOpacity; + + for (RenderLayer* curr = m_owningLayer->parent(); curr != 0; curr = curr->parent()) { No need for curr != 0. It can just be curr. +// A simple background is either none or a solid color. +static bool hasSimpleBackground(RenderStyle* inStyle) +{ + return !inStyle->hasBackgroundImage(); +} + +static bool hasBorderOutlineOrShadow(RenderStyle* inStyle) +{ + return (inStyle->hasBorder() || inStyle->hasBorderRadius() || inStyle->hasOutline() || (inStyle->boxShadow() != 0)); +} The "in" prefix is not necessary. + Vector<RenderLayer*>* posZOrderList = m_owningLayer->posZOrderList(); + if (posZOrderList && posZOrderList->size() > 0) + for (Vector<RenderLayer*>::const_iterator it = posZOrderList->begin(); it != posZOrderList->end(); ++it) { + RenderLayer* curLayer = (*it); + if (!curLayer->isComposited()) + return true; + } The if-statement should have braces. + } + + Vector<RenderLayer*>* overflowList = m_owningLayer->overflowList(); + if (overflowList && overflowList->size() > 0) + for (Vector<RenderLayer*>::const_iterator it = overflowList->begin(); it != overflowList->end(); ++it) { + RenderLayer* curLayer = (*it); + if (!curLayer->isComposited()) + return true; + } Same here. + + IntSize contentOffset = contentOffsetInCompostingLayer(); Extra space. + // Paint any child layers that have overflow. + Vector<RenderLayer*>* overflowList = m_owningLayer->overflowList(); + if (overflowList) + for (Vector<RenderLayer*>::iterator it = overflowList->begin(); it != overflowList->end(); ++it) + it[0]->paintLayer(rootLayer, context, paintDirtyRect, haveTransparency, paintRestriction, paintingRoot); + + // Now walk the sorted list of children with positive z-indices. + Vector<RenderLayer*>* posZOrderList = m_owningLayer->posZOrderList(); + if (posZOrderList) + for (Vector<RenderLayer*>::iterator it = posZOrderList->begin(); it != posZOrderList->end(); ++it) + it[0]->paintLayer(rootLayer, context, paintDirtyRect, haveTransparency, paintRestriction, paintingRoot); If statements need braces. + // FIXME: move into a helper function + FloatQuad hitTestQuad(hitTestRect); Extra space. + // Layer to clip children + bool hasClippingLayer() const { return m_clippingLayer != 0; } + GraphicsLayer* clippingLayer() const { return m_clippingLayer; } + + // Layer to get clipped by ancestor + bool hasAncestorClippingLayer() const { return m_ancestorClippingLayer != 0; } + GraphicsLayer* ancestorClippingLayer() const { return m_ancestorClippingLayer; } + + bool hasContentsLayer() const { return m_contentsLayer != 0; } + GraphicsLayer* contentsLayer() const { return m_contentsLayer; } Extra spaces before the opening brace. + RenderLayer* findHitCompositingLayer(RenderLayer* rootLayer, const HitTestRequest&, HitTestResult&, const IntRect& hitTestRect, const IntPoint& hitTestPoint); + +private: This private: is redundant + +#if ENABLE(3D_TRANSFORMS) +// This symbol is used to determine from a script whether 3D transforms are enabled (via 'nm'). +bool WebCoreHas3DTransforms = true; This seems a little odd. How is this used and why? +#include "SystemTime.h" I think this is now in WTF + +#ifndef NDEBUG +#include "CString.h" +#include "RenderTreeAsText.h" +#endif + +namespace WebCore { + +struct CompositingState { + CompositingState(RenderLayer* inCompAncestor) + : m_subtreeIsCompositing(false) + , m_compositingAncestor(inCompAncestor) + , m_depth(0) Initialization list should be indented and the "in" prefix is not necessary + { + } + + bool m_subtreeIsCompositing; + RenderLayer* m_compositingAncestor; + int m_depth; // just used for debugging If this is just for debugging, shoudld it be in #ifndef NDEBUG? +void RenderLayerCompositor::enableCompositingMode(bool enable /* = true */) +{ + if (enable != m_compositing) { + m_compositing = enable; Early return might be cleaner here. + + { + // If you uncomment this, be sure to comment the layout() and updateCompositingLayers() lines in + // RenderTreeAsText.cpp, externalRepresentation(), to avoid recursion. + //String dump = externalRepresentation(m_renderView); + //fprintf(stderr, "%s", dump.utf8().data()); We really don't like including commented out code. If you would like to keep this in, you should surround it with a #define that is usually off. + Vector<RenderLayer*>* negZOrderList = layer->negZOrderList(); + if (negZOrderList) + for (Vector<RenderLayer*>::iterator it = negZOrderList->begin(); it != negZOrderList->end(); ++it) { + RenderLayer* curLayer = (*it); + if (!curLayer->isComposited()) { + IntRect childUnionBounds = calculateCompositedBounds(curLayer, layer); + unionBounds.unite(childUnionBounds); + } + } + + Vector<RenderLayer*>* posZOrderList = layer->posZOrderList(); + if (posZOrderList) + for (Vector<RenderLayer*>::iterator it = posZOrderList->begin(); it != posZOrderList->end(); ++it) { + RenderLayer* curLayer = (*it); + if (!curLayer->isComposited()) { + IntRect childUnionBounds = calculateCompositedBounds(curLayer, layer); + unionBounds.unite(childUnionBounds); + } + } + + Vector<RenderLayer*>* overflowList = layer->overflowList(); + if (overflowList) + for (Vector<RenderLayer*>::iterator it = overflowList->begin(); it != overflowList->end(); ++it) { + RenderLayer* curLayer = (*it); + if (!curLayer->isComposited()) { + IntRect curAbsBounds = calculateCompositedBounds(curLayer, layer); + unionBounds.unite(curAbsBounds); + } + } If statements need braces. + bool childOverflowOnly = layer->isOverflowOnly(); + RenderLayer* curr = layer->parent(); + + for ( ; curr != 0; curr = curr->parent()) { The RenderLayer* curr = layer->parent(); can be inside the for-loop header and there is no need for the != 0. +RenderLayer* RenderLayerCompositor::enclosingNonStackingClippingLayer(const RenderLayer* layer) const +{ + RenderLayer* curr = layer->parent(); + for ( ; curr != 0; curr = curr->parent()) { Same here. + ASSERT(!layer->m_overflowListDirty); + Vector<RenderLayer*>* overflowList = layer->overflowList(); + if (overflowList && overflowList->size() > 0) + for (Vector<RenderLayer*>::const_iterator it = overflowList->begin(); it != overflowList->end(); ++it) { + RenderLayer* curLayer = (*it); + computeCompositingRequirements(curLayer, childState); + } + + if (layer->isStackingContext()) { + Vector<RenderLayer*>* posZOrderList = layer->posZOrderList(); + if (posZOrderList && posZOrderList->size() > 0) + for (Vector<RenderLayer*>::const_iterator it = posZOrderList->begin(); it != posZOrderList->end(); ++it) { + RenderLayer* curLayer = (*it); + computeCompositingRequirements(curLayer, childState); + } + } Braces around multiline if-statements. +void RenderLayerCompositor::rebuildCompositingLayerTree(RenderLayer* layer, struct CompositingState& ioCompState) +{ +#if 0 + bool internalNeedToComposite = requiresCompositingLayer(layer); + + for (int i = 0; i < ioCompState.m_depth; ++i) + fprintf(stderr, " "); + + fprintf(stderr, "rebuildCompositingLayerTree %p: has layer %d, requires layer %d, force layer %d, stacking %d\n", + this, layer->isComposited(), internalNeedToComposite, ioCompState.m_subtreeIsCompositing, layer->isStackingContext()); + + for (int i = 0; i < ioCompState.m_depth; ++i) + fprintf(stderr, " "); + fprintf(stderr, "forceCompositingLayer(%d) %p\n", ioCompState.m_subtreeIsCompositing, this); +#endif This is pretty much the same thing as commented out code. If we want to keep this, we should a named macro. + ASSERT(!layer->m_overflowListDirty); + Vector<RenderLayer*>* overflowList = layer->overflowList(); + if (overflowList && overflowList->size() > 0) + for (Vector<RenderLayer*>::iterator it = overflowList->begin(); it != overflowList->end(); ++it) { + RenderLayer* curLayer = (*it); + rebuildCompositingLayerTree(curLayer, childState); + if (curLayer->isComposited()) + setCompositingParent(curLayer, childState.m_compositingAncestor); + } + + if (layer->isStackingContext()) { + Vector<RenderLayer*>* posZOrderList = layer->posZOrderList(); + if (posZOrderList && posZOrderList->size() > 0) + for (Vector<RenderLayer*>::const_iterator it = posZOrderList->begin(); it != posZOrderList->end(); ++it) { + RenderLayer* curLayer = (*it); + rebuildCompositingLayerTree(curLayer, childState); + if (curLayer->isComposited()) + setCompositingParent(curLayer, childState.m_compositingAncestor); + } + } Multiline if-statment. + Vector<RenderLayer*>* negZOrderList = layer->negZOrderList(); + if (negZOrderList) + for (Vector<RenderLayer*>::iterator it = negZOrderList->begin(); it != negZOrderList->end(); ++it) { + RenderLayer* curLayer = (*it); + int x = 0, y = 0; + curLayer->convertToLayerCoords(layer, x, y); + IntRect childRect(inRect); + childRect.move(-x, -y); + repaintCompositingLayerRect(curLayer, childRect, true); + } + + Vector<RenderLayer*>* posZOrderList = layer->posZOrderList(); + if (posZOrderList) + for (Vector<RenderLayer*>::iterator it = posZOrderList->begin(); it != posZOrderList->end(); ++it) { + RenderLayer* curLayer = (*it); + int x = 0, y = 0; + curLayer->convertToLayerCoords(layer, x, y); + IntRect childRect(inRect); + childRect.move(-x, -y); + repaintCompositingLayerRect(curLayer, childRect, true); + } + + Vector<RenderLayer*>* overflowList = layer->overflowList(); + if (overflowList) + for (Vector<RenderLayer*>::iterator it = overflowList->begin(); it != overflowList->end(); ++it) { + RenderLayer* curLayer = (*it); + int x = 0, y = 0; + curLayer->convertToLayerCoords(layer, x, y); + IntRect childRect(inRect); + childRect.move(-x, -y); + repaintCompositingLayerRect(curLayer, childRect, true); + } Same here, +void RenderLayerCompositor::willBeDetached() +{ + if (m_rootPlatformLayer && m_rootLayerAttached) { + Frame* frame = m_renderView->frameView()->frame(); + if (!frame) + return; + Page* page = frame->page(); + if (!page) + return; + page->chrome()->client()->attachRootGraphicsLayer(frame, 0); + m_rootLayerAttached = false; + } +} + +void RenderLayerCompositor::wasAttached() +{ + if (m_rootPlatformLayer) { + Frame* frame = m_renderView->frameView()->frame(); + if (!frame) + return; + Page* page = frame->page(); + if (!page) + return; + page->chrome()->client()->attachRootGraphicsLayer(frame, m_rootPlatformLayer); + m_rootLayerAttached = true; + } +} These might both look cleaner with early returns. + +#define VERBOSE_COMPOSITINGLAYER 0 We usually put these at the top of the file. + void computeCompositingRequirements(RenderLayer*, struct CompositingState& compState); + void rebuildCompositingLayerTree(RenderLayer* layer, struct CompositingState& compState); + + void removeCompositedChildren(RenderLayer* layer); + + void parentInRootLayer(RenderLayer* layer); The "layer" and "compState" parameter names are not necessary

I'll fix all these issues.

Comment on attachment 26775 [details] Patch, changelog Will attach new patch with stylistic changes addressed.

Created attachment 26986 [details] Patch, changelog

Created attachment 26988 [details] Patch, changelog

Comment on attachment 26988 [details] Patch, changelog Looks great. I'll be much more picky about GraphicsLayer when I see it. :)

Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/WebCore.xcodeproj/project.pbxproj A WebCore/rendering/RenderLayerBacking.cpp A WebCore/rendering/RenderLayerBacking.h A WebCore/rendering/RenderLayerCompositor.cpp A WebCore/rendering/RenderLayerCompositor.h Committed r40434