[webkit-reviews] review denied: [Bug 23395] Web Inpsector Debugger's Source List Should Be Sorted : [Attachment 26823] accidentally neglected to update the change log

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 16 19:59:32 PST 2009


Oliver Hunt <oliver at apple.com> has denied boucher <rboucher at gmail.com>'s
request for review:
Bug 23395: Web Inpsector Debugger's Source List Should Be Sorted
https://bugs.webkit.org/show_bug.cgi?id=23395

Attachment 26823: accidentally neglected to update the change log
https://bugs.webkit.org/attachment.cgi?id=26823&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
This seems awfully convoluted, why not just
function lowerBoundForObjectInSortedList(list, object, comparator) {
    var floor = Math.floor;
    var first = 0;
    var last = list.length - 1;
    while (first <= last)
    {
	var mid = floor((first + last) / 2);
	var c = comparator(object, list[mid]);

	if (c > 0)
	    first = mid + 1;
	else if (c < 0)
	    last = mid - 1;
	else
	{
	    while (mid < length - 1 && comparator(object, list[mid + 1]) == 0)
		mid++;

	    return mid;
	}
    }

    return -1;
}

then you just have
insertionIndex = lowerBoundForObjectInSortedList(select.childNodes, option,
function(...){...})

Which removes the completely unnecessary use of apply -- `call` would have been
better in this case anyway as you have a fixed number of arguments.

My other concern is that your original code would appear to have been untested,
line 957 uses aContext which doesn't appear to exist anywhere (and isn't
necessary), length also doesn't appear to be defined anywhere, although i can't
work out how the code could work at all in this case.


More information about the webkit-reviews mailing list