Bug 173984 - Web Inspector: Default string comparisons to treat numeric characters as numbers
Summary: Web Inspector: Default string comparisons to treat numeric characters as numbers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-29 12:51 PDT by Devin Rousso
Modified: 2017-06-30 14:47 PDT (History)
4 users (show)

See Also:


Attachments
Patch (24.78 KB, patch)
2017-06-29 14:23 PDT, Devin Rousso
joepeck: review-
joepeck: commit-queue-
Details | Formatted Diff | Diff
Patch (26.64 KB, patch)
2017-06-29 19:10 PDT, Devin Rousso
joepeck: review+
Details | Formatted Diff | Diff
Patch (27.30 KB, patch)
2017-06-30 14:11 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2017-06-29 12:51:17 PDT
We should always be sorting strings as `x1 < x2 < x10` instead of `x1 < x10 < x2`.
Comment 1 Devin Rousso 2017-06-29 14:23:10 PDT
Created attachment 314166 [details]
Patch
Comment 2 Matt Baker 2017-06-29 14:32:55 PDT
Comment on attachment 314166 [details]
Patch

Utility functions are testable. Other than lack of tests, looks good.
Comment 3 Joseph Pecoraro 2017-06-29 17:02:25 PDT
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
Comment 4 Devin Rousso 2017-06-29 19:10:54 PDT
Created attachment 314206 [details]
Patch
Comment 5 Joseph Pecoraro 2017-06-30 11:01:59 PDT
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)
Comment 6 Devin Rousso 2017-06-30 14:11:50 PDT
Created attachment 314287 [details]
Patch
Comment 7 WebKit Commit Bot 2017-06-30 14:47:09 PDT
Comment on attachment 314287 [details]
Patch

Clearing flags on attachment: 314287

Committed r219014: <http://trac.webkit.org/changeset/219014>
Comment 8 WebKit Commit Bot 2017-06-30 14:47:10 PDT
All reviewed patches have been landed.  Closing bug.