[webkit-reviews] review granted: [Bug 6616] Double-clicking on a search result seems broken : [Attachment 6830] Patch

bugzilla-request-daemon at opendarwin.org bugzilla-request-daemon at opendarwin.org
Sat Mar 4 11:56:40 PST 2006

Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 6616: Double-clicking on a search result seems broken

Attachment 6830: Patch

------- Additional Comments from Darin Adler <darin at apple.com>
The finalize method needs to call [super finalize]. But also, as long as
there's an observer installed, the object will never get finalized because the
pointers in the notification center will keep it alive. You need to come up
with some other way to handle the object lifetime. Some way of "shutting down"
other than actually deallocating the object that will work both with and
without GC. If this is a global object that's never deallocated, then you just
can not worry about it.

setSearchQuery: and searchPerformed: should use copy on the passed-in string
instead of retain.

+    BOOL selectFirst = (![_private->searchResults count] ? YES : NO);

No need for ? YES : NO on something that's already a boolean.

I noticed that for JavaScript you aren't putting the start brace for functions
on a separate line. I think you should match our C coding style more closely

+    } else if(!query.length && searchActive) {

Above line is missing a space after the if.

+	 <input id="search" type="search" autosave="nodeSearch" results="20"
placeholder="Search" incremental="incremental"
onsearch="performSearch(this.value)" />

Not sure why you are using self-closing tags in this file. Is it XHTML or HTML?

Generally looks good. I'm going to mark it review+ even though the finalize
method is wrong.

More information about the webkit-reviews mailing list