[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 118334] rebase

[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 118334: rebase
https://bugs.webkit.org/attachment.cgi?id=118334&action=review

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


> Source/WebCore/html/HTMLElement.cpp:197
> +	   // FIXME: this does not work when xml:lang is present without lang.

We use sentence style and so should capitalize the word "this".

Why should we write the code knowing it does not handle xml:lang properly? It's
trivial to write code that covers xml:lang too, so lets do that instead of
landing it wrong with the FIXME. I believe we just need to put a case for
XMLNames::langAttr into this parseMappedAttribute function.

> Source/WebCore/html/HTMLElement.cpp:203
> +	   if (hasAttribute(XMLNames::langAttr))
> +	       value = getAttribute(XMLNames::langAttr);
> +	   else
> +	       value = attr->value();

This is the wrong idiom to use.

- First, we should call fastGetAttribute and not getAttribute.
- Second, we should check for presence of the attribute by checking the value
for null, not by a separate call to hasAttribute.
- Third, this code is called only when HTMLNames::langAttr is changed. We
should have code called when XMLNames::langAttr is changed; this entire
function should do nothing at all if an XMLNames::langAttr is present. (See
comment above about covering xml:lang instead of doing a FIXME).

> Source/WebCore/html/HTMLElement.cpp:205
> +	   if (value.length()) {

Better style is to use isEmpty.

> Source/WebCore/html/HTMLElement.cpp:211
> +	       // Have to enclose with a pair of quotation marks to get the
> +	       // locale id treated as a string instead of as a CSS keyword.
> +	       DEFINE_STATIC_LOCAL(String, doubleQuoteChar, ("\""));
> +	       value.insert(doubleQuoteChar, 0);
> +	       value.append(doubleQuoteChar);
> +	       addCSSProperty(attr, CSSPropertyWebkitLocale, value);

I am extremely surprised that quoting is the correct way to do this or only way
to do so. Please double check there is no better solution. If we do need to
quote there are a few things wrong:

1) This is unnecessarily inefficient, allocating two intermediate strings. It's
more efficient to just write "\"" + value + "\"", since it will do both
insertions at once.
2) This does not handle strings with quote marks in them correctly.
3) There is a function named quoteCSSString in CSSParser.h that we could
probably use for this that would do it correctly.