[webkit-reviews] review denied: [Bug 11882] Need a way to regression test .webarchive output files : [Attachment 12015] Patch v1

[bugzilla-request-daemon at macosforge.org]

mitz at webkit.org has denied mitz at webkit.org's request for review:
Bug 11882: Need a way to regression test .webarchive output files
http://bugs.webkit.org/show_bug.cgi?id=11882

Attachment 12015: Patch v1
http://bugs.webkit.org/attachment.cgi?id=12015&action=edit

------- Additional Comments from mitz at webkit.org
+static NSString* dumpWebArchiveAsText(WebArchive *webArchive)

The first star should be next to the function name.
I think the function name is misleading, since it doesn't actually dump.

+    NSPropertyListFormat format;

Since you don't use the format you can remove this variable and just pass NULL
to propertyListFromData:mutabilityOption:format:errorDescription:.

+    NSString *result = [[NSString alloc] initWithData:xmlData
encoding:NSUTF8StringEncoding];
+    [result release];

This is wrong: you're releasing result so there's nothing keeping it from being
deallocated. You should either autorelease or delay the release until after
you're done with the string.

+    NSMutableString *normalizedResult = [result mutableCopy];

+    return normalizedResult;

This is leaking: -mutableCopy gives you a retained object, so you should
autorelease (but not release) normalizedResult before returning it.

Anyway, there is no need to allocate and initialize an immutable string and
then make a mutable copy. This is simpler:

    NSMutableString *normalizedResult = [[NSMutableString alloc] 
initWithData:xmlData encoding:NSUTF8StringEncoding];
    // normalize the string
    return [normalizedResult autorelease];

I think it's slightly safer to prepend "file://" to the search string, but I
wonder if it wouldn't be safer to not wait until after serialization to do the
normalization, i.e. walk the dictionary and apply the normalization only to
WebResourceURL values.

r- because of the memory management issues.