[webkit-reviews] review denied: [Bug 22949] Refactor HTMLFormElement code : [Attachment 26179] Updated patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Dec 21 09:54:14 PST 2008


Darin Adler <darin at apple.com> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 22949: Refactor HTMLFormElement code
https://bugs.webkit.org/show_bug.cgi?id=22949

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

------- Additional Comments from Darin Adler <darin at apple.com>
I think the basic idea on this patch is OK, but making the data used to build
form data into a member of HTMLFormElement is a step backward. Your troubles
with the lifetime of the contents of m_encodedData and m_multiPartData
demonstrate this. It's just not good to take local data and turn it into a data
member.

There are plenty of ways around this. One is to pass the vectors in to all the
relevant functions. The other is to make this two classes instead of one.

> + * Copyright (C) 2008 Nikolas Zimmermann
<nikolas.zimmermann at torchmobile.com>

When you're grabbing a bunch of existing code and refactoring it, you should
not assume that the original copyright holders are relinquishing copyright on
all the code. You need to list the appropriate original authors here too.

> +static inline void appendString(Vector<char>& buffer, const char string)
> +{
> +    buffer.append(string);
> +}

It's strange to call this function "appendString" since it appends a character,
not a string. The "string" in "appendString" is what's being appended, not what
you're appending to. Maybe we should just name the helper function "append"
instead of "appendString". Also, the type of the argument should be "char", not
"const char".

> +    static const char s_hexDigits[17] = "0123456789ABCDEF";
> +
> +    // Same safe characters as Netscape for compatibility.
> +    static const char s_safeCharacters[] = "-._*";

I don't think these s_ prefixes add anything; since these are so local I think
it reads great to just use normal names. I also think the code read well when
the definitions were right where these things were used.

> +    // The RFC 2046 spec says the AlphaNumeric characters plus the following
characters

The word "alphanumeric" does not have a capital "n" nor should it be
capitalized.

> +    // are legal for boundaries:  '()+_,-./:=?
> +    // However the following characters, though legal, cause some sites to
fail:
> +    // (),./:=
> +    // http://bugs.webkit.org/show_bug.cgi?id=13352
> +    static const char s_alphaNumericEncodingMap[64] =
> +    {
> +	 0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48,

Braces go on the same line as the definition.

Indentation is 4 spaces, not 2 spaces.

> +    bool isPostMethod() const { return m_isPostMethod; }
> +    void setIsPostMethod(bool value) { m_isPostMethod = value; }
> +
> +    bool isMultiPartForm() const { return m_isMultiPartForm; }
> +    void setIsMultiPartForm(bool value) { m_isMultiPartForm = value; }
> +
> +    String encodingType() const { return m_encodingType; }
> +    void setEncodingType(const String& value) { m_encodingType = value; }
> +
> +    String acceptCharset() const { return m_acceptCharset; }
> +    void setAcceptCharset(const String& value) { m_acceptCharset = value; }
> +
> +    void parseEncodingType(const String&);
> +    void parseMethodType(const String&);
> +
> +    TextEncoding dataEncoding(Document*) const;
> +
> +    // Helper functions used by HTMLFormElement/WMLGoElement for multi-part
form data
> +    void beginMultiPartHeader(Vector<char>&, const CString& boundary, const
CString& name) const;
> +    void addBoundaryToMultiPartHeader(Vector<char>& header, const CString&
boundary, bool isLastBoundary = false) const;
> +    void addFileNameToMultiPartHeader(Vector<char>&, const TextEncoding&
encoding, const String& fileName) const;
> +    void addContentTypeToMultiPartHeader(Vector<char>&, const CString&
mimeType) const;
> +    void finishMultiPartHeader(Vector<char>&) const;
> +
> +    // Functions used by HTMLFormElement/WMLGoElement for non multi-part
form data
> +    void addKeyValuePairAsFormData(Vector<char>&, const CString& key, const
CString& value) const;
> +
> +    // Encode the input string as HTML form compatible data
> +    void encodeAsFormData(Vector<char>&, const CString&) const;
> +
> +    // Returns a 0-terminated C string in the vector
> +    void generateUniqueBoundaryString(Vector<char>&) const;

Can any of these functions be private? When you make a new class with so many
public functions and so few private ones, I worry that the architecture isn't
right.

There's no need for the name "encoding" for the const TextEncoding& argument.

Do we use the spelling "fileName" or "filename" in WebKit? It looks like you
preferred "fileName", treating it as two separate words, and renamed many
things from "fileName" to "filename" but I see "filename" in all sorts of other
places in the WebKit code; maybe we should standardize on that.

I originally preferred "fileName", but now I'm leaning toward "filename".

I'm going to say review- because I don't think it's good to add two Vector
members to HTMLFormElement that used to be local variables.


More information about the webkit-reviews mailing list