Following on bug 147784, XHR should use HTTPHeaderName::ContentType in lieu of "Content-Type"
Created attachment 265854 [details] Patch
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.
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.
Created attachment 265944 [details] Patch for landing
Comment on attachment 265944 [details] Patch for landing Clearing flags on attachment: 265944 Committed r192680: <http://trac.webkit.org/changeset/192680>
All reviewed patches have been landed. Closing bug.
(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.
(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.