[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 120000] incorporated review comments

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

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


>>> Source/WebCore/html/HTMLElement.cpp:211
>>> +	 } else if (attr->name().matches(XMLNames::langAttr)) {
>> 
>> Which of the test cases fails if you change this from matches to ==? I need
to know that to be sure this code is correct.
> 
> In lang-mapped-to-webkit-locale.xhtml, all of the test cases with an
"xml:lang" (and without a "lang" with the same value) fail. For example:
> 
> <div xml:lang="fr" id="n1">
> FAIL getLangOfNode('n1') should be fr. Was auto.
> 
> Full list of failures:
> FAIL getLangOfNode('x1') should be ja. Was auto.
> FAIL getLangOfNode('x2') should be ja. Was auto.
> FAIL getLangOfNode('x3') should be ja. Was auto.
> FAIL getLangOfNode('n1') should be fr. Was auto.
> FAIL getLangOfNode('n2') should be fr. Was auto.
> FAIL getLangOfNode('p1') should be ja. Was auto.
> FAIL getLangOfNode('q3') should be auto. Was ja.
> FAIL getLangOfNode('q4') should be ar. Was ja.
> FAIL getLangOfNode('q5') should be auto. Was ja.

The use of matches here instead of == is indeed wrong or at least unneeded, and
since == is more efficient we should not use matches.

The use of matches is a workaround for bug 75066, which should be fixed
instead.