[webkit-reviews] review denied: [Bug 70442] [chromium] Track when CCLayerImpl properties have changed : [Attachment 112603] renamed the bool flag

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Shawn Singh
<shawnsingh at chromium.org>'s request for review:
Bug 70442: [chromium] Track when CCLayerImpl properties have changed
https://bugs.webkit.org/show_bug.cgi?id=70442

Attachment 112603: renamed the bool flag
https://bugs.webkit.org/attachment.cgi?id=112603&action=review

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


I don't think the macros in CCLayerImpl.cpp are really worthwhile here - they
aren't used all that much (1, 4, and 12 times by my count) and will make
debugging a lot harder - for example you can't practically single-step, if we
get crashes the line numbers will be bogus.  I'd prefer that you just expand
these out.

Other than that I think this is good to go.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:50
> +#define DESCENDANTS_CHANGED_IF_NOT_EQUAL(member, newvalue)	\

there's only one user of this macro, so it doesn't seem worthwhile having it.
Just expand it out.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:327
> +void CCLayerImpl::setDebugBorderColor(Color c)
> +{
> +    LAYER_CHANGED_IF_NOT_EQUAL(m_debugBorderColor, c);

we don't typically use 1-letter variable names in WebKit. I think 'color' would
be better than 'c', or possibly 'debugBorderColor'