[webkit-reviews] review granted: [Bug 19509] Database Tables in the Inspector should be sortable : [Attachment 56774] Adds the ability to sort a Database Table in the Web Inspector.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jessie Berlin <jberlin at webkit.org>'s
request for review:
Bug 19509: Database Tables in the Inspector should be sortable
https://bugs.webkit.org/show_bug.cgi?id=19509

Attachment 56774: Adds the ability to sort a Database Table in the Web
Inspector.
https://bugs.webkit.org/attachment.cgi?id=56774&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	       var comparison;
> +	       if (isNaN(item1) || isNaN(item2))
> +		   comparison = item1 < item2 ? -1 : (item1 > item2 ? 1 : 0);
> +	       else {
> +		   // Don't sort numbers alphanumerically.
> +		   var number1 = parseFloat(item1);
> +		   var number2 = parseFloat(item2);
> +		   comparison = number1 < number2 ? -1 : (number1 > number2 ? 1
: 0);
> +	       }

I think the comments here are a little strange and the code a bit oblique. I
presume the idea here is that if the values are both numeric, we want to
compare them as numbers. But I don't see how isNaN accomplishes this. What does
that function do exactly? Does it look only at the numeric prefix of a number
or will it return false for something like "1a". Also, comparing some pairs
with one sort function and other pairs with a different one is going to create
trouble for the sort algorithm. Fortunately we fixed this a while back so it
couldn't cause a crash in JavaScriptCore by using a tree sort rather than
something like qsort that depends on consistent results from sort function.

If you wanted the code to behavior like this I think you'd need to explain why
isNaN works to check if things are numeric. Even though NaN is "not a number",
I think the usage here is unclear.

But I think there are some possibly better options:

   1) Distinguish numeric columns and non-numeric columns and chose the sort
function based on the column type.

   2) Write a comparison function that works like kCFCompareNumerically,
although I'm not sure that will give results that are good for columns of
floating point numbers.

   3) Scan the column and sort numerically only if every item in the column s a
number.

Maybe you can think of something else that will work even better. Also, I would
say "Sort numbers based on comparing their values rather than a lexicographical
comparison". I think the term "alphanumerically" doesn't describe
lexicographical comparison.

r=me as is, but I think the comparison function can be improved