Bug 173984

Summary: Web Inspector: Default string comparisons to treat numeric characters as numbers
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, inspector-bugzilla-changes, joepeck, mattbaker
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=173807
Attachments:
Description Flags
Patch
joepeck: review-, joepeck: commit-queue-
Patch
joepeck: review+
Patch none

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.