[webkit-reviews] review requested: [Bug 11882] Need a way to regression test .webarchive output files : [Attachment 12036] Patch v2

[bugzilla-request-daemon at macosforge.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has asked  for review:
Bug 11882: Need a way to regression test .webarchive output files
http://bugs.webkit.org/show_bug.cgi?id=11882

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

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
(In reply to comment #4)
> (From update of attachment 12015 [edit])
> +static NSString* dumpWebArchiveAsText(WebArchive *webArchive)
> 
> The first star should be next to the function name.

Fixed.

> I think the function name is misleading, since it doesn't actually dump.

Renamed to serializeWebArchiveToXML().	Ignore my remarks in Comment #5.

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

Fixed to use nil since it's ObjC code.

> +    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];

Fixed.	Thanks for the suggestion!  Guess I need to better familiarize myself
with the NSMutableString API.

> 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.

I prepended "file://" to the search string and the replacement string, but I
think this code is going to be MUCH clearer to understand than walking the
WebArchive data structure, finding NSURL objects, and replacing them with new
NSURL objects (since the originals are immutable).  Also, I'm not sure if the
original WebArchive uses Mutable versions of data structures, which means I may
have to recreate part or all of the data structure.

Finally, the odds that the WebKit checkout path will appear "randomly" in the
XML is highly unlikely, and even if it did, this is only used for tests and
such an issue could be worked around easily.