[webkit-reviews] review denied: [Bug 31749] ER: Add a few C-string alternatives to methods that take AtomicString : [Attachment 43738] patch 2 (smaller)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 43738: patch 2 (smaller)
https://bugs.webkit.org/attachment.cgi?id=43738&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +void Element::setCStringAttribute(const QualifiedName& name, const char*
cStringValue) {
> +    ExceptionCode ec;
> +    setAttribute(name, AtomicString(cStringValue), ec);
> +}

Brace needs to go on the next line.

> +    // Please don't use setCStringAttribute in performance-sensitive code;
> +    // use a static AtomicString value instead to avoid the conversion
overhead.
> +    void setCStringAttribute(const QualifiedName&, const char*
cStringValue);

I am not entirely comfortable with adding this and then advising people not to
use it. Also, is there a reason we do this with a separate name and not with
overloading?

> -QualifiedName::QualifiedName(const AtomicString& p, const AtomicString& l,
const AtomicString& n)
> +void QualifiedName::init(const AtomicString& p, const AtomicString& l, const
AtomicString& n)
>  {
>      if (!gNameCache)
>	   gNameCache = new QNameSet;
> @@ -62,6 +62,16 @@ QualifiedName::QualifiedName(const Atomi
>	   m_impl->ref();
>  }

I think this could be marked inline so we don't do an extra function call just
to get in; or maybe the function call is worth it. If we do inline, we would
probably get better code if we put the line of code that creates the QNameSet
into a separate non-inline function.

    if (!gNameCache)
	createNameCache();

Something like that.

>      QualifiedName(const AtomicString& prefix, const AtomicString& localName,
const AtomicString& namespaceURI);
> +    QualifiedName(const AtomicString& prefix, const char* localName, const
AtomicString& namespaceURI);

Does this need advice about when to use it and when to not use it?

> +// Magic adapter that allows the HashMap to take C strings as keys.

Not a big fan of the word "Magic" in this comment.

> +    static unsigned hash(const char* c)
> +    {
> +	   return CaseFoldingHash::hash(c);
> +    }

The use of "c" as the argument name here is a bit strange. I would name it
string.

> +    static bool equal(AtomicString key, const char* s)

I think this needs to take const AtomicString& to avoid causing unnecessary
reference count thrash.

> +	   return equalIgnoringCase(key,s);

We use spaces after commas, so "key" needs a space after it.

> +    static void translate(AtomicString& location, const char* const& c,
unsigned /*hash*/)
> +    {
> +	   location = AtomicString(c);
> +    }

We should make sure we have a member function on AtomicString so we don't have
to construct a temporary AtomicString just to do this operation.

I think the "c" argument can just be const char* and doesn't need to be const
char* const&. And also it's a strange name.

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

We normally do not do else after return.

> +bool HTTPHeaderMap::contains(const char* name) const {

Brace needs to be on the next line.

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

The "*" for const char* needs to be next to "char". The argument should be
const String& instead of String to avoid causing unnecessary reference count
thrash.

> +	   String get(const AtomicString &name) const

The "&" should be moved next to the AtomicString.

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

This should take a const AtomicString& and a const String&.

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

The "*" should be moved next to the char. This should take a const String&.

> +	   void setHTTPHeaderField(const char* name, const String& value) {
setHTTPHeaderField(AtomicString(name), value); }

Could we put this out of line?

I think at least a few of the suggestions above are mandatory, so review-.