Bug 143052 – Separate entry decoding from validation

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

Factor encoding and decoding related code to a class.

[Antti Koivisto]

Created attachment 249412 [details]
patch

[WebKit Commit Bot]

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.

[Antti Koivisto]

Created attachment 249413 [details]
patch

[Chris Dumez]

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?

[Chris Dumez]

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?

[Antti Koivisto]

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

[Antti Koivisto]

https://trac.webkit.org/r181970