[webkit-reviews] review granted: [Bug 133392] HTTPHeaderMap should not derive from HashMap : [Attachment 232499] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 133392: HTTPHeaderMap should not derive from HashMap
https://bugs.webkit.org/show_bug.cgi?id=133392

Attachment 232499: Patch
https://bugs.webkit.org/attachment.cgi?id=232499&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=232499&action=review


> Source/WebCore/platform/network/HTTPHeaderMap.cpp:62
> +	   m_headers.set(std::move(header.first), std::move(header.second));

Should use add instead of set here.

> Source/WebCore/platform/network/HTTPHeaderMap.cpp:104
>  String HTTPHeaderMap::get(const char* name) const
>  {
> -    const_iterator i = find<CaseFoldingCStringTranslator>(name);
> -    if (i == end())
> +    auto it = find(name);
> +    if (it == end())
>	   return String();
> -    return i->value;
> +    return it->value;
>  }

Seems like we should make this one inline.

> Source/WebCore/platform/network/HTTPHeaderMap.cpp:109
>  bool HTTPHeaderMap::contains(const char* name) const
>  {
> -    return find<CaseFoldingCStringTranslator>(name) != end();
> +    return find(name) != end();
> +}

Seems like we want to make this one inline.

> Source/WebCore/platform/network/HTTPHeaderMap.h:49
> +    HTTPHeaderMap();
> +    ~HTTPHeaderMap();

Why are we declaring/defining these? To make sure they aren’t inlined perhaps?

> Source/WebCore/platform/network/HTTPHeaderMap.h:88
> +    friend bool operator!=(const HTTPHeaderMap& a, const HTTPHeaderMap& b)
> +    {
> +	   return !(a == b);
> +    }

This doesn’t need to be a friend.

> Source/WebCore/platform/network/HTTPHeaderMap.h:91
>      // FIXME: Not every header fits into a map. Notably, multiple Set-Cookie
header fields are needed to set multiple cookies.

Seems like this comment belongs before the class, not in the private section.

> Source/WebCore/platform/network/ResourceRequestBase.h:36
>  #include <wtf/OwnPtr.h>

Why are we keeping this?

> Source/WebCore/platform/network/ResourceRequestBase.h:37
> +#include <wtf/PassOwnPtr.h>

Why are we adding this?

> Source/WebKit/win/WebURLResponse.cpp:363
> +    HashMap<String, String, CaseFoldingHash> fields;

1>..\..\win\WebURLResponse.cpp(367): error C2664:
'COMPropertyBag<WTF::String,WTF::AtomicString,WTF::CaseFoldingHash>
*COMPropertyBag<WTF::String,WTF::AtomicString,WTF::CaseFoldingHash>::adopt(WTF:
:HashMap<WTF::AtomicString,WTF::String,WTF::CaseFoldingHash,WTF::HashTraits<WTF
::AtomicString>,WTF::HashTraits<WTF::String>> &)' : cannot convert argument 1
from
'WTF::HashMap<WTF::String,WTF::String,WTF::CaseFoldingHash,WTF::HashTraits<WTF:
:String>,WTF::HashTraits<WTF::String>>' to
'WTF::HashMap<WTF::AtomicString,WTF::String,WTF::CaseFoldingHash,WTF::HashTrait
s<WTF::AtomicString>,WTF::HashTraits<WTF::String>> &'

> Source/WebKit/win/WebURLResponse.cpp:365
> +	   fields.set(keyValuePair.key, keyValuePair.value);

Should use add instead of set here.