Bug 40932 – Separate line-ending conversion code from BlobItem for code cleanup

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

[Kinuko Yasuda]

StrongBlobItem has code for fixing line-endings (e.g. CRLF to native endings etc) but it'd be better to have them separated from BlobItem.  (It'd be also good if we're going to obsolete BlobItem later.)

[Kinuko Yasuda]

Created attachment 59273 [details]
Patch

[Jian Li]

Comment on attachment 59273 [details]
Patch

WebCore/platform/text/LineEnding.cpp:2
 +   * Copyright (C) 2010 Google Inc. All rights reserved.
Please check the author of the original function and give credit here too.

WebCore/platform/text/LineEnding.h:53
 +  // Convert WebCore::String to CString with given line-ending and encoding.
This comment is a little bit vague. It does not mention what is fixed regarding the line ending. Could you please add more detailed comment here?
Also mentioning "Convert WebCore::String to CString" seems to be unnecessary since the argument type and return type indicate that clearly.

[Kinuko Yasuda]

Created attachment 61139 [details]
Patch

[Kinuko Yasuda]

Thanks for reviewing, updated the code with recent fixes.

(In reply to comment #2)
> (From update of attachment 59273 [details])
> WebCore/platform/text/LineEnding.cpp:2
>  +   * Copyright (C) 2010 Google Inc. All rights reserved.
> Please check the author of the original function and give credit here too.

Fixed.

> WebCore/platform/text/LineEnding.h:53
>  +  // Convert WebCore::String to CString with given line-ending and encoding.
> This comment is a little bit vague. It does not mention what is fixed regarding the line ending. Could you please add more detailed comment here?
> Also mentioning "Convert WebCore::String to CString" seems to be unnecessary since the argument type and return type indicate that clearly.

Fixed - also splitted the function into simple ones (one for converting CRLF, one for CR, one for LF and etc).

[Jian Li]

Comment on attachment 61139 [details]
Patch

r=me

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

WebCore/platform/text/LineEnding.h:38
 +  using WTF::CString;
Please try to avoid using "using" directive in the header file if possible.

WebCore/platform/text/LineEnding.h:51
 +  // Normalize all line-endings in the given string to the native line-endings
Please end the comment with period.

WebCore/platform/text/LineEnding.h:52
 +  // (e.g. normalize to CRLF on Windows etc).
Better to also mention that we normalize to LF for all other platforms, including Linux and MacOSX.

[Kinuko Yasuda]

Committed r63343: <http://trac.webkit.org/changeset/63343>