[webkit-reviews] review granted: [Bug 84817] Add a logging channel and some ouput for compositing : [Attachment 138716] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 84817: Add a logging channel and some ouput for compositing
https://bugs.webkit.org/show_bug.cgi?id=84817

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=138716&action=review


Looks fine, but please revise so it doesn’t break the Windows build.

> Source/WebCore/platform/graphics/GraphicsLayer.h:429
> +    // Return an estimate of how much memory is used for backing store. May
be incorrect for tiled layers.

Maybe this should have the word “estimate” in its name too, not just the
comment.

> Source/WebCore/rendering/RenderLayerBacking.h:162
> +    double backingStoreArea() const;

Probably needs a comment, and maybe the word “estimate” in its name.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:154
> +#if !LOG_DISABLED
> +    return LogCompositing.state == WTFLogChannelOn;
> +#endif
> +    return false;

Should use #else here. I suspect this may be why the Windows build is failing.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:374
> +	   startTime = WTF::currentTime();

I’m surprised the WTF:: prefix is needed here.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:439
> +#if !LOG_DISABLED
> +void RenderLayerCompositor::logCompositingInfo(const RenderLayer* layer)

One neat trick is to always compile logCompositingInfo but make it an inline
function that does nothing in when LOG_DISABLED is set. This eliminates the
need for #if at the call sites.