[webkit-reviews] review denied: [Bug 27329] Inspector: Properties Should be Sorted more Naturally : [Attachment 32829] Sort Properties with the Alphanum Sort

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Joseph Pecoraro
<joepeck02 at gmail.com>'s request for review:
Bug 27329: Inspector: Properties Should be Sorted more Naturally
https://bugs.webkit.org/show_bug.cgi?id=27329

Attachment 32829: Sort Properties with the Alphanum Sort
https://bugs.webkit.org/attachment.cgi?id=32829&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 45964)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,13 @@
> +2009-07-15  Joseph Pecoraro	<joepeck02 at gmail.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Inspector: Properties Should be Sorted more Naturally
> +	   https://bugs.webkit.org/show_bug.cgi?id=27329
> +
> +	   * inspector/front-end/utilities.js:
> +	   (Object.sortedProperties): added alphaNumSort sorting function
> +
>  2009-07-15  Adam Langley  <agl at google.com>
>  
>	   No review: reverting previous change.
> Index: WebCore/inspector/front-end/utilities.js
> ===================================================================
> --- WebCore/inspector/front-end/utilities.js	(revision 45963)
> +++ WebCore/inspector/front-end/utilities.js	(working copy)
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2007 Apple Inc.  All rights reserved.
> + * Copyright (C) 2007, 2009 Apple Inc.  All rights reserved.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions
> @@ -96,10 +96,43 @@ Object.describe = function(obj, abbrevia
>  
>  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

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

> +	       return 0;
> +
> +	   var diff = 0;
> +	   var chunk = /^\d+|^\D+/;
> +	   var chunka, chunkb, anum, bnum;
> +	   while (diff === 0) {
> +	       if (!a && b)
> +		   return -1;
> +	       if (!b && a)
> +		   return 1;
> +	       chunka = a.match(chunk)[0];
> +	       chunkb = b.match(chunk)[0];
> +	       anum = +chunka+1; // quick falsey check if its a number
> +	       bnum = +chunkb+1; // quick falsey check if its a number
This seems unnecessarily complex -- why "+1" ?	aside from that the second +
should have spaces on either side.

> +	       if (anum && !bnum)
> +		   return -1;
> +	       if (bnum && !anum)
> +		   return 1;
> +	       if (anum && bnum)
> +		   diff = chunka - chunkb;
> +	       else if (chunka !== chunkb)
> +		   return (chunka<chunkb) ? -1 : 1;
Should have spaces around <

> +	       a = a.substring(chunka.length);
> +	       b = b.substring(chunkb.length);
> +	   }
> +	   return diff;
> +    }
> +    

Otherwise this basically seems fine, just needs my comments addressed