[Webkit-unassigned] [Bug 117735] [curl] Improve multipart response handling

[bugzilla-daemon at webkit.org]

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


Brent Fulgham <bfulgham at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #206765|review?                     |review-
               Flag|                            |




--- Comment #6 from Brent Fulgham <bfulgham at webkit.org>  2013-07-23 10:18:40 PST ---
(From update of attachment 206765)
View in context: https://bugs.webkit.org/attachment.cgi?id=206765&action=review

Overall this looks good.  There are a few minor style problems that need to be corrected.  I'm also not sure about the meaning of 'strict' header parsing.  It would be helpful to know if there are documented rules for when something should be parsed strictly or not.

> Source/WebCore/platform/network/HTTPParsers.cpp:608
> +size_t parseHTTPHeader(const char* start, size_t length, String& failureReason, AtomicString& nameStr, String& valueStr, bool strict)

Who governs whether we are parsing in 'strict' mode or not?  Is this documented by an RFC?

> Source/WebCore/platform/network/curl/MultipartHandle.cpp:37
> +bool MultipartHandle::extractBoundary(const String &contentType, String &boundary)

We write this as "const String& contentType" and "String& boundary".  The reference is part of the type, not the argument name.

> Source/WebCore/platform/network/curl/MultipartHandle.cpp:77
> +bool MultipartHandle::matchForBoundary(const char *data, size_t pos, size_t &matched)

We write this as "const char* data" and "size_t& matched".  The pointer (and reference) token is part of the type, not the argument name.

> Source/WebCore/platform/network/curl/MultipartHandle.cpp:81
> +    for (size_t i = 0; i < m_boundaryLength; ++i)

When the contents of a for statement spans multiple lines, I prefer to see them wrapped in brackets to aid readability:

for (size_t i = 0; i < m_boundaryLength; ++i) {

> Source/WebCore/platform/network/curl/MultipartHandle.cpp:85
> +        }

... and close here.

> Source/WebCore/platform/network/curl/MultipartHandle.cpp:91
> +bool MultipartHandle::checkForBoundary(size_t &boundaryStart, size_t &lastPartialMatch)

Argument names...

> Source/WebCore/platform/network/curl/MultipartHandle.cpp:94
> +    const char *content = m_buffer.data();

const char* content...

> Source/WebCore/platform/network/curl/MultipartHandle.cpp:116
> +    const char *content = m_buffer.data();

const char* content ...

> Source/WebCore/platform/network/curl/MultipartHandle.cpp:119
> +        return false; // Not enough data, skipping for now.

This comment isn't helpful; go ahead and remove it.

> Source/WebCore/platform/network/curl/MultipartHandle.cpp:136
> +    for (; p < end; p++) {

It's good to be in the habit of using prefix operator ++/--, since there is a performance benefit to using them for iterators.

for (; p < end; ++p) {

> Source/WebCore/platform/network/curl/MultipartHandle.cpp:156
> +void MultipartHandle::contentReceived(const char *data, size_t length)

const char* data

> Source/WebCore/platform/network/curl/MultipartHandle.cpp:182
> +*/

Nice comment!

> Source/WebCore/platform/network/curl/MultipartHandle.cpp:293
> +    return true; // There is still things to process, so go for it.

This *is* still *a* thing, or There *are* still things.

> Source/WebCore/platform/network/curl/MultipartHandle.cpp:322
> +        const char *data = m_buffer.data();

const char* data

> Source/WebCore/platform/network/curl/MultipartHandle.h:40
> +    static bool extractBoundary(const String &contentType, String &boundary);

&'s should be connected to the type declarations, not the argument names.

> Source/WebCore/platform/network/curl/MultipartHandle.h:42
> +    MultipartHandle(ResourceHandle* handle, const String &boundary)

Ditto.

> Source/WebCore/platform/network/curl/MultipartHandle.h:68
> +    bool matchForBoundary(const char *data, size_t pos, size_t &matched);

&'s (and *'s) should be connected to the type declarations, not the argument names.

> Source/WebCore/platform/network/curl/MultipartHandle.h:69
> +    inline bool checkForBoundary(size_t &boundaryStart, size_t &lastPartialMatch);

&'s should be connected to the type declarations, not the argument names.

> Source/WebCore/platform/network/curl/MultipartHandle.h:73
> +    ResourceHandle *m_resourceHandle;

* should be part of the type.

Since ResourceHandle is not 'owned' by this object, and the MultipartHandle cannot exist without a resource handle, I wonder if this should be a reference?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.