[Webkit-unassigned] [Bug 143052] Separate entry decoding from validation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=143052

Chris Dumez <cdumez at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #249413|review?                     |review+
              Flags|                            |

--- Comment #4 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 249413
  --> https://bugs.webkit.org/attachment.cgi?id=249413
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150325/4b1be99e/attachment-0002.html>