Bug 62499 – DocumentParser::appendBytes shouldn't have a "flush" boolean parameter

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

DocumentParser::appendBytes shouldn't have a "flush" boolean parameter

[Adam Barth]

Created attachment 96847 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Darin Adler]

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.

[Adam Barth]

> That feature should almost certainly be removed. The caller can call strlen.

Yeah.  That's next on my list.

[Adam Barth]

Created attachment 96861 [details]
Patch for landing

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Adam Barth]

Created attachment 96866 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 96866 [details]
Patch for landing

Clearing flags on attachment: 96866

Committed r88609: <http://trac.webkit.org/changeset/88609>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.