Bug 62499

Summary: DocumentParser::appendBytes shouldn't have a "flush" boolean parameter
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dglazkov, eric, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch for landing
none
Archive of layout-test-results from ec2-cq-03
none
Patch for landing none

Description Adam Barth 2011-06-11 00:26:40 PDT
DocumentParser::appendBytes shouldn't have a "flush" boolean parameter
Comment 1 Adam Barth 2011-06-11 00:30:34 PDT
Created attachment 96847 [details]
Patch
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 6 Adam Barth 2011-06-11 14:57:06 PDT
Created attachment 96861 [details]
Patch for landing
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 9 Adam Barth 2011-06-11 17:12:31 PDT
Created attachment 96866 [details]
Patch for landing
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.