[webkit-reviews] review granted: [Bug 62499] DocumentParser::appendBytes shouldn't have a "flush" boolean parameter : [Attachment 96847] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jun 11 09:03:01 PDT 2011


Darin Adler <darin at apple.com> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 62499: DocumentParser::appendBytes shouldn't have a "flush" boolean
parameter
https://bugs.webkit.org/show_bug.cgi?id=62499

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=96847&action=review

> Source/WebCore/ChangeLog:10
> +	   some code in DocumentWriter look less rediculous.

ridiculous

> Source/WebCore/dom/DecodedDataDocumentParser.cpp:55
> +    String renamingData = writer->createDecoderIfNeeded()->flush();

remainingData

> Source/WebCore/loader/DocumentWriter.cpp:194
> +    if (m_receivedData)
> +	   return;

This shows that the name, “received data” is not good for a boolean data
member. I read this as “if I received data, then return”, which is nonsense.

The data member needs a name like m_hasReceivedData or maybe an even clearer
name. Maybe m_hasReceivedFirstData or m_hasReceivedSomeData?

> Source/WebCore/loader/DocumentWriter.cpp:204
> +    if (length == -1)
> +	   length = strlen(bytes);

The result of strlen does not fit into int. If someone passes a string with
more than 2^31 bytes in it, we will get the length wrong. Seems possible on a
64-bit system.

> Source/WebCore/loader/DocumentWriter.h:54
> -    void addData(const char* string, int length = -1, bool flush = false);
> +    void addData(const char* bytes, int length = -1);

The name “string” makes sense given the fact that the function will call strlen
if the magic number -1 is used for length. That feature should almost certainly
be removed. The caller can call strlen.


More information about the webkit-reviews mailing list