http/tests/local/blob/send-data-blob.html test is failing on Windows.
Created attachment 59801 [details] Patch
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?
Created attachment 59943 [details] Patch
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 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. }
Created attachment 59968 [details] Patch
Created attachment 59971 [details] Patch
(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 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.
Committed r62155: <http://trac.webkit.org/changeset/62155>