[webkit-reviews] review denied: [Bug 8516] frame load delegate method webView:willCloseFrame: is called at the wrong times : [Attachment 7870] patch for review

[bugzilla-request-daemon at opendarwin.org]

Geoffrey Garen <ggaren at apple.com> has denied Geoffrey Garen
<ggaren at apple.com>'s request for review:
Bug 8516: frame load delegate method webView:willCloseFrame: is called at the
wrong times
http://bugzilla.opendarwin.org/show_bug.cgi?id=8516

Attachment 7870: patch for review
http://bugzilla.opendarwin.org/attachment.cgi?id=7870&action=edit

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
Generally, I think you're right on here, but I have to r- for a few reasons:

1. Standard patch "etiquette" requires (1) a test case*; (2) changelog and
patch in same file; (3) adding your name to the copyrights of files you've
changed. 

2. Like you point out, I think it's wrong for _closeOldDataSources to send the
-willCloseFrame delegate call. The documentation says, "Invoked when a frame
will be closed," but _closeOldDataSources is called from
_commitProvisionalLoad, which makes the behavior, "Invoked when a frame loads
content." (However, I think it's right for _closeOldDataSources to call
-setMainFrameDocumentReady:NO.)

I sure wouldn't mind it if your patch renamed _detachFromParent to, say,
"_close."

* Test case: You can find examples in LayoutTests/fast. They're snippets of
HTML that demonstrate the problem. run-webkit-tests loads each snippet into a
mini-app called DumpRenderTree to dump the result. Since you're testing the
Objc bindings, you'll probably want to modify DumpRenderTree itself so that a
test can, through JavaScript, tell DumpRenderTree to register for the
-willCloseFrame delegate, then close a frame, then query DumpRenderTree to see
if got the delegate callback.