Bug 151438 – Use HTTPHeaderName as much as possible in XMLHttpRequest

• Component: XML
• Status: RESOLVED FIXED

[youenn fablet]

Following on bug 147784, XHR should use HTTPHeaderName::ContentType in lieu of "Content-Type"

[youenn fablet]

Created attachment 265854 [details]
Patch

[Darin Adler]

Comment on attachment 265854 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265854&action=review

> Source/WebCore/xml/XMLHttpRequest.cpp:603
>          if (!m_requestHeaders.contains(HTTPHeaderName::ContentType)) {
>  #if ENABLE(DASHBOARD_SUPPORT)
>              if (usesDashboardBackwardCompatibilityMode())
> -                setRequestHeaderInternal("Content-Type", "application/x-www-form-urlencoded");
> +                m_requestHeaders.set(HTTPHeaderName::ContentType, "application/x-www-form-urlencoded");
>              else
>  #endif
>                  // FIXME: this should include the charset used for encoding.
> -                setRequestHeaderInternal("Content-Type", document->isHTMLDocument() ? "text/html;charset=UTF-8":"application/xml;charset=UTF-8");
> +                m_requestHeaders.set(HTTPHeaderName::ContentType, document->isHTMLDocument() ? "text/html;charset=UTF-8":"application/xml;charset=UTF-8");
>          }

Should be add instead of set. The existing code was doing add, and add is slightly more efficient than set.

Optional: We could also optimize this so it does only one hash table lookup by doing an add unconditionally; will be smaller code size and more efficient. The semantics of add is that it does nothing if the hash table already contains a value for that key. We can pass the new value to add, or if computing it is expensive we can simply pass a null string to add, and then overwrite the null string if it’s a new entry, which can be checked by looking at the isNewEntry boolean in the result of add.

Also, ASCIILiteral("") is more efficient than just passing a literal and letting it be converted to a string.

> Source/WebCore/xml/XMLHttpRequest.cpp:633
>      if (!body.isNull() && m_method != "GET" && m_method != "HEAD" && m_url.protocolIsInHTTPFamily()) {
> -        String contentType = getRequestHeader("Content-Type");
> +        String contentType = m_requestHeaders.get(HTTPHeaderName::ContentType);
>          if (contentType.isNull()) {
>  #if ENABLE(DASHBOARD_SUPPORT)
>              if (usesDashboardBackwardCompatibilityMode())
> -                setRequestHeaderInternal("Content-Type", "application/x-www-form-urlencoded");
> +                m_requestHeaders.set(HTTPHeaderName::ContentType, "application/x-www-form-urlencoded");
>              else
>  #endif
> -                setRequestHeaderInternal("Content-Type", "text/plain;charset=UTF-8");
> +                m_requestHeaders.set(HTTPHeaderName::ContentType, "text/plain;charset=UTF-8");
>          } else {
>              replaceCharsetInMediaType(contentType, "UTF-8");
>              m_requestHeaders.set(HTTPHeaderName::ContentType, contentType);

All the same comments as above.

Note also that add also makes the value of anything existing in the hash table available with addResult.iterator->value, so it can be used for an efficient get/set in a single operation like the one here.

> Source/WebCore/xml/XMLHttpRequest.cpp:658
>          if (!m_requestHeaders.contains(HTTPHeaderName::ContentType)) {
>              const String& blobType = body->type();
>              if (!blobType.isEmpty() && isValidContentType(blobType))
> -                setRequestHeaderInternal("Content-Type", blobType);
> +                m_requestHeaders.set(HTTPHeaderName::ContentType, blobType);
>              else {
>                  // From FileAPI spec, whenever media type cannot be determined, empty string must be returned.
> -                setRequestHeaderInternal("Content-Type", "");
> +                m_requestHeaders.set(HTTPHeaderName::ContentType, "");
>              }
>          }

Same comments again.

> Source/WebCore/xml/XMLHttpRequest.cpp:678
>          if (!m_requestHeaders.contains(HTTPHeaderName::ContentType))
> -            setRequestHeaderInternal("Content-Type", makeString("multipart/form-data; boundary=", m_requestEntityBody->boundary().data()));
> +            m_requestHeaders.set(HTTPHeaderName::ContentType, makeString("multipart/form-data; boundary=", m_requestEntityBody->boundary().data()));

Same comments as above, the simplest case.

[youenn fablet]

Thanks for the review.

> > +                m_requestHeaders.set(HTTPHeaderName::ContentType, document->isHTMLDocument() ? "text/html;charset=UTF-8":"application/xml;charset=UTF-8");
> >          }
> 
> Should be add instead of set. The existing code was doing add, and add is
> slightly more efficient than set.

I would think "set" and "add" to be equivalent both in terms of performances and functionality, since we are doing a contain check before doing it.

"set" seems semantically more appropriate for Content-Type header, as we do not want any concatenation to happen.

I plan to land the patch with "set" but am open to changing it later on.

> Optional: We could also optimize this so it does only one hash table lookup
> by doing an add unconditionally; will be smaller code size and more
> efficient. The semantics of add is that it does nothing if the hash table
> already contains a value for that key. We can pass the new value to add, or
> if computing it is expensive we can simply pass a null string to add, and
> then overwrite the null string if it’s a new entry, which can be checked by
> looking at the isNewEntry boolean in the result of add.

Interesting, this sounds better in terms of performances.
We may be loosing some readability though to handle the expensive-value-computation case.  

> Also, ASCIILiteral("") is more efficient than just passing a literal and
> letting it be converted to a string.

I will update the patch accordingly.

[youenn fablet]

Created attachment 265944 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 265944 [details]
Patch for landing

Clearing flags on attachment: 265944

Committed r192680: <http://trac.webkit.org/changeset/192680>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #3)
> I would think "set" and "add" to be equivalent both in terms of performances
> and functionality, since we are doing a contain check before doing it.

HashMap::set is always slower than HashMap::add. HashMap::set is implemented by first calling add and then optionally doing some additional work. So it can never be as fast as HashMap::add.

> "set" seems semantically more appropriate for Content-Type header, as we do
> not want any concatenation to happen.

This is a misunderstanding. HashMap::add never does any concatenation.

[youenn fablet]

(In reply to comment #7)
> (In reply to comment #3)
> > I would think "set" and "add" to be equivalent both in terms of performances
> > and functionality, since we are doing a contain check before doing it.
> 
> HashMap::set is always slower than HashMap::add. HashMap::set is implemented
> by first calling add and then optionally doing some additional work. So it
> can never be as fast as HashMap::add.
> 
> > "set" seems semantically more appropriate for Content-Type header, as we do
> > not want any concatenation to happen.
> 
> This is a misunderstanding. HashMap::add never does any concatenation.

XHR is going through HTTPHeaderMap wrapper.
HTTPHeaderMap::add does concatenation.
Inlining these methods might bring some benefit.