[webkit-reviews] review denied: [Bug 76701] Use content-language to set document locale and font : [Attachment 123711] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Matt Falkenhagen
<falken at chromium.org>'s request for review:
Bug 76701: Use content-language to set document locale and font
https://bugs.webkit.org/show_bug.cgi?id=76701

Attachment 123711: Patch
https://bugs.webkit.org/attachment.cgi?id=123711&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=123711&action=review


> Source/WebCore/ChangeLog:3
> +	   Use content-language to set document locale and font

Does this work in Firefox and/or IE?

> Source/WebCore/css/CSSStyleSelector.cpp:1238
> +    if (!document->contentLanguage().isEmpty())
> +	   documentStyle->setLocale(document->contentLanguage());

What guarantees correct precedence of these hints? In tests, I see that a lang
attribute on a node takes precedence. Are there any locale hints already
implemented in WebKit for which precedence needs to be tested?

> Source/WebCore/css/CSSStyleSelector.cpp:1272
> -	   const AtomicString& stdfont = settings->standardFontFamily();
> +	   const AtomicString& stdfont =
settings->standardFontFamily(fontDescription.script());

It's not a new name, but "stdfont" is of course not in accordance to WebKit
coding style.

> Source/WebCore/dom/Document.cpp:1096
> +void Document::setContentLanguage(const String& language)

I think that this name should make it clear that we're parsing it in accordance
to pragma rules. "This pragma is not exactly equivalent to the HTTP
Content-Language header. [HTTP]"

HTML5 itself calls this "pragma set default language".

> Source/WebCore/dom/Document.cpp:1102
> +    size_t pos = strippedLanguage.find(' ');

Please check for all HTML whitespace characters, not just U+0020. I don't think
that simplifyWhiteSpace() gets you exactly that, not to mention that it's
unnecessarily slow to modify the string.

> Source/WebCore/dom/Document.cpp:1107
> +	   recalcStyle(Force);

I don't know if it's OK to make this call here. We don't do that often in
Document.cpp.

Could you please check with a layout&rendering expert?

> Source/WebCore/dom/Document.cpp:2715
> +	   if (contentLanguage().isEmpty())
> +	       setContentLanguage(content);

It may be a mistake in the spec that an empty or invalid Content-Language can
be overwritten with a later pragma. Why the special case?

> Source/WebCore/dom/Document.cpp:2716
> +    } else if (equalIgnoringCase(equiv, "x-dns-prefetch-control"))

Unrelated to this patch, but I wonder if any other pragmas need to be respected
only once. It can be important to not let attacker controlled content override
these values.

> LayoutTests/fast/text/content-language-multiple-tags.html:4
> +<meta http-equiv="content-language" content="zh-CN"/>

These are all HTML files - why do you put the slash in the end? It's just
ignored by the parser.

> LayoutTests/fast/text/content-language-multiple-tags.html:7
> +<link rel="stylesheet" href="../js/resources/js-test-style.css" />

Ditto.

> LayoutTests/fast/text/international/content-language-font-selection.html:4
> +<meta http-equiv="content-language" content="zh-tw"/>

It looks like all tests have a lower case "content-language" string. Please
test that it's case insensitive.