Factor encoding and decoding related code to a class.
Created attachment 249412 [details] patch
Attachment 249412 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:40: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 249413 [details] patch
Comment on attachment 249413 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=249413&action=review r=me with comments. > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:115 > + varyValue.split(',', false, varyingHeaderNames); Could you add an inline comment to indicate for /false/ stands for? It is really not clear unfortunately (I hate those bool arguments). > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:120 > + varyingRequestHeaders.append(std::make_pair(headerName, headerValue)); Probably not worth it but we could reserve capacity for varyingRequestHeaders. > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:248 > + return useDecision <= UseDecision::Validate; nit: I think (useDecision == Use || useDecision == Validate) would be more readable. It is only 2 values to check so it is not too long. > Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp:43 > +Entry::Entry(const Key& key, const WebCore::ResourceResponse& response, RefPtr<WebCore::SharedBuffer>&& buffer, Vector<std::pair<String, String>> varyingRequestHeaders) We probably want to pass varyingRequestHeaders as a const reference. > Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp:81 > + std::unique_ptr<Entry> entry(new Entry(storageEntry)); nit: I still prefer make_unique() and auto for the variable type. > Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp:91 > + if (!decoder.decode(hasVaryingRequestHeaders)) We may want to add a LOG() for this. > Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp:95 > + if (!decoder.decode(entry->m_varyingRequestHeaders)) We may want to add a LOG() for this. > Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp:149 > +bool Entry::asJSON(StringBuilder& json) We never return false so why bother returning a boolean? This way we don't need error handling as the call site. > Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp:151 > + json.append("{\n"); Most of those should use appendLiteral(), not append(). > Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp:176 > + json.append("}"); append('}') is better. > Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.h:56 > + const Vector<std::pair<String, String>> varyingRequestHeaders() const { return m_varyingRequestHeaders; } const Vector<std::pair<String, String>>& ? (missing ref) > Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.h:68 > + bool asJSON(StringBuilder& json); Looks like this should be a const method?
Comment on attachment 249413 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=249413&action=review >> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:248 >> + return useDecision <= UseDecision::Validate; > > nit: I think (useDecision == Use || useDecision == Validate) would be more readable. It is only 2 values to check so it is not too long. Actually, returning false here will cause the Storage to remove the entry. We probably only want to do this if the useDecision is UseDecision::NoDueToDecodeFailure?
(In reply to comment #4) > Comment on attachment 249413 [details] > nit: I still prefer make_unique() and auto for the variable type. Me too, unfortunately make_unique does not work with private constructor and I think that is a preferable feature here. > We never return false so why bother returning a boolean? This way we don't > need error handling as the call site. Good point.
https://trac.webkit.org/r181970