[webkit-reviews] review granted: [Bug 31749] ER: Add a few C-string alternatives to methods that take AtomicString : [Attachment 44043] patch 3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jens Alfke <snej at chromium.org>'s
request for review:
Bug 31749: ER: Add a few C-string alternatives to methods that take
AtomicString
https://bugs.webkit.org/show_bug.cgi?id=31749

Attachment 44043: patch 3
https://bugs.webkit.org/attachment.cgi?id=44043&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   // An alternate version of add() that finds the object by hashing
and comparing
> +	   // with some other type, to avoid the cost of type conversion if the
object is already
> +	   // in the table. HashTranslator must have the following methods:

Better terminology is probably "function members", not "methods", as in the
previous comment.

> +void QualifiedName::init(const AtomicString& p, const AtomicString& l, const
AtomicString& n)

I think we should consider marking this member function inline so it gets
inlined into the constructors. Not sure it's needed, but maybe it would be
better than the extra level of function call. I raised this issue last time,
but I think you may have misunderstood my comment.

> +String HTTPHeaderMap::get(const char* name) const
> +{
> +    const_iterator i = find<const char*,
CaseFoldingCStringTranslator>(name);
> +    if (i == end())
> +	   return String();
> +    return i->second;
> +}

Should we offer a HashMap::get that works with a custom key translator? If we
had one we could use it here.

> +	   String get(const AtomicString &name) const

Wrong position here for the "&" character. It should be by the type name. I
wonder why that style checker script didn't notice.

> +	   pair<iterator, bool> add(const AtomicString &name, const String
&value)

And here.

> +	   // Alternate accessors that are faster than converting the char* to
AtomicString first

Need a period here.

> +	   pair<iterator, bool> add(const char* name, const String &value);

Wrong position here for the "&" character. It should be by the type name.

It's probably not good that the HTTPHeaderMap::get function takes an const
AtomicString& rather than a const String&. If the caller has a String but not
an AtomicString, it will make things slower with no benefit. Similar issue, but
debatable with the HTTPHeaderMap::add function -- tradeoff is slightly
different because the string needs to be made into an AtomicString if it's a
new key. I see no downside to taking a const String& in the get function,
though.

r=me, but I think you could do some further refinement here