[webkit-reviews] review denied: [Bug 12850] Leaks >10k objects : [Attachment 13328] revised patch again again

bugzilla-request-daemon at macosforge.org bugzilla-request-daemon at macosforge.org
Thu Feb 22 18:34:39 PST 2007


Maciej Stachowiak <mjs at apple.com> has denied Maciej Stachowiak
<mjs at apple.com>'s request for review:
Bug 12850: Leaks >10k objects
http://bugs.webkit.org/show_bug.cgi?id=12850

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

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
The right way to add a test case is to included it in the patch, in the
LayoutTests directory. In this case, LayoutTests/fast/events would be the
appropriate directory. This is so the person committing doesn't have to do
extra work applying the patch.

Also: I think the comments on the findJSEventListener and related methods are
excessive. Can we get these down to one or two lines instead of having whole
paragraphs in the header?

Better yet, how about renaming the methods to make behavior more clear from the
name? getJSEventListener could be renamed to findOrCreateJSEventListener for
instance.

r- for lack of test case. 

Please also consider my feedback about the large comments. We tend to avoid
those in WebKit because they can make it harder to read the code. Code-wise
though, the patch is fine to land once a test case is added to the patch.



More information about the webkit-reviews mailing list