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

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin 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 8061: replacement patch for DumpRenderTree
http://bugzilla.opendarwin.org/attachment.cgi?id=8061&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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.