Bug 133392 – HTTPHeaderMap should not derive from HashMap

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Anders Carlsson]

HTTPHeaderMap should not derive from HashMap

[Anders Carlsson]

Created attachment 232269 [details]
Patch

[Geoffrey Garen]

Comment on attachment 232269 [details]
Patch

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

r=me

> Source/WebCore/loader/DocumentLoader.cpp:608
> -    DEPRECATED_DEFINE_STATIC_LOCAL(AtomicString, xFrameOptionHeader, ("x-frame-options", AtomicString::ConstructFromLiteral));
> -
> -    auto it = response.httpHeaderFields().find(xFrameOptionHeader);
> +    auto it = response.httpHeaderFields().find("x-frame-options");
>      if (it != response.httpHeaderFields().end()) {

Why don't we like static local AtomicString here?

[Build Bot]

Comment on attachment 232269 [details]
Patch

Attachment 232269 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4631480315674624

New failing tests:
media/W3C/video/preload/preload_property_exists.html

[Build Bot]

Created attachment 232275 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Anders Carlsson]

Created attachment 232486 [details]
Patch

[Anders Carlsson]

Created attachment 232487 [details]
Patch

[Anders Carlsson]

Created attachment 232499 [details]
Patch

[Darin Adler]

Comment on attachment 232499 [details]
Patch

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::HashTraits<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.

[Anders Carlsson]

(In reply to comment #8)
> (From update of attachment 232499 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232499&action=review
> 
> > 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.

I didn't want to move this since it's going to be replaced with a version that takes an enum.

> Seems like we want to make this one inline.

Same thing here.

> 
> > Source/WebCore/platform/network/HTTPHeaderMap.h:49
> > +    HTTPHeaderMap();
> > +    ~HTTPHeaderMap();
> 
> Why are we declaring/defining these? To make sure they aren’t inlined perhaps?

Yup.

> 
> > 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.

I want it to be inside the class, so it does need to be a friend.

[Anders Carlsson]

Created attachment 232664 [details]
Patch

[Anders Carlsson]

Created attachment 232665 [details]
Patch

[Anders Carlsson]

Committed r169679: <http://trac.webkit.org/changeset/169679>

[Csaba Osztrogonác]

(In reply to comment #12)
> Committed r169679: <http://trac.webkit.org/changeset/169679>

It broke the WinCairo build:
   1>C:\Projects\BuildSlave\win-cairo-release\build\Source\WebCore\platform\network\curl\ResourceRequest.h(81): error C2027: use of undefined type 'WTF::PassOwnPtr<WebCore::CrossThreadResourceRequestData>' (..\loader\FrameLoadRequest.cpp)
     1>C:\Projects\BuildSlave\win-cairo-release\build\Source\WebCore\platform\network\curl\ResourceRequest.h(81): error C2027: use of undefined type 'WTF::PassOwnPtr<WebCore::CrossThreadResourceRequestData>' (..\loader\ResourceLoader.cpp)
     1>C:\Projects\BuildSlave\win-cairo-release\build\Source\WebCore\platform\network\curl\ResourceRequest.h(81): error C2027: use of undefined type 'WTF::PassOwnPtr<WebCore::CrossThreadResourceRequestData>' (..\loader\NetscapePlugInStreamLoader.cpp)
     1>C:\Projects\BuildSlave\win-cairo-release\build\Source\WebCore\platform\network\curl\ResourceRequest.h(81): error C2027: use of undefined type 'WTF::PassOwnPtr<WebCore::CrossThreadResourceRequestData>' (..\loader\CrossOriginAccessControl.cpp)
     1>C:\Projects\BuildSlave\win-cairo-release\build\Source\WebCore\platform\network\curl\ResourceRequest.h(81): error C2027: use of undefined type 'WTF::PassOwnPtr<WebCore::CrossThreadResourceRequestData>' (..\loader\DocumentThreadableLoader.cpp)
     1>C:\Projects\BuildSlave\win-cairo-release\build\Source\WebCore\platform\network\curl\ResourceRequest.h(81): error C2027: use of undefined type 'WTF::PassOwnPtr<WebCore::CrossThreadResourceRequestData>' (..\loader\ImageLoader.cpp)
     1>C:\Projects\BuildSlave\win-cairo-release\build\Source\WebCore\platform\network\curl\ResourceRequest.h(81): error C2027: use of undefined type 'WTF::PassOwnPtr<WebCore::CrossThreadResourceRequestData>' (..\page\DragController.cpp)
  
...

cc-ing WinCairo maintainers