HTTPHeaderMap should not derive from HashMap
Created attachment 232269 [details] Patch
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?
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
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
Created attachment 232486 [details] Patch
Created attachment 232487 [details] Patch
Created attachment 232499 [details] Patch
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.
(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.
Created attachment 232664 [details] Patch
Created attachment 232665 [details] Patch
Committed r169679: <http://trac.webkit.org/changeset/169679>
(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