the webView:willCloseFrame: delegate method is invoked at the wrong times. it is currently only called when a page is transitioning to having new contents. it is not invoked when a window containing a webview is closed, and isn't triggered by deallocating the frame. this patch adds a _willCloseFrame method to WebFrame which handles the delegate call and KVO notifications. the routine that had those functions now calls _willCloseFrame, and a new call to it is added to _detachFromParent: so that it is invoked when the frame is closed.
Created attachment 7870 [details] patch for review the changelog is on its way next.
Created attachment 7871 [details] changelog the ChangeLog should normally be part of the fix patch, right? sorry, i'm still getting the hang of this.
Yes. The goal is to make landing your change as easy as possible for whoever volunteers to do it. One patch is easier than two. You'll also need to add your name to the copyright info of the files you've modified.
Comment on attachment 7870 [details] patch for review 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.
Created attachment 8049 [details] patch for DumpRenderTree to add the necessary hooks to test delegate messages and window closing This adds some new facilities to DumpRenderTree for logging delegate messages and a way to trigger a test to be performed when the window is closing. It also adds similar hooks for key-value coding and observing, which will make it possible to write tests for the Cocoa bindings support.
Created attachment 8050 [details] test case using DumpRenderTree changes to illustrate this bug
Comment on attachment 8049 [details] patch for DumpRenderTree to add the necessary hooks to test delegate messages and window closing Overall looks good; I haven't reviewed in detail yet, but I saw a couple things: + // to each from of the WebView. Need to fix that comment. + // simply assign numeric IDs on a first-come-first-serve basis + // since test output is fairly deterministic, this appears to work well. I'm not sure this is a good approach. Maybe we can come up with something more reliable than this.
> + // to each from of the WebView. > > Need to fix that comment. heh.. wow, that's really bad cut-and-paste editing. i'll take care of it. > + // simply assign numeric IDs on a first-come-first-serve basis > + // since test output is fairly deterministic, this appears to work well. > > I'm not sure this is a good approach. Maybe we can come up with something more > reliable than this. what about a setDumpDescription:forObject: method that is exposed to the scripts? and DRT can call it directly for things the scripts don't have access to.
Created attachment 8061 [details] replacement patch for DumpRenderTree fixed the comments and changed the way the dumpID works. dumpIDs are now almost entirely configurable from the tests, so the test case author can decide how they should be. I also moved the window closing test out of main and put it into its own function. and added verbosity flags for most of the new messages.
Comment on attachment 8061 [details] replacement patch for DumpRenderTree This looks great. An excellent set of ideas. I see a typo: "messaages" Another thought for the future: if you're going to enhance the test tool like this you should really use a separate bug. It's hard for us to keep track of landing multiple distinct patches from the same bug report; we're going to want this to stay open while we fix the real bug. In general we try to use nouns for method names that return values. So "dump" is a method name for something that has a side effect and no result (unless you mean the "city dump" and I know you don't). So this patch would be even better if the method was named something more like descriptionForTestResult or descriptionForDumping or something else that's a noun phrase rather than a verb. These standards are not really as important in a test tool. The MethodSwizzle technique unfortunately uses API that's deprecated, but I guess someone on the Apple side will figure out a way to make it work without that. It's a little strange to use class_poseAs elsewhere and this technique here. + // only do this once.. install the replacement KVO methods into NSObject I'd suggest ":" rather than ".." in this message. For the long term, I'm not sure we're going to be keeping the WebDefaultXXXDelegate classes and _WebSafeForwarder -- instead we might just call respondsToSelector in each caller. I suppose that will break this test. Maybe there's a better technique that will accomplish the same thing. I believe the -[NSObject dump] method has a bug. If description is empty, then it will raise an exception because [parts count] will be 0. The KEY() macro is not a great idea; it assumes that an unsigned int is the same size as a pointer. It's easy enough to make a CFDictionary for a purpose like this, or if you must use a hack like this one, -[NSValue valueWithNonretainedObject:] is a better version.