[webkit-reviews] review denied: [Bug 78184] Allow per-script font settings to be specified in layout tests : [Attachment 131551] add WebCore.exp.in back in

[bugzilla-daemon at webkit.org]

Hajime Morrita <morrita at google.com> has denied Matt Falkenhagen
<falken at chromium.org>'s request for review:
Bug 78184: Allow per-script font settings to be specified in layout tests
https://bugs.webkit.org/show_bug.cgi?id=78184

Attachment 131551: add WebCore.exp.in back in
https://bugs.webkit.org/attachment.cgi?id=131551&action=review

------- Additional Comments from Hajime Morrita <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=131551&action=review


The approach looks good. added some nitpicky comments.

> Source/WebCore/testing/InternalSettings.cpp:287
> +	   (*setter)(settings(), family, code);

I think we can use a member function pointer here.
http://stackoverflow.com/questions/5499155/c-member-function-pointer

> Source/WebCore/testing/InternalSettings.h:33
> +#include <wtf/unicode/Unicode.h>

Could you get rid of this include? I want the dependency to be as small as
possible.

> Source/WebCore/testing/InternalSettings.h:85
> +    void setFontFamily(const String& family, const String& script,
SetFontFamilyWrapper setter);

For that purpose, this could be be s static function.