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
Kinuko Yasuda
2010-06-21 11:57:53 PDT
Created attachment 59273 [details]
Patch
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.
Created attachment 61139 [details]
Patch
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 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.
Committed r63343: <http://trac.webkit.org/changeset/63343> |