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

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.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 123296: Patch
https://bugs.webkit.org/attachment.cgi?id=123296&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=123296&action=review


>> LayoutTests/fast/text/content-language-mapped-to-webkit-locale.html:6
>> +<script src="../js/resources/js-test-pre.js"></script>
> 
> IIRC, js-test-*.js is deprecated so it might be better not to use.

@bashi, you seem confused with script-tests deprecation outside of fast/js/. 
Using js-test-*.js is nice.

> LayoutTests/fast/text/content-language-mapped-to-webkit-locale.html:15
> +function getLangOfNode(n) {
> +    e = document.getElementById(n);

We had better follow C++ style rule for naming.  The function name should be
languageOfNode().
http://www.webkit.org/coding/coding-style.html#names-setter-getter

The argument name 'n' is not appropriate.  It should be 'id'.

'e' should be a local variable;  'var element'.
Or you can just remove the local variable and fold this expression in the next
expression; getComputedStyle(document.getElementById(...

> LayoutTests/fast/text/content-language-mapped-to-webkit-locale.html:19
> +shouldBeEqualToString("getLangOfNode('x1')", "zh");
> +shouldBeEqualToString("getLangOfNode('m1')", "ar");

You should add more test cases as Alexey wrote.