[webkit-reviews] review granted: [Bug 111725] Adding a histogram to track when m_needsCompositedScrolling is turned on and off : [Attachment 191986] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Glenn Hartmann
<hartmanng at chromium.org>'s request for review:
Bug 111725: Adding a histogram to track when m_needsCompositedScrolling is
turned on and off
https://bugs.webkit.org/show_bug.cgi?id=111725

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=191986&action=review


> Source/WebCore/rendering/RenderLayer.cpp:2018
> +	   if (acceleratedCompositingForOverflowScrollEnabled())
> +	      
HistogramSupport::histogramEnumeration("Renderer.NeedsCompositedScrolling",
m_needsCompositedScrolling, 2);

It would have been nice to explain *what* you are trying to measure as well as
*why*. Without the context, it's difficult to know if this is correct or why we
can't other some other mechanism like FeatureObserver.

After talking with Glenn over IM, they want to measure the frequency of the
feature at the layer level as a rough estimate of what could be done. They
understand that this number will not be actionable but that giving the reasons
for not having composited scrolling are expensive to compute and we want the
overhead minimal.

All in all, the code change make sense but please add the relevant bits to the
ChangeLog or as a comment in the code (remember that WebKit prefers 'why'
comments over 'what' comments though).