[Webkit-unassigned] [Bug 27329] Inspector: Properties Should be Sorted more Naturally

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27329





--- Comment #8 from Joseph Pecoraro <joepeck02 at gmail.com>  2009-07-16 07:46:42 PDT ---
(In reply to comment #5)

> I noticed
>   alphaNumSort("01", "1")
> will cause an error.

Wow, excellent point.  Leading zeros really complicates things.  I just wrote a
modified version (unfortunately much dirtier) to be me my desired sort for
leading zeros.  However, this may be a little too stringent, as this really is
a corner case and I resort to using regex again.

  ["000", "001", "002", "003", "00", "01", "02", "0", "1", "2"]

Note that `ls` and the Finder each produce different results.  I'll attach a
picture in a second.


(In reply to comment #6)
> I'm sorry. My mistake, I left out the "!". Now it seems the speed is about the
> same. 

You're right, I just ran the same benchmarks.  Because it is clearer and
basically the same performance, I'll switch to !isNaN().


(In reply to comment #7)
> This seems unnecessarily complex -- why "+1" ?  aside from that
> the second + should have spaces on either side.

The + 1 was to prevent 0 from being a "falsey" value because I later use it in
an if statement.  However this has been changed to the !isNaN() version. 
Cleaner.


> >  Object.sortedProperties = function(obj)
> >  {
> > +
> > +    function alphaNumSort(a,b) {
> > +        a = a.toString();
> > +        b = b.toString();
> In our usage alphaNumSort is always passed strings so these toString calls are
> unnecessary, and are definitely not free, so you shouldn't have them

Done.  This is certainly correct for sortedProperties.  Good catch.  In my
generic tests I had all sorts of values.  If somebody feels this is worth
leaving a comment "// alphaNumSort only sorts strings" when they land this
patch, so anyone wanting to use the sort knows it only works on strings, that
might be nice.


> > +        if (a===b)
> should have spaces around ===

Done.  As well as all other style changes.


> Otherwise this basically seems fine, just needs my comments addressed

Thanks for the review.  Good catch on the toString().

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.