[webkit-reviews] review denied: [Bug 20797] add Settings entries for per-script/lang generic family : [Attachment 33195] patch updated (macros expanded)

[bugzilla-daemon at webkit.org]

Eric Seidel <eric 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 33195: patch updated (macros expanded)
https://bugs.webkit.org/attachment.cgi?id=33195&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Why do two hash lookups here?
120	if (m_standardFontFamilyMap.contains(static_cast<int>(script)))
 121	     return
m_standardFontFamilyMap.find(static_cast<int>(script))->second;

Seems you should just store the result of find in a local and check !null

We could do this w/o templates using simple static functions, no?

setFamilyInMap(m_fixedFontFamilyMap, script, m_page);

static inline setFamilyInMap(ScriptFontFamilyMap& familyMap, UScriptCode code,
Page* page)
{
     if (familyMap.set(static_cast<int>(script), family).second)
	 setNeedsReapplyStylesInAllFrames(page);
}

Same could be done for the getters.

Why do we need USCRIPT_CODE_H?	Should it be autogenerated?

Please re-post the patch using the static functions and avoiding the
double-lookup in the getters.  (Unless I"m missing some obvious reason why we
can't use static funtions here?)

Please also explain in the ChangeLog why we're adding UScriptCode, where it
came from, and why it shouldn't be autogenerated (from scriptnames.txt...
whatever that is.)

I'm happy to r+ it with the above changes!  Thanks for the patch!