[webkit-reviews] review denied: [Bug 12463] WebArchiver - attempt to insert nil exception when archive empty iframe : [Attachment 12789] patch

[bugzilla-request-daemon at macosforge.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied David Kilzer
(ddkilzer) <ddkilzer at webkit.org>'s request for review:
Bug 12463: WebArchiver - attempt to insert nil exception when archive empty
iframe
http://bugs.webkit.org/show_bug.cgi?id=12463

Attachment 12789: patch
http://bugs.webkit.org/attachment.cgi?id=12789&action=edit

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
>-	      [subframeArchives addObject:[self
_archiveCurrentStateForFrame:childFrame]];
>+	      WebArchive *childFrameArchive = [self archiveFrame:childFrame];
>+	      if (childFrameArchive)
>+		  [subframeArchives addObject:childFrameArchive];

This is wrong for two reasons:

1. The new code does not match the behavior of the old code.

2. This code doesn't exhibit the same problem that the archiveFrame: method
does.  After the page is loaded, the iframe contains "about:blank" when the
WebArchiver archives the page, so there will never be an attempt to add nil to
an NSMutableArray.

Darin, I need to extend DRT to make this fix testable.	The current method name
is dumpAsWebArchive(), but I need two methods, one that dumps the current DOM
into a webarchive (which is the current behavior) and one that dumps the
original source into a webarchive.

I'm thinking of:  dumpDOMAsWebArchive() and dumpSourceAsWebArchive() or
dumpFrameAsWebArchive().  Any opinions?