[webkit-reviews] review granted: [Bug 72520] [chromium] Create CCDamageTracker class to determine regions of change for a surface : [Attachment 116708] forgot to commit style fix, should be fixed now

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Shawn Singh
<shawnsingh at chromium.org>'s request for review:
Bug 72520: [chromium] Create CCDamageTracker class to determine regions of
change for a surface
https://bugs.webkit.org/show_bug.cgi?id=72520

Attachment 116708: forgot to commit style fix, should be fixed now
https://bugs.webkit.org/attachment.cgi?id=116708&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116708&action=review


Enne covered the logic so I have nothin' but style nits. R=me, but a decent
number of things to follow up on.

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:54
> +void CCDamageTracker::updateDamageRectForNextFrame(const
WTF::Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID,
CCLayerImpl* targetSurfaceMaskLayer)

nit: remove the WTF:: from WTF::Vector, there's a using declaration in Vector.h
that pulls Vector into the global namespace and WebKit style is to leave out
the qualifier

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:153
> +FloatRect CCDamageTracker::computeDamageFromActiveLayers(const
WTF::Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID)

drop WTF::

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:215
> +    originTransform.translate3d(-0.5 * layer->bounds().width(), -0.5 *
layer->bounds().height(), 0);

should just be translate()

can you comment (briefly) why this translation is taking place?

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:278
> +	      
replicaOriginTransform.translate3d(layer->replicaLayer()->position().x(),
layer->replicaLayer()->position().y(), 0);

why translate3d(.., ..., 0) instead of translate(.., ...)? the latter will save
a round of multiplications per component

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:280
> +	      
replicaOriginTransform.translate3d(-layer->replicaLayer()->position().x(),
-layer->replicaLayer()->position().y(), 0);

translate()

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:294
> +	  
replicaOriginTransform.translate3d(layer->replicaLayer()->position().x(),
layer->replicaLayer()->position().y(), 0);

translate()

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:296
> +	  
replicaOriginTransform.translate3d(-layer->replicaLayer()->position().x(),
-layer->replicaLayer()->position().y(), 0);

same here - should just be translate()

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h:42
> +    static WTF::PassOwnPtr<CCDamageTracker> create();

remove WTF:: please

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h:43
> +    virtual ~CCDamageTracker() { }

please out-of-line this destructor (leave just a declaration here and put the
definition in the .cpp), the OwnPtr<HashMap> destructor is pretty darn big and
we don't want it in the header.

should this be virtual? i don't see any subclasses or protected members

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h:45
> +    void updateDamageRectForNextFrame(const WTF::Vector<RefPtr<CCLayerImpl>
>& layerList, int targetSurfaceLayerID, CCLayerImpl* targetSurfaceMaskLayer);

drop WTF::

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h:51
> +    FloatRect computeDamageFromActiveLayers(const
WTF::Vector<RefPtr<CCLayerImpl> >& layerList, int targetSurfaceLayerID);

remove WTF:: please

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:89
> +    root->renderSurface()->setContentRect(IntRect(IntPoint(),
IntSize(500.0f, 500.0f)));

drop the .0f bit from here - we don't use those in WebKit except when it's
necessary to force math to happen in a certain type, and in this case it's just
passed to an int parameter anyway. See
http://www.webkit.org/coding/coding-style.html "Floating point literals"

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:91
> +    child->setPosition(FloatPoint(100.0f, 100.0f));

drop the .0f's here too

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:117
> +    root->renderSurface()->setContentRect(IntRect(IntPoint(),
IntSize(500.0f, 500.0f)));

drop .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:119
> +    child1->setPosition(FloatPoint(100.0f, 100.0f));

drop .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:122
> +    child1->setOpacity(0.5f); // with a child that drawsContent, this will
cause the layer to create its own renderSurface.

0.5 is fine here

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:125
> +    child2->setPosition(FloatPoint(11.0f, 11.0f));

ditto on .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:130
> +    grandChild1->setPosition(FloatPoint(200.0f, 200.0f));

ditto on .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:135
> +    grandChild2->setPosition(FloatPoint(190.0f, 190.0f));

ditto on .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:188
> +    EXPECT_FLOAT_RECT_EQ(FloatRect(0.0f, 0.0f, 500.0f, 500.0f),
rootDamageRect);

ditto on .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:210
> +    EXPECT_FLOAT_RECT_EQ(FloatRect(190.0f, 190.0f, 16.0f, 18.0f),
childDamageRect);
> +    EXPECT_FLOAT_RECT_EQ(FloatRect(0.0f, 0.0f, 500.0f, 500.0f),
rootDamageRect);

ditto on .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:220
> +    child->setUpdateRect(FloatRect(10.0f, 11.0f, 12.0f, 13.0f));

ditto on .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:225
> +    EXPECT_FLOAT_RECT_EQ(FloatRect(110.0f, 111.0f, 12.0f, 13.0f),
rootDamageRect);

ditto on .0f

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:229
> +    child->setUpdateRect(FloatRect(10.0f, 11.0f, 12.0f, 13.0f));

ditto on .0f

i won't annotate the rest in this patch