[Webkit-unassigned] [Bug 73533] Upstream the Blackberry porting of multipart delegate

[bugzilla-daemon at webkit.org]

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





--- Comment #2 from Leo Yang <leo.yang at torchmobile.com.cn>  2011-12-01 01:22:47 PST ---
(From update of attachment 117369)
View in context: https://bugs.webkit.org/attachment.cgi?id=117369&action=review

This is an informal review.

> Source/WebCore/ChangeLog:8
> +
> +        No new tests. (OOPS!)

Please explain why no new tests.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:57
> +static TRIMPOSITIONS trimStringT(const std::string& input,
> +                                 const std::string::value_type trimChars[],
> +                                 TRIMPOSITIONS positions,
> +                                 std::string& output)

Why name it trimStringT? Why T?

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:111
> +static std::string getSpecificHeader(const std::string& headers, const std::string& name)
> +{
> +    // We want to grab the Value from the "Key: Value" pairs in the headers,
> +    // which should look like this (no leading spaces, \n-separated) (we format
> +    // them this way in url_request_inet.cc):
> +    //    HTTP/1.1 200 OK\n
> +    //    ETag: "6d0b8-947-24f35ec0"\n
> +    //    Content-Length: 2375\n
> +    //    Content-Type: text/html; charset=UTF-8\n
> +    //    Last-Modified: Sun, 03 Sep 2006 04:34:43 GMT\n
> +    if (headers.empty())
> +        return std::string();
> +
> +    std::string match('\n' + name + ':');

So you aren't handling 'name : value' case (one or more white space after name)? Should we support this?

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:138
> +static size_t findStringEnd(const std::string& line, size_t start, char delim)
> +{

Minor: delim --> delimiter.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:139
> +    const char set[] = { delim, '\\', '\0' };

Ditto.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:161
> +    // NOTREACHED();

Add ASSERT_NOT_REACHED()?

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:170
> +        const char delimStr[] = { delimiter, '"', '\'', '\0' };

delimStr --> delimiterSet.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:172
> +        if (curDelimPos == std::string::npos)

curDelimPos --> currentDelimiterPosition.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:200
> +static void parseContentType(const std::string& contentTypeStr,
> +                             std::string& mimeType, std::string& charset,
> +                             bool& hadCharset)

contentTypeStr --> contentType.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:211
> +    size_t charsetVal = 0;

charsetVal --> charsetValue.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:216
> +    size_t paramStart = contentTypeStr.find_first_of(';', typeEnd);

paramStart --> parameterStart.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:221
> +            size_t curParamEnd = findDelimiter(contentTypeStr, curParamStart, ';');

curParamEnd --> currentParameterEnd.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:223
> +            size_t paramNameStart = contentTypeStr.find_first_not_of(" \t", curParamStart);

paramNameStart --> parameterNameStart

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:226
> +            static const char charsetStr[] = "charset=";

charsetStr --> charsetString.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:252
> +        } else {
> +            charsetEnd = std::min(contentTypeStr.find_first_of(" \t" ";(", charsetVal),
> +                                   charsetEnd);
> +        }

Wrap the std::min call to one line and remove curve brackets?

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:255
> +    // if the server sent "*/*", it is meaningless, so do not store it.

if --> If

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:269
> +        bool eq = !mimeType.empty()
> +                  && lowerCaseEqualsASCII(contentTypeStr.begin() + typeVal,
> +                                          contentTypeStr.begin() + typeEnd,
> +                                          mimeType.data());

eq --> equation.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:272
> +            mimeType.assign(contentTypeStr.begin() + typeVal,
> +                              contentTypeStr.begin() + typeEnd);

Wrap to one line or fix indent issue.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:278
> +            charset.assign(contentTypeStr.begin() + charsetVal,
> +                            contentTypeStr.begin() + charsetEnd);

Ditto.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:287
> +static int pushOverLine(const std::string& data, size_t pos)
> +{

pos --> position.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:338
> +void MultipartResponseDelegate::onReceivedData(const char* data,
> +                                               int datalen,
> +                                               int encodedDataLength)
> +{

datalen --> length.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:371
> +        int pos = pushOverLine(m_data, 0);

pos --> position.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:384
> +    size_t boundaryPos;

boundaryPos --> boundaryPosition.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:389
> +            int datalength = boundaryPos;

datalength --> dataLength.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:404
> +        size_t boundaryEndPos = boundaryPos + m_boundary.length();

boundaryEndPos --> boundaryEndPosition.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:459
> +    size_t lineStartPos = 0;

lineStartPos --> lineStartPosition.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:460
> +    size_t lineEndPos = m_data.find('\n');

lineEndPos --> lineEndPosition.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:505
> +        bool isNeedAdd = true;

isNeedAdd --> needsAdd.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:521
> +            response.setHTTPHeaderField(WTF::String::fromUTF8(name.data()),
> +                                        WTF::String::fromUTF8(value.data()));

WTF::String --> String.

> Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:538
> +    size_t boundaryPos = m_data.find(m_boundary);

boundaryPos --> boundaryPosition.

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