[webkit-reviews] review granted: [Bug 213827] Locale-specific quotes infrastructure needs to compare locale strings properly : [Attachment 403267] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 213827: Locale-specific quotes infrastructure needs to compare locale
strings properly
https://bugs.webkit.org/show_bug.cgi?id=213827

Attachment 403267: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 403267
  --> https://bugs.webkit.org/attachment.cgi?id=403267
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?