Bug 41228

Summary: Fix http/tests/local/blob/send-data-blob.html on Windows
Product: WebKit Reporter: Kinuko Yasuda <kinuko>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: jianli, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch jianli: review+

Description Kinuko Yasuda 2010-06-25 14:35:11 PDT
http/tests/local/blob/send-data-blob.html test is failing on Windows.
Comment 1 Kinuko Yasuda 2010-06-25 15:03:58 PDT
Created attachment 59801 [details]
Patch
Comment 2 Jian Li 2010-06-28 11:25:22 PDT
Comment on attachment 59801 [details]
Patch

LayoutTests/ChangeLog:7
 +          Also fix test expectations for Windows.
Please add an empty line before this line.

WebCore/ChangeLog:7
 +          Fix line-ending conversion code.  Seems like I made a bad fix.
Please add an empty line before this line. Also might be better to say something like:
  Fix a regression bug in line-ending conversion code.

WebCore/platform/BlobItem.cpp:157
 +                      calculatedLength += (endingLength - 2);
Better to add more comment here. Like:
  // Turn CRLF into CR or LF if the line ending is set to CR or LF.
Please also add the comment in other appropriate places.

The calculation logic is kind of hard to understand with regard to "(endingLength - 2)" could be a negative value. This should be the reason we keep having regression problems here. I am thinking it might be better to go back to the original way of doing the calculation. That is, we do not set the calculatedLength to the string length first and then adjust it. Instead, we set it to 0 and then add up to it.

WebCore/platform/BlobItem.cpp:158
 +                      ++needFix;
It seems to better to define needFix as a boolean.

WebCore/platform/BlobItem.cpp:185
 +              } else if (ending != EndingCR) {
What are you going to do with else part? Do we need to copy the '\r' character?
Comment 3 Kinuko Yasuda 2010-06-28 14:42:18 PDT
Created attachment 59943 [details]
Patch
Comment 4 Kinuko Yasuda 2010-06-28 14:48:19 PDT
I rewrote the convertToCString function with single-purpose three functions that just convert line-endings into CRLF, CR and LF respectively.

(In reply to comment #2)
> (From update of attachment 59801 [details])
> LayoutTests/ChangeLog:7
>  +          Also fix test expectations for Windows.
> Please add an empty line before this line.

Done.

> WebCore/ChangeLog:7
>  +          Fix line-ending conversion code.  Seems like I made a bad fix.
> Please add an empty line before this line. Also might be better to say something like:
>   Fix a regression bug in line-ending conversion code.

Fixed.

> WebCore/platform/BlobItem.cpp:158
>  +                      ++needFix;
> It seems to better to define needFix as a boolean.

Done.
 
> WebCore/platform/BlobItem.cpp:185
>  +              } else if (ending != EndingCR) {
> What are you going to do with else part? Do we need to copy the '\r' character?

(In the previous code) yes, else case would be char=='\r' and ending==EndingCR.
Comment 5 Jian Li 2010-06-28 16:15:57 PDT
Comment on attachment 59943 [details]
Patch

WebCore/platform/BlobItem.cpp:128
 +  // Convert line-endings (CR and LF) into CRLF.
Probably better to say:
  // Normalize all line-endings to CRLF.

WebCore/platform/BlobItem.cpp:176
 +  static CString convertToCR(const CString& from)
I think we can combine convertToCR and convertToLF to one single function like the following:
  convertToCROrLF(const CString& from, bool toCR)
  {
      char fromLineEndingChar = toCR ? '\n' : '\r';
      char toLineEndingChar = toCR ? '\r' : '\n';
      ...
      while (...) {
          ...
          } else if (c == fromLineEndingChar) {
              // Turn CF/LF into LF/CR.
  }
Comment 6 Kinuko Yasuda 2010-06-28 18:18:08 PDT
Created attachment 59968 [details]
Patch
Comment 7 Kinuko Yasuda 2010-06-28 18:35:14 PDT
Created attachment 59971 [details]
Patch
Comment 8 Kinuko Yasuda 2010-06-28 18:37:11 PDT
(In reply to comment #5)
> (From update of attachment 59943 [details])
> WebCore/platform/BlobItem.cpp:128
>  +  // Convert line-endings (CR and LF) into CRLF.
> Probably better to say:
>   // Normalize all line-endings to CRLF.

Done.

> WebCore/platform/BlobItem.cpp:176
>  +  static CString convertToCR(const CString& from)
> I think we can combine convertToCR and convertToLF to one single function like the following:
>   convertToCROrLF(const CString& from, bool toCR)
>   {
>       char fromLineEndingChar = toCR ? '\n' : '\r';
>       char toLineEndingChar = toCR ? '\r' : '\n';
>       ...
>       while (...) {
>           ...
>           } else if (c == fromLineEndingChar) {
>               // Turn CF/LF into LF/CR.
>   }

Done.
Comment 9 Jian Li 2010-06-29 10:43:49 PDT
Comment on attachment 59971 [details]
Patch

Looks good except the following minor issue. Please address it before you land the patch.

WebCore/platform/BlobItem.cpp:242
 +      ASSERT_NOT_REACHED();
This is not needed.
Comment 10 Kinuko Yasuda 2010-06-29 18:16:39 PDT
Committed r62155: <http://trac.webkit.org/changeset/62155>