Bug 173984 – Web Inspector: Default string comparisons to treat numeric characters as numbers

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

We should always be sorting strings as `x1 < x2 < x10` instead of `x1 < x10 < x2`.

[Devin Rousso]

Created attachment 314166 [details]
Patch

[Matt Baker]

Comment on attachment 314166 [details]
Patch

Utility functions are testable. Other than lack of tests, looks good.

[Joseph Pecoraro]

Comment on attachment 314166 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314166&action=review

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1492
> +function numericStringComparator(a, b)
> +{
> +    return a.localeCompare(b, undefined, {numeric: true});
> +}

I totally agree with Matt that we should have a quick unit-test for this. See: LayoutTests/inspector/unit-tests/*

Did this just replace every instance of localeCompare? I think we should name this along the lines of localeCompare otherwise I'm going to forget about it and end up adding localeCompare.

How about:

    numberAwareLocaleCompare() // Gives an idea what it does
    extendedLocaleCompare() // Indicates its somehow better thane just localeCompare
    String.prototype.extendedLocaleCompare // like Number.prototype.bytesToString

[Devin Rousso]

Created attachment 314206 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 314206 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314206&action=review

r=me

> LayoutTests/inspector/unit-tests/string-utilities.html:44
> +            InspectorTest.expectEqual("1".extendedLocaleCompare("2"), -1, `"1" < "2"`);
> +            InspectorTest.expectEqual("2".extendedLocaleCompare("1"), 1, `"2" > "1"`);
> +            InspectorTest.expectEqual("2".extendedLocaleCompare("10"), -1, `"2" < "10"`);
> +            InspectorTest.expectEqual("10".extendedLocaleCompare("2"), 1, `"10" > "2"`);
> +            InspectorTest.expectEqual("1".extendedLocaleCompare("10"), -1, `"1" < "10"`);
> +            InspectorTest.expectEqual("10".extendedLocaleCompare("1"), 1, `"10" > "1"`);

How does: "a2" compare to "a10"?
Can we have a test?

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:676
> +    value: function(other)

Style:

    value(other)

[Devin Rousso]

Created attachment 314287 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 314287 [details]
Patch

Clearing flags on attachment: 314287

Committed r219014: <http://trac.webkit.org/changeset/219014>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.