Bug 213827 – Locale-specific quotes infrastructure needs to compare locale strings properly

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Locale-specific quotes infrastructure needs to compare locale strings properly

[Myles C. Maxfield]

Created attachment 403267 [details]
Patch

[Myles C. Maxfield]

Windows crashes seem unrelated to this patch.

[Darin Adler]

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?

[Myles C. Maxfield]

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)

[Darin Adler]

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.

[Darin Adler]

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.

[Myles C. Maxfield]

(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?

[Darin Adler]

(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.

[Myles C. Maxfield]

Committed r263998: <https://trac.webkit.org/changeset/263998>

[Radar WebKit Bug Importer]

<rdar://problem/65154465>