[webkit-reviews] review denied: [Bug 67586] Map 'lang' and xml:lang attributes to '-webkit-locale' CSS property for use with font fallback and text-transform : [Attachment 119591] revised patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Matt Falkenhagen
<falken at chromium.org>'s request for review:
Bug 67586: Map 'lang' and xml:lang attributes to '-webkit-locale' CSS property
for use with font fallback and text-transform
https://bugs.webkit.org/show_bug.cgi?id=67586

Attachment 119591: revised patch
https://bugs.webkit.org/attachment.cgi?id=119591&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=119591&action=review


> Source/WebCore/html/HTMLElement.cpp:173
> +void HTMLElement::mapLanguageAttributeToLocale(Attribute* attr)

In new code like this, please use words rather than abbreviations. So
“attribute” rather than “attr”.

> Source/WebCore/html/HTMLElement.cpp:176
> +    String value = attr->value();

The local variable should be const AtomicString& instead of String to avoid
unnecessary reference count churn.

> Source/WebCore/html/HTMLElement.cpp:179
> +	   // Have to quote so the locale id is treated as a string instead of
as a
> +	   // CSS keyword.

Should keep this comment all on one line.

> Source/WebCore/html/HTMLElement.cpp:182
> +	   // Empty 'lang' should be treated as 'auto'.

Is it really correct to special case both the lack of an attribute (value of
null) and the empty string (value is empty), but not, say, a string that is
entirely whitespace, other invalid strings, or the word "auto"? I’d like to see
tests covering these cases and also see specification wording that makes it
clear that an empty string should work that way.

> Source/WebCore/html/HTMLElement.cpp:213
> +    } else if (attr->name().matches(XMLNames::langAttr)) {
> +	   mapLanguageAttributeToLocale(attr);

Is using matches instead of == here correct behavior? Should an attribute named
xml:lang that is not actually in the XML namespace be respected? I don’t want
to land this patch without determining this. I see no test cases covering this
since the only test case is XHTML. And no information about why it’s correct to
ignore the namespace, citations of say, other browsers’ behavior or a reference
to relevant text in a standard or specification.

Also, this code will not correctly handle the case where xml:lang is being
removed and we already have a lang attribute. We need a test case that covers
that.