[webkit-reviews] review denied: [Bug 41228] Fix http/tests/local/blob/send-data-blob.html on Windows : [Attachment 59801] Patch

[bugzilla-daemon at webkit.org]

Jian Li <jianli at chromium.org> has denied Kinuko Yasuda <kinuko at chromium.org>'s
request for review:
Bug 41228: Fix http/tests/local/blob/send-data-blob.html on Windows
https://bugs.webkit.org/show_bug.cgi?id=41228

Attachment 59801: Patch
https://bugs.webkit.org/attachment.cgi?id=59801&action=review

------- Additional Comments from Jian Li <jianli at chromium.org>
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?