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

bugzilla-request-daemon at macosforge.org bugzilla-request-daemon at macosforge.org
Thu Feb 22 14:16:47 PST 2007


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

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

------- Additional Comments from Darin Adler <darin at apple.com>
Great fix!

Is there no way to add a test for this? We really want a test for each bug fix
if at all possible.

+	     if (listener) {
		 node->removeEventListener(args[0]->toString(exec),
listener,args[2]->toBoolean(exec));
+	     }

Please don't add braces. Our coding style guidelines call for no braces on a
single-line if statement. Also, adding those braces makes the patch longer for
no good reason.

+JSEventListener *Window::findJSEventListener(JSValue* val, bool html) {

Braces that open a function call go on a separate line per our guidelines. The
* goes next to the type name as per our style guidelines.

+  JSObject *object = static_cast<JSObject *>(val);

The * goes next to the type name as per our style guidelines.

-  case Window::RemoveEventListener:
+  case Window::RemoveEventListener: {
	 if (!window->isSafeScript(exec))
	     return jsUndefined();
-	 if (JSEventListener *listener =
Window::retrieveActive(exec)->getJSEventListener(args[1]))
+	 JSEventListener *listener =
Window::retrieveActive(exec)->findJSEventListener(args[1]);
+	 if (listener) {
	     if (Document *doc = frame->document())
		
doc->removeWindowEventListener(AtomicString(args[0]->toString(exec)), listener,
args[2]->toBoolean(exec));
+	 }
	 return jsUndefined();
+  }

Why the gratuitous style change here? By putting the variable inside the if we
avoid the need for braces around the entire case statement. There's no need to
tweak the style while fixing this bug; I think it just makes the patch bigger
and obscures the point of what you're fixing.

+    JSEventListener *findJSEventListener(JSValue*, bool html = false);

* goes next to the type name.

+    // JS Object.
+    JSUnprotectedEventListener *findJSUnprotectedEventListener(JSValue*, bool
html = false);

Please put a blank lines between the comment and the declaration here as with
the other functions.



More information about the webkit-reviews mailing list