Bug 40932

Summary: Separate line-ending conversion code from BlobItem for code cleanup
Product: WebKit Reporter: Kinuko Yasuda <kinuko>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dimich, fishd, jianli
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch jianli: review+

Description Kinuko Yasuda 2010-06-21 11:57:53 PDT
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.)
Comment 1 Kinuko Yasuda 2010-06-21 12:21:02 PDT
Created attachment 59273 [details]
Patch
Comment 2 Jian Li 2010-06-28 10:17:16 PDT
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.
Comment 3 Kinuko Yasuda 2010-07-09 18:49:16 PDT
Created attachment 61139 [details]
Patch
Comment 4 Kinuko Yasuda 2010-07-09 18:52:45 PDT
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).
Comment 5 Jian Li 2010-07-13 14:59:15 PDT
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.
Comment 6 Kinuko Yasuda 2010-07-14 12:26:43 PDT
Committed r63343: <http://trac.webkit.org/changeset/63343>