[webkit-reviews] review denied: [Bug 69197] First round of unit tests for CCLayerTreeHostCommon : [Attachment 110028] fixed a few more incorrect comments

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Shawn Singh
<shawnsingh at chromium.org>'s request for review:
Bug 69197: First round of unit tests for CCLayerTreeHostCommon
https://bugs.webkit.org/show_bug.cgi?id=69197

Attachment 110028: fixed a few more incorrect comments
https://bugs.webkit.org/attachment.cgi?id=110028&action=review

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


3 small nits and this is good to go

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:39
> +// NOTE CAREFULLY:
> +//
> +// In these tests, the relationship between anchor point, position, and
bounds is not intuitive. The following two points
> +// should help clarify the code:

This comment is very nice but I'm not sure why it's in this test file in
particular. Should this be on LayerChromium.h instead? I think it's more likely
that people trying to understand an interface will look at the header, and not
at a comment in a test file.

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:81
> +void setLayerPropertiesForTesting(LayerChromium * layer, const
TransformationMatrix& transform, const TransformationMatrix& sublayerTransform,
const FloatPoint& anchor, const FloatPoint& position, const IntSize& bounds,
bool preserves3D)

WebKit style is to put the * with the typename - so for instance
"LayerChromium* layer".

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:91
> +void executeCalculateDrawTransformsAndVisibility(LayerChromium * rootLayer)

"LayerChromium *" -> "LayerChromium*"

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:161
> +    setLayerPropertiesForTesting(layer.get(), identityMatrix,
arbitraryTranslation, FloatPoint(0.0f, 0.0f), FloatPoint(0.0f, 0.0f),
IntSize(0, 0), false);

note: for the future, there's a FloatPoint::zero() convenience function to get
a 0,0 FloatPoint, or you can just say FloatPoint()