|Summary:||DocumentParser::appendBytes shouldn't have a "flush" boolean parameter|
|Product:||WebKit||Reporter:||Adam Barth <abarth>|
|Component:||New Bugs||Assignee:||Adam Barth <abarth>|
|Severity:||Normal||CC:||darin, dglazkov, eric, rakuco, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Adam Barth 2011-06-11 00:26:40 PDT
DocumentParser::appendBytes shouldn't have a "flush" boolean parameter
Comment 2 WebKit Review Bot 2011-06-11 00:50:10 PDT
Comment on attachment 96847 [details] Patch Attachment 96847 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8825631 New failing tests: userscripts/user-script-plugin-document.html
Comment 3 WebKit Review Bot 2011-06-11 00:50:15 PDT
Created attachment 96849 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 4 Darin Adler 2011-06-11 09:03:02 PDT
Comment on attachment 96847 [details] Patch 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.
Comment 5 Adam Barth 2011-06-11 11:56:20 PDT
> That feature should almost certainly be removed. The caller can call strlen. Yeah. That's next on my list.
Comment 7 WebKit Review Bot 2011-06-11 15:42:44 PDT
Comment on attachment 96861 [details] Patch for landing Rejecting attachment 96861 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2 Last 500 characters of output: d (64.2%) => Tests to be fixed (1221): 6 DumpRenderTree crashes ( 0.5%) 5 tests timed out ( 0.4%) 53 text diff mismatch ( 4.3%) 326 skipped (26.7%) => Tests that will only be fixed if they crash (WONTFIX) (8114): 1 test timed out ( 0.0%) 110 text diff mismatch ( 1.4%) 7954 skipped (98.0%) Regressions: Unexpected text diff mismatch : (1) userscripts/user-script-plugin-document.html = TEXT Full output: http://queues.webkit.org/results/8830414
Comment 8 WebKit Review Bot 2011-06-11 15:42:49 PDT
Created attachment 96864 [details] Archive of layout-test-results from ec2-cq-03 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: ec2-cq-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 10 WebKit Review Bot 2011-06-11 17:51:24 PDT
Comment on attachment 96866 [details] Patch for landing Clearing flags on attachment: 96866 Committed r88609: <http://trac.webkit.org/changeset/88609>
Comment 11 WebKit Review Bot 2011-06-11 17:51:34 PDT
All reviewed patches have been landed. Closing bug.