[webkit-reviews] review granted: [Bug 48278] Convert DumpRenderTree webarchive code to CoreFoundation : [Attachment 71819] Part 3: Convert WebArchiveDumpSupport.mm from NS objects to CF types

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted David Kilzer (ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 48278: Convert DumpRenderTree webarchive code to CoreFoundation
https://bugs.webkit.org/show_bug.cgi?id=48278

Attachment 71819: Part 3: Convert WebArchiveDumpSupport.mm from NS objects to
CF types
https://bugs.webkit.org/attachment.cgi?id=71819&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=71819&action=review

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.h:33
> +extern "C" {
> +typedef struct _CFURLResponse* CFURLResponseRef;
> +}

I don't think extern "C" is needed for CF-style typedefs. At least, we don't
normally use it for them.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.h:40
> -NSString *serializeWebArchiveToXML(WebArchive *webArchive);
> +CFStringRef createXMLStringFromWebArchiveData(CFDataRef webArchiveData);
>  
>  #pragma mark -
>  #pragma mark Platform-specific methods
>  
> -NSURLResponse *unarchiveNSURLResponseFromResponseData(NSData *responseData);

> +CFURLResponseRef createCFURLResponseFromResponseData(CFDataRef
responseData);

Maybe these new functions should return RetainPtrs?

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:30
> +#import <JavaScriptCore/RetainPtr.h>

Does <wtf/RetainPtr.h> not work in DRT?

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:48
> +    if (CFStringCompare(mimeType, CFSTR("text/xml"),
kCFCompareAnchored|kCFCompareCaseInsensitive) == kCFCompareEqualTo)

I think you should mention the change to using a case-insensitive comparison in
your ChangeLog. You also need spaces around the | operator.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:52
> +    if (CFStringCompare(mimeType, CFSTR("application/x-javascript"),
kCFCompareAnchored|kCFCompareCaseInsensitive) == kCFCompareEqualTo)

Ditto.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:60
> +    CFStringLowercase(mimeType, CFLocaleGetSystem());
>      convertMIMEType(mimeType);

Seems like you don't need to lowercase here, given that convertMIMEType is now
case-insensitive.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:72
> +	   RetainPtr<CFStringRef> dataAsString(AdoptCF,
CFStringCreateFromExternalRepresentation(NULL, data, stringEncoding));

Since this is C++ code, you should use 0 instead of NULL. (Though I think we
may be moving toward always using kCFAllocatorDefault in this case.)

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:94
> -    if ([fields objectForKey:@"Connection"])
> -	   [fields removeObjectForKey:@"Connection"];
> -    if ([fields objectForKey:@"Keep-Alive"])
> -	   [fields removeObjectForKey:@"Keep-Alive"];
> +    if (CFDictionaryContainsKey(fields, CFSTR("Connection")))
> +	   CFDictionaryRemoveValue(fields, CFSTR("Connection"));
> +    if (CFDictionaryContainsKey(fields, CFSTR("Keep-Alive")))
> +	   CFDictionaryRemoveValue(fields, CFSTR("Keep-Alive"));

It's weird that we check for the presence of the keys before removing them.
That shouldn't be necessary, right?

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:117
> +    RetainPtr<CFMutableDictionaryRef> responseDictionary(AdoptCF,
CFDictionaryCreateMutable(NULL, 0, &kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));

Same comment about NULL here (and elsewhere in this patch).

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:119
> -    NSMutableString *urlString = [[[response URL] description] mutableCopy];

> -    normalizeWebResourceURL(urlString);
> -    [responseDictionary setObject:urlString forKey:@"URL"];
> -    [urlString release];
> +    RetainPtr<CFMutableStringRef> urlString(AdoptCF,
CFStringCreateMutableCopy(NULL, 0,
CFURLGetString(CFURLResponseGetURL(response.get()))));

What does -[NSURL description] return? Is that really the same as
CFURLGetString?

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:132
> +    SInt64 expectedContentLength =
CFURLResponseGetExpectedContentLength(response.get());
> +    RetainPtr<CFNumberRef> expectedContentLengthNumber(AdoptCF,
CFNumberCreate(NULL, kCFNumberLongLongType, &expectedContentLength));

Seems like you should either be using "long long" or kCFNumberSInt64Type.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:142
> +	   CFIndex statusCode =
CFHTTPMessageGetResponseStatusCode(httpMessage);
> +	   RetainPtr<CFNumberRef> statusCodeNumber(AdoptCF,
CFNumberCreate(NULL, kCFNumberLongType, &statusCode));

Seems like you should either be using "long" or kCFNumberCFIndexType.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:152
> +    CFStringRef url1 =
(CFStringRef)CFDictionaryGetValue((CFDictionaryRef)val1,
CFSTR("WebResourceURL"));
> +    CFStringRef url2 =
(CFStringRef)CFDictionaryGetValue((CFDictionaryRef)val2,
CFSTR("WebResourceURL"));

You could use static_cast here.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:166
> +	       return CFErrorCopyDescription(error);
> +	   return CFSTR("An unknown error occurred converting data to property
list.");

Presumably you need to pass the result of CFSTR() through CFRetain before
returning it to maintain the "create" semantics of this function.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:189
> +	   for (CFIndex i = 0; i < CFArrayGetCount(subresources); ++i) {

It might be better to store the result of CFArrayGetCount in a local variable.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:206
> +	       return CFErrorCopyDescription(error);
> +	   return CFSTR("An unknown error occurred converting property list to
data.");

Same comment here about CFRetain.