Bug 212626

Created attachment 400787 [details] Patch

<rdar://problem/60227623>

Created attachment 400788 [details] Patch

Created attachment 400789 [details] Patch

Created attachment 400874 [details] Patch

Created attachment 400894 [details] Patch

Comment on attachment 400894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400894&action=review OK as is. A few comments. My review comments are mostly about the moved code, so they are about pre-existing issues. > Source/WebCore/platform/graphics/FontDescription.cpp:72 > + const Vector<String>& preferredLanguages = userPreferredLanguages(); > + for (auto& language : preferredLanguages) { This would be better without the local variable. > Source/WebCore/platform/graphics/FontDescription.cpp:93 > + static std::once_flag onceFlag; > + std::call_once(onceFlag, [&] { Using call_once is overkill. This code isn’t otherwise thread-safe, using non-thread safe string reference counting, so the threading-safe call_once machinery isn’t valuable. Another way to do this would be to check cachedSpecializedChineseLocale() for null, and do the work if it’s null. Ideally I think we would encapsulate this logic in a specializedChineseLocale() function rather than putting this in setLocale directly. static const AtomString& specializedChineseLocale() { auto& locale = cachedSpecializedChineseLocale(); if (cachedSpecializedChineseLocale().isNull()) { static char forNonNullPointer; addLanguageChangeObserver(&forNonNullPointer, &fontDescriptionLanguageChanged); // We will never remove the observer, so all we need is a non-null pointer. fontDescriptionLanguageChanged(nullptr); } return locale; } > Source/WebCore/platform/graphics/FontDescription.h:55 > const AtomString& locale() const { return m_locale; } > + const AtomString& originalLocale() const { return m_originalLocale; } Worth considering even better names. To me "original" doesn't say "web-exposed" and "not original" doesn't say "use for text shaping and font fallback". Nor is there any clear rationale here for why one is more specific than the other. Nothing here says *why* web-exposed locale strings should not reveal the specialized Chinese locale. And I think it should. > Source/WebCore/platform/graphics/FontDescription.h:148 > + AtomString m_locale; // This is what you should be using for things like text shaping and font fallback > + AtomString m_originalLocale; // This is what you should be using for web-exposed things like -webkit-locale I’d rather see these comments on the getters rather than the data members. Important for users of this class, rather than implementers of it. Missing periods at the end for WebKit coding style. > Source/WebCore/rendering/RenderQuote.cpp:383 > - if (const QuotesForLanguage* quotes = quotesForLanguage(style().locale())) > + if (const QuotesForLanguage* quotes = quotesForLanguage(style().originalLocale())) This usage doesn’t match the comments. Is this "web-exposed"? I don’t think it’s clear how to use these new properties correctly.

(In reply to Darin Adler from comment #7) > Comment on attachment 400894 [details] > Worth considering even better names. To me "original" doesn't say > "web-exposed" and "not original" doesn't say "use for text shaping and font > fallback". Nor is there any clear rationale here for why one is more > specific than the other. Nothing here says *why* web-exposed locale strings > should not reveal the specialized Chinese locale. And I think it should. The purpose I had for this patch was to change the effective behavior of the locales, but not affect things like getComputedStyle(). WebKit's current behavior is that if you put lang="something" on an element, and then call getComputedStyle("-webkit-locale"), you'll always get out what you put in. In general, the platform text and unicode libraries are free to interpret locales in any way they want - this patch just works around one specific policy they use that isn't the best behavior for the web. I don't thing WebKit's policy should be "reach down into all the system frameworks and try to figure out how they are interpreting each locale string and then expose that to the web."

(In reply to Darin Adler from comment #7) > > Source/WebCore/rendering/RenderQuote.cpp:383 > > - if (const QuotesForLanguage* quotes = quotesForLanguage(style().locale())) > > + if (const QuotesForLanguage* quotes = quotesForLanguage(style().originalLocale())) > > This usage doesn’t match the comments. Is this "web-exposed"? I don’t think > it’s clear how to use these new properties correctly. Yeah, this is a workaround for the table at http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#quotes. It appears to say that lang="zh" should get simplified Chinese quotes, but that zh-Hans shouldn't (unless the lang() function[1] is supposed to do some magic that isn't implemented in this function). I need to investigate this further in the spec, but in the short term, this is a workaround for the test. [1] https://drafts.csswg.org/selectors-4/#the-lang-pseudo

Created attachment 401429 [details] Patch for comitting

Committed r262796: <https://trac.webkit.org/changeset/262796> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401429 [details].

My comment wasn't about "policy", it was about terminology. I don’t understand how your comments justify the terminology. I am sure that you would agree that we want to make code that non-experts have a chance to understand using a combination of naming and comments.

(In reply to Myles C. Maxfield from comment #9) > in the short term, this is a > workaround for the test A comment that says that would likely be helpful.

(In reply to Myles C. Maxfield from comment #8) > (In reply to Darin Adler from comment #7) > > Comment on attachment 400894 [details] > > Worth considering even better names. To me "original" doesn't say > > "web-exposed" and "not original" doesn't say "use for text shaping and font > > fallback". Nor is there any clear rationale here for why one is more > > specific than the other. Nothing here says *why* web-exposed locale strings > > should not reveal the specialized Chinese locale. And I think it should. > > The purpose I had for this patch was to change the effective behavior of the > locales, but not affect things like getComputedStyle(). WebKit's current > behavior is that if you put lang="something" on an element, and then call > getComputedStyle("-webkit-locale"), you'll always get out what you put in. > > In general, the platform text and unicode libraries are free to interpret > locales in any way they want - this patch just works around one specific > policy they use that isn't the best behavior for the web. I don't thing > WebKit's policy should be "reach down into all the system frameworks and try > to figure out how they are interpreting each locale string and then expose > that to the web." OK. But the word "original" doesn't say that to me. A word you used here was "effective". Often a good strategy is to explain the code to someone and then harvest the terminology from that discussion.

Oh, just looked. The terms you used, "specified" and "computed", do seem better. Thanks for changing that.

(In reply to Darin Adler from comment #13) > (In reply to Myles C. Maxfield from comment #9) > > in the short term, this is a > > workaround for the test > > A comment that says that would likely be helpful. I'm fixing this properly here: https://bugs.webkit.org/show_bug.cgi?id=213827

This bug was fixed, but there is still a failure expectation checked in to platform/mac/TestExpectations: # <rdar://problem/60227623> fast/text/international/generic-font-family-language-traditional.html is failing. [ BigSur+ ] fast/text/international/generic-font-family-language-traditional.html [ ImageOnlyFailure ] Marked with this bug number.