Bug 73533

Created attachment 117369 [details] Patch

Comment on attachment 117369 [details] Patch 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.

Created attachment 117589 [details] Patch

Why is this needed in WebCore? Isn't this provided by the network layer?

(In reply to comment #4) > Why is this needed in WebCore? Isn't this provided by the network layer? NetworkJob on Blackberry port in NetWorkManager of WebCore used it. BlackBerry dose not define the public Apis for NetworkJob handled sendResponseIfNeeded(). so It makes sense to keep it in WebCore. NetworkJob is upstreaming soon.

Comment on attachment 117589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117589&action=review > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:128 > + const char* b) b does not seem very descriptive here. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.h:95 > + // true until we get our first on received data call "Make comments look like sentences by starting with a capital letter and ending with a period (punctation)." > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.h:102 > + // true if we're truncated in the middle of a header Ditto. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.h:108 > + // processing AddData requests. Ditto.

Created attachment 118314 [details] Patch

Created attachment 118318 [details] patch fix Rob's comments

Comment on attachment 118318 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=118318&action=review Informal review. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:219 > + size_t curParamStart = parameterStart + 1; curParamStart --> currentParamterStart And the corresponding usages should be updated. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:351 > + // Eat leading \r\n You are missing a '.' at the end. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:354 > + int pos = pushOverLine(m_data, 0); > + if (pos) > + m_data = m_data.substr(pos); pos --> position > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:372 > + int pos = pushOverLine(m_data, 0); > + if (pos) > + m_data = m_data.substr(pos); pos --> position. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:415 > + // Ok, back to parsing headers You are missing a '.' at the end. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:469 > + // A blank line, end of headers You are missing a '.' at the end. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:481 > + // Eat headers Ditto. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.h:76 > + // Checks to see if data[pos] character is a line break; handles crlf, lflf, > + // lf, or cr. Returns the number of characters to skip over (0, 1 or 2). > + What's this for?

Created attachment 118353 [details] patch fix Leo's comments

Comment on attachment 118353 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=118353&action=review I wonder if we could convert std::string to WTF::String and then utilize WTF::String for more of the string manipulation code instead of rolling our own around std::string. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:2 > + Nit: Remove this empty line. It doesn't help improve the readability of the license information in this file. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:23 > +// That gcc wants both of these prototypes seems mysterious. VC, for > +// its part, can't decide which to use (another mystery). Matching of > +// template overloads: the final frontier. > +#ifndef _MSC_VER > +template <typename T, size_t N> > +char (&ArraySizeHelper(const T (&array)[N]))[N]; > +#endif > + > +#define arraysize(array) (sizeof(ArraySizeHelper(array))) This code is a similar to the code for WTF_ARRAY_LENGTH() in JavaScriptCore/wtf/StdLibExtras.h: <http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/StdLibExtras.h?rev=97675#L142>. We should #include <wtf/StdLibExtras.h> and use WTF_ARRAY_LENGTH() instead of duplicating its functionality in this file. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:52 > +enum TRIMPOSITIONS { > + TRIM_NONE = 0, > + TRIM_LEADING = 1 << 0, > + TRIM_TRAILING = 1 << 1, > + TRIM_ALL = TRIM_LEADING | TRIM_TRAILING, > +}; The enum name and its values don't conform to the naming conventions described in the WebKit Code Style Guidelines. Also, I suggest making the name of this enum singular (i.e. TrimPosition) to improve the readability of the code when used. On another note, we seem to be using variables of type TRIMPOSITIONS as a bitmask instead of as a enum since we bitwise-OR enum values both on line 51 and 83. Instead, I suggest we define an anonymous enum with values TrimNone, TrimLeading, and TrimTrailing. Then add "typedef unsigned TrimPosition;" and define a "const unsigned TrimLeadingAndTrailing = TrimLeading | TrimTrailing;". By taking this approach we don't get into a position where a variable has a static type of an enum but its value is a bitwise-OR of enum values (i.e. doesn't correspond to a single enum value) should there not exist an enum value with such a composition. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:57 > +static TRIMPOSITIONS trimString(const std::string& input, > + const std::string::value_type trimChars[], > + TRIMPOSITIONS positions, > + std::string& output) The indentation of lines 55 through 57 appears to be off by a space. That being said, we usually write function signatures on one line. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:59 > + // Find the edges of leading/trailing whitespace as desired. This comment is incorrect. Depending on the characters we are trimming (i.e. contents of trimChars) we may or may not be trimming white space characters. Moreover, this comment doesn't add much value even it were correct. Hence, I suggest removing this comment. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:64 > + const std::string::size_type lastChar = input.length() - 1; > + const std::string::size_type firstGoodChar = (positions & TRIM_LEADING) ? > + input.find_first_not_of(trimChars) : 0; > + const std::string::size_type lastGoodChar = (positions & TRIM_TRAILING) ? > + input.find_last_not_of(trimChars) : lastChar; Nit: Instead of repeating std::string::size_type, I suggest adding the declaration "using std::string::size_type" in the body of this function. Then you can reference this type as size_type. The parentheses on line 61 and 63 aren't necessary because the bitwise AND (&) operator has a higher precedence than the ternary operator (?:). Also, I would write lines 61-62 on one line instead of breaking it over two lines. Similarly, I would do the same for lines 63-64. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:68 > + // When the string was all whitespace, report that we stripped off whitespace > + // from whichever position the caller was interested in. For empty input, we > + // stripped no whitespace, but we still need to clear |output|. This comment doesn't explain anything that couldn't otherwise be understood from reading the code below. Therefore, I suggest we remove this comment. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:71 > + || (firstGoodChar == std::string::npos) > + || (lastGoodChar == std::string::npos)) { Nit: The inner parentheses are not necessary since the == operator has higher precedence than the || operator. Also, I would write this on one line instead of breaking the if-statement condition over three lines. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:77 > + // Trim the whitespace. Please remove this comment as it's inane. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:80 > + // Return where we trimmed from. Remove this comment since it's inane. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:96 > +static const char WhitespaceASCII[] = { > + // <control-0009> to <control-000D> > + 0x09, > + 0x0A, > + 0x0B, > + 0x0C, > + 0x0D, > + // Space > + 0x20, > + 0 > +}; This variable is only used in getSpecificHeader() below. Hence, I suggest moving this definition into that function. Moreover, I would write it as const C-String instead of a mutable array of characters because we aren't changing this list of characters at run-time and by declaring this as a C-String we can omit the trailing null character (which isn't a character we actually want to trim but is present as artifact of our usage of std::string::{find_first_not_of, find_last_not_of}(). In getSpecificHeader(), I would write: static const char* WhitespaceASCII = "\t\n\v\f\r ";

Created attachment 118543 [details] patch fix Daniel's nice comments.

Comment on attachment 118543 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=118543&action=review Hm, I'm stopping my initial review with a question: Do we have the time to refactor this right now to avoid std::string. I don't want to delay the initial upstreaming, hence I'm asking. If we agree to clean it up later, no problem with me, but maybe with others :-) > Source/WebCore/ChangeLog:9 > + Test cases: > + http://avhs.axis.com This needs a more detailed ChangeLog, at least one or two descriptive lines. Also listing this testcase doesn't help a lot. > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:4 > +// Copyright (C) 2011 Research In Motion Limited. All rights reserved. > +// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. > +// Use of this source code is governed by a BSD-style license that can be > +// found in the LICENSE file. We normally include licenses inline - why is it different here? Can you change it? > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:15 > +static bool compareChar(char x, char y) For example: s/compareChar/doCharactersMatchCaseInsensitive/. Anything descriptive should be used, compareChar is mystic, what does it do if you read this name on a call-site? :-) > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:24 > +static void stringToLowerASCII(std::string& s) > +{ > + for (std::string::iterator i = s.begin(); i != s.end(); ++i) > + *i = toASCIILower(*i); > +} This looks very slow. IMHO the whole code should move away from std::string. I'd highly recommend to convert this to WTF::String natively.

(In reply to comment #13) > (From update of attachment 118543 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118543&action=review > > Hm, I'm stopping my initial review with a question: Do we have the time to refactor this right now to avoid std::string. I don't want to delay the initial upstreaming, hence I'm asking. If we agree to clean it up later, no problem with me, but maybe with others :-) I sent you an email to explain it. I think we can keep std::string in initial upstream and refactor later.:) > > > Source/WebCore/ChangeLog:9 > > + Test cases: > > + http://avhs.axis.com > > This needs a more detailed ChangeLog, at least one or two descriptive lines. Also listing this testcase doesn't help a lot. yes, it needs a username and password, I am not sure that I can mark them in it. So I may remove this test cases in initial upstream. agree? > > > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:4 > > +// Copyright (C) 2011 Research In Motion Limited. All rights reserved. > > +// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. > > +// Use of this source code is governed by a BSD-style license that can be > > +// found in the LICENSE file. > > We normally include licenses inline - why is it different here? Can you change it? my multipart code referred to Google chromium's code, so I kept the license. please instruct me how to change it? > > > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:15 > > +static bool compareChar(char x, char y) > > For example: s/compareChar/doCharactersMatchCaseInsensitive/. Anything descriptive should be used, compareChar is mystic, what does it do if you read this name on a call-site? :-) yes, agree, thanks. > > > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:24 > > +static void stringToLowerASCII(std::string& s) > > +{ > > + for (std::string::iterator i = s.begin(); i != s.end(); ++i) > > + *i = toASCIILower(*i); > > +} > > This looks very slow. > IMHO the whole code should move away from std::string. I'd highly recommend to convert this to WTF::String natively. Yes, refactor later.

Created attachment 118932 [details] Patch

Created attachment 119979 [details] Patch

Comment on attachment 119979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119979&action=review Looks good, some variable names need to be fixed though. > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:341 > + bool needsAdd = true; Can you come up with a clearer name? > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:342 > + int replaceHedearIndex = 0; I think replaceHeaderIndex is meant here.

Created attachment 120128 [details] Patch

Comment on attachment 120128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120128&action=review Looks great, just fix the one nit before landing :) > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:345 > + needsCopyfromOriginalResponse= false; needsCopyfromOriginalResponse = false;

Comment on attachment 120128 [details] Patch I'll help fix the last comment and land it manually.

Committed r103487: <http://trac.webkit.org/changeset/103487>