[webkit-reviews] review granted: [Bug 9660] Clean up WebKit and WebCore's finalize methods (needs clearly defined tear-down) : [Attachment 9100] Cleans up the finalize methods

bugzilla-request-daemon at opendarwin.org bugzilla-request-daemon at opendarwin.org
Thu Jun 29 23:54:06 PDT 2006


Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 9660: Clean up WebKit and WebCore's finalize methods (needs clearly defined
tear-down)
http://bugzilla.opendarwin.org/show_bug.cgi?id=9660

Attachment 9100: Cleans up the finalize methods
http://bugzilla.opendarwin.org/attachment.cgi?id=9100&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
In KWQPageState, I don't see what prevents it from being used after being
closed, and then we'd have a storage leak, right?

"If the parent window closes and the WebView has no hostWindow then the WebView
is automatically closed if autoClose is YES."

That's not really a good enough rule for an application like Safari. I think
you want it to also automatically get closed if the WebView is not in a window
(window is nil) and the host window is closed.

There's no need to do the respondsToSelector: call on WebCorePageCacheStateKey
is there?

+    [WebPreferences _removeReferenceForIdentifier: [self
preferencesIdentifier]];

Should not have a space there.

In the case of decoding a WebView, do we get a second viewWillMoveToWindow:
call after the WebView is initialized? If not, then we need to call
addObserver: somewhere or we won't observe the WillClose notification.

autoClose and setAutoClose should be named shouldCloseAutomatically and
setShouldCloseAutomatically or shouldCloseWithWindow and
setShouldCloseWithWindow or something like that.

I don't think the close call in WebHTMLView finalize is correct -- we need to
fix that eventually.

I am confused about why close would be declared in WebHTMLViewPrivate. Either
it's used elsewhere in the WebKit, in which case it can be in WebHTMLView.h
(which is internal to the framework, I believe), or it's used generically on
document views, in which case it doesn't need to be declared at all on
WebHTMLView.

I think this is a reasonable first cut, but needs work. We can land this, but I
think we need to refine some more.

In particular, I suspect there are many ways for a "closed" object to come back
out of the closed state and do things that then won't get cleaned up.

r=me on this first cut, but lets keep working to get this refined



More information about the webkit-reviews mailing list