Bug 213827

Created attachment 403267 [details] Patch

Windows crashes seem unrelated to this patch.

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

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

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

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

<rdar://problem/65154465>