Bug 213827 - Locale-specific quotes infrastructure needs to compare locale strings properly
Summary: Locale-specific quotes infrastructure needs to compare locale strings properly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-30 22:13 PDT by Myles C. Maxfield
Modified: 2020-07-06 17:00 PDT (History)
13 users (show)

See Also:


Attachments
Patch (31.71 KB, patch)
2020-06-30 22:23 PDT, Myles C. Maxfield
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2020-06-30 22:13:02 PDT
Locale-specific quotes infrastructure needs to compare locale strings properly
Comment 1 Myles C. Maxfield 2020-06-30 22:23:48 PDT
Created attachment 403267 [details]
Patch
Comment 2 Myles C. Maxfield 2020-07-01 10:29:34 PDT
Windows crashes seem unrelated to this patch.
Comment 3 Darin Adler 2020-07-01 10:42:44 PDT
Comment on attachment 403267 [details]
Patch

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

> Source/WebCore/rendering/RenderQuote.cpp:119
> +    if (result.keyLength == result.rangeLength)
> +        result.comparison = strncmp(key, range, result.keyLength);

I don’t understand why this is an important optimization. But if it is, it should be memcmp, not strncmp. I see no benefit to using strncmp.

> Source/WebCore/rendering/RenderQuote.cpp:121
> +        result.comparison = strcmp(key, range);

Why is this OK when we found hyphens? Will this return -1 or +1 when it should be returning 0? Do we have enough test coverage for this?

> Source/WebCore/rendering/RenderQuote.cpp:162
> +    SubtagComparison firstSubtagComparison = subtagCompare(key->language, range->language);

auto

> Source/WebCore/rendering/RenderQuote.cpp:178
> +    size_t keyOffset = firstSubtagComparison.keyContinue;
> +    while (true) {

How about a for loop?

> Source/WebCore/rendering/RenderQuote.cpp:179
> +        SubtagComparison nextSubtagComparison = subtagCompare(key->language + keyOffset, range->language + firstSubtagComparison.rangeContinue);

auto

> Source/WebCore/rendering/RenderQuote.cpp:209
> +    // FIXME: This table is out-of-date.

How thorough is our test coverage?

> Source/WebCore/rendering/RenderQuote.cpp:480
> +        if (const QuotesForLanguage* quotes = quotesForLanguage(style().computedLocale()))

auto?
Comment 4 Myles C. Maxfield 2020-07-06 14:46:25 PDT
Comment on attachment 403267 [details]
Patch

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

>> Source/WebCore/rendering/RenderQuote.cpp:119
>> +        result.comparison = strncmp(key, range, result.keyLength);
> 
> I don’t understand why this is an important optimization. But if it is, it should be memcmp, not strncmp. I see no benefit to using strncmp.

It's an optimization for lines 164-165 below. I'll change it to memcmp().

>> Source/WebCore/rendering/RenderQuote.cpp:121
>> +        result.comparison = strcmp(key, range);
> 
> Why is this OK when we found hyphens? Will this return -1 or +1 when it should be returning 0? Do we have enough test coverage for this?

It's an optimization for line 167-168 below.

>> Source/WebCore/rendering/RenderQuote.cpp:178
>> +    while (true) {
> 
> How about a for loop?

It would look like "for (size_t keyOffset = firstSubtagComparison.keyContinue; ;) { ... }. Is this better? In the future, how should I know whether to collapse this kind of thing into a for loop?

>> Source/WebCore/rendering/RenderQuote.cpp:209
>> +    // FIXME: This table is out-of-date.
> 
> How thorough is our test coverage?

fast/css-generated-content/quotes-lang.html tests every single one of these (but doesn't test suffixes and prefixes the way that this new test in this patch does)
Comment 5 Darin Adler 2020-07-06 14:48:53 PDT
Comment on attachment 403267 [details]
Patch

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

>>> Source/WebCore/rendering/RenderQuote.cpp:178
>>> +    while (true) {
>> 
>> How about a for loop?
> 
> It would look like "for (size_t keyOffset = firstSubtagComparison.keyContinue; ;) { ... }. Is this better? In the future, how should I know whether to collapse this kind of thing into a for loop?

Also "keyOffset += nextSubtagComparison.keyContinue" as the third part.
Comment 6 Darin Adler 2020-07-06 14:49:06 PDT
Comment on attachment 403267 [details]
Patch

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

>>>> Source/WebCore/rendering/RenderQuote.cpp:178
>>>> +    while (true) {
>>> 
>>> How about a for loop?
>> 
>> It would look like "for (size_t keyOffset = firstSubtagComparison.keyContinue; ;) { ... }. Is this better? In the future, how should I know whether to collapse this kind of thing into a for loop?
> 
> Also "keyOffset += nextSubtagComparison.keyContinue" as the third part.

Oh, but we can’t. Darn.
Comment 7 Myles C. Maxfield 2020-07-06 14:58:27 PDT
(In reply to Darin Adler from comment #6)
> Comment on attachment 403267 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403267&action=review
> 
> >>>> Source/WebCore/rendering/RenderQuote.cpp:178
> >>>> +    while (true) {
> >>> 
> >>> How about a for loop?
> >> 
> >> It would look like "for (size_t keyOffset = firstSubtagComparison.keyContinue; ;) { ... }. Is this better? In the future, how should I know whether to collapse this kind of thing into a for loop?
> > 
> > Also "keyOffset += nextSubtagComparison.keyContinue" as the third part.
> 
> Oh, but we can’t. Darn.

Right. I thought about doing a for loop, but thought that if there's only a single clause, it might as well just be a while loop. But I do see an argument about scoping of the keyOffset variable, so I think in this case a for loop is better. On the other hand, if the loop was "for (; expr;)" it clearly should be a while loop, and if it's "for (;; expr)" then I don't know what it should be. Is there some guidance about the right way to perform these transformations?
Comment 8 Darin Adler 2020-07-06 15:27:04 PDT
(In reply to Myles C. Maxfield from comment #7)
> Is there some guidance about the right way to
> perform these transformations?

Often we can create an object that can help turn a loop into a range-based for loop. Probably my best advice. But don’t do anything here.
Comment 9 Myles C. Maxfield 2020-07-06 16:59:29 PDT
Committed r263998: <https://trac.webkit.org/changeset/263998>
Comment 10 Radar WebKit Bug Importer 2020-07-06 17:00:22 PDT
<rdar://problem/65154465>