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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Dec 8 13:18:57 PST 2011


Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
 Attachment #118353|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |

--- Comment #11 from Daniel Bates <dbates at webkit.org>  2011-12-08 13:18:56 PST ---
(From update of attachment 118353)
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
> +    TRIM_NONE     = 0,
> +    TRIM_LEADING  = 1 << 0,
> +    TRIM_TRAILING = 1 << 1,
> +};

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 ";

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

More information about the webkit-unassigned mailing list