Bug 41228 – Fix http/tests/local/blob/send-data-blob.html on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kinuko Yasuda]

http/tests/local/blob/send-data-blob.html test is failing on Windows.

[Kinuko Yasuda]

Created attachment 59801 [details]
Patch

[Jian Li]

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?

[Kinuko Yasuda]

Created attachment 59943 [details]
Patch

[Kinuko Yasuda]

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.

[Jian Li]

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.
  }

[Kinuko Yasuda]

Created attachment 59968 [details]
Patch

[Kinuko Yasuda]

Created attachment 59971 [details]
Patch

[Kinuko Yasuda]

(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.

[Jian Li]

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.

[Kinuko Yasuda]

Committed r62155: <http://trac.webkit.org/changeset/62155>