[webkit-reviews] review granted: [Bug 81358] New PageCache histogram for tracking the highest leverage frame reject reasons. : [Attachment 132295] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Gavin Peters <gavinp at chromium.org>'s
request for review:
Bug 81358: New PageCache histogram for tracking the highest leverage frame
reject reasons.
https://bugs.webkit.org/show_bug.cgi?id=81358

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=132295&action=review


> Source/WebCore/history/PageCache.cpp:282
> +	   const unsigned v = frameRejectReasons & ~(1 << ClientDeniesCaching);


v ?  Can we give this variable a name that explains what it is?  Perhaps
reasonsForRejectingFrameOtherThanClientDeniesCaching?

> Source/WebCore/history/PageCache.cpp:294
> +	   int index = 0;
> +	   if (v & 0xFFFF0000)
> +	       index = 16;
> +	   if (v & 0xFF00FF00)
> +	       index += 8;
> +	   if (v & 0xF0F0F0F0)
> +	       index += 4;
> +	   if (v & 0xCCCCCCCC)
> +	       index += 2;
> +	   if (v & 0xAAAAAAAA)
> +	       index += 1;

Can you factor this out into a helper function.  This logic seems pretty
general.  That will help folks understand what this code is doing.