[webkit-reviews] review denied: [Bug 73533] Upstream the Blackberry porting of multipart delegate : [Attachment 118353] patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Chris.Guan
<chris.guan at torchmobile.com.cn>'s request for review:
Bug 73533: Upstream the Blackberry porting of multipart delegate
https://bugs.webkit.org/show_bug.cgi?id=73533

Attachment 118353: patch 
https://bugs.webkit.org/attachment.cgi?id=118353&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
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 ";