[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