[webkit-reviews] review denied: [Bug 57488] Cached Resource Overhead Space Usage and Accounting Inaccurate : [Attachment 87645] Patch to reduce ResourceResponse memory usage and bring overhead size inline with reality.

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 57488: Cached Resource Overhead Space Usage and Accounting Inaccurate
https://bugs.webkit.org/show_bug.cgi?id=57488

Attachment 87645: Patch to reduce ResourceResponse memory usage and bring
overhead size inline with reality.
https://bugs.webkit.org/attachment.cgi?id=87645&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=87645&action=review

r- because I think at least a few of my comments indicate real bugs.

> Source/WebCore/platform/network/ResourceResponseBase.cpp:560
> +void ResourceResponseBase::lazyInit(bool baseInitOnly) const

Instead of a boolean, we usually prefer to use a named enum for this sort of
thing, so it's clear what the parameter means at the call sites. I just read
through a bunch of newly modified call sites and it was not at all clear what
lazyInit(true) means.

> Source/WebCore/platform/network/cf/ResourceResponse.h:98
> +    void platformLazyInit(bool baseInitOnly = false);

It's a bad idea to have a default value for a parameter in the subclass, but
not in the case class.

> Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:90
> +	   // FIXME: We may need to do MIME type sniffing here (unless that is
done in CFURLResponseGetMIMEType).

It is done in CFURLResponseGetMIMEType. You can remove the FIXME.

> Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:116
> +	   for (int idx = 0; idx < numCommonHeaderFields; idx++) {
> +	       CFStringRef value;
> +	       if (CFDictionaryGetValueIfPresent(headers.get(),
commonHeaderFields[i], &value))
> +		   m_httpHeaderFields.set(commonHeaderFields[i], value);

We conventionally name for loop index variables just "i" rather than "idx".
Some code below is subscripted by i, and I don't see an outer loop using i as
the index, so I wonder if this is mistaken?

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:105
> +	       for (int idx = 0; idx < numCommonHeaderFields; idx++) {
> +		   if (NSString* headerValue = [headers
objectForKey:commonHeaderFields[idx]])
> +		       m_httpHeaderFields.set([commonHeaderFields[idx]
UTF8String], headerValue);

I suggest using i instead of idx for the index variable.