Bug 173984

Summary:

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

Product:

WebKit

Reporter:

Devin Rousso <hi>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, inspector-bugzilla-changes, joepeck, mattbaker

Priority:

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

Devin Rousso

Reported 2017-06-29 12:51:17 PDT

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

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Devin Rousso

Comment 1 2017-06-29 14:23:10 PDT

Created attachment 314166 [details] Patch

Matt Baker

Comment 2 2017-06-29 14:32:55 PDT

Comment on attachment 314166 [details] Patch Utility functions are testable. Other than lack of tests, looks good.

Joseph Pecoraro

Comment 3 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

Devin Rousso

Comment 4 2017-06-29 19:10:54 PDT

Created attachment 314206 [details] Patch

Joseph Pecoraro

Comment 5 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)

Devin Rousso

Comment 6 2017-06-30 14:11:50 PDT

Created attachment 314287 [details] Patch

WebKit Commit Bot

Comment 7 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>

WebKit Commit Bot

Comment 8 2017-06-30 14:47:10 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.