[webkit-reviews] review denied: [Bug 20797] add Settings entries for per-script/lang generic family : [Attachment 94025] got rid of a tab in ChangeLog (style error)

[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 94025: got rid of a tab in ChangeLog (style error)
https://bugs.webkit.org/attachment.cgi?id=94025&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=94025&action=review

I think this is great.	But I think we should use a special HashTraits instead
of the +2 hack.  Where are we going to add callers to these new methods?

> Source/JavaScriptCore/wtf/unicode/UScriptCode.h:2
> + * Copyright (c) 1995-2011 International Business Machines Corporation and
others

If we're modifying this, we need to add our copyright, no?

> Source/JavaScriptCore/wtf/unicode/UScriptCode.h:33
> +namespace WTF {

Do we normally modify these headers?  I didn't think we did.

> Source/JavaScriptCore/wtf/unicode/UScriptCode.h:35
> +enum UScriptCode {

I assume this is the right version of the enum to match the other headers?

> Source/WebCore/page/Settings.cpp:59
> +    // USCRIPT_INVALID_CODE is -1 and USRITP_COMMON is 0, which are special
(deleted and empty) in HashMap with int keys.
> +    // To avoid using -1 and 0 as a key, we add 2.
> +    fontMap.set(static_cast<int>(script) + 2, family);

This feels fragile.  Why can't we use -2 and -3 as deleted and empty?  We have
full control over our Hash values with HashTraits.  See HashTraits.h and
StringTraits.h (I think that's what it's called) for examples, or search for
HashTraits in the code base.

> Source/WebCore/page/Settings.h:57
> +    typedef HashMap<int, AtomicString> ScriptFontFamilyMap;

Yeah, I think we just want a special HashTraits for WTF::UScriptCode.