[webkit-reviews] review granted: [Bug 212626] lang=zh needs to defer to system preferences to know whether it should be simplified or traditional : [Attachment 400894] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jun 3 10:22:47 PDT 2020


Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 212626: lang=zh needs to defer to system preferences to know whether it
should be simplified or traditional
https://bugs.webkit.org/show_bug.cgi?id=212626

Attachment 400894: Patch

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




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


More information about the webkit-reviews mailing list