[webkit-reviews] review denied: [Bug 20797] add Settings entries for per-script/lang generic family : [Attachment 28601] patch that should build for ports not using ICU

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> 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 28601: patch that should build for ports not using ICU
https://bugs.webkit.org/attachment.cgi?id=28601&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>

I would advise against using macros here. It's true that the code was a bit
boilerplate-ish, but using macros this way makes debugging more painful, and in
my opinion it's not worth the amount of duplication saved here.

I don't see any other problems, but r- for this style issue. Perhaps Mitz has
additional comments.

> Index: WebCore/page/Settings.cpp
> ===================================================================
> --- WebCore/page/Settings.cpp (revision 41628)
> +++ WebCore/page/Settings.cpp (working copy)

> +#define GENERIC_FONT_GETTER_SETTER(generic, Generic) \
> +const AtomicString& Settings::generic##FontFamily(UScriptCode script) const\

> +{\
> +    if (m_##generic##FontFamilyMap.contains(static_cast<int>(script)))\
> +	   return
m_##generic##FontFamilyMap.find(static_cast<int>(script))->second;\
> +    return emptyAtom;\
> +}\
> +\
> +void Settings::set##Generic##FontFamily(const AtomicString& family,
UScriptCode script)\
> +{\
> +    if (m_##generic##FontFamilyMap.set(static_cast<int>(script),
family).second)\
> +	   setNeedsReapplyStylesInAllFrames(m_page);\
> +}
> +
> +GENERIC_FONT_GETTER_SETTER(standard, Standard)
> +GENERIC_FONT_GETTER_SETTER(fixed, Fixed)
> +GENERIC_FONT_GETTER_SETTER(serif, Serif)
> +GENERIC_FONT_GETTER_SETTER(sansSerif, SansSerif)
> +GENERIC_FONT_GETTER_SETTER(cursive, Cursive)
> +GENERIC_FONT_GETTER_SETTER(fantasy, Fantasy)
>  
>  void Settings::setMinimumFontSize(int minimumFontSize)
>  {