[webkit-reviews] review denied: [Bug 20797] add Settings entries for per-script/lang generic family : [Attachment 98254] patch updated per review comments

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Jungshik Shin
<jshin at chromium.org>'s request for review:
Bug 20797: add Settings entries for per-script/lang generic family
https://bugs.webkit.org/show_bug.cgi?id=20797

Attachment 98254: patch updated per review comments 
https://bugs.webkit.org/attachment.cgi?id=98254&action=review

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

Thank you, this looks mostly fine.

> Source/JavaScriptCore/GNUmakefile.list.am:633
> +	Source/JavaScriptCore/wtf/unicode/ScriptcodesFromICU.h \

Typo (capitalization of "codes").

>> Source/JavaScriptCore/wtf/unicode/ScriptCodesFromICU.h:6
>> +#ifndef ScriptCodesFromICU_H
> 
> #ifndef header guard has wrong style, please use: WTF_ScriptCodesFromICU_h 
[build/header_guard] [5]

Unlike most style bot warnings for this patch, this one would be good to
address.

> Source/JavaScriptCore/wtf/unicode/Unicode.h:30
>  #include "qt4/UnicodeQt4.h"
> +#include <wtf/unicode/ScriptCodesFromICU.h>

I would suggest following the example of UnicodeMacrosFromICU.h and including
the new file from platform specific headers (like UnicodeQt4.h). Not sure if
it's actually better, but it's consistent.

> Source/WebCore/ChangeLog:16
> +	   uscript.h has been updated to that of ICU 4.6 with two function
> +	   declarations new to ICU 4.6 removed.

I don't understand this. Can we just match the version of ICU in Mac OS X 10.5
(which is probably the oldest that we care about now)? Using a version of the
file from 4.6 that doesn't quite match 4.6 will be confusing.

Is there anything in the new version that isn't in the old, and that we
actually need in WebKit?

>> Source/WebCore/page/Settings.h:61
>> +	    //static const bool needsDestruction = false;
> 
> Should have a space between // and comment  [whitespace/comments] [4]

Please don't land commented out code.