[webkit-reviews] review denied: [Bug 27371] [WINCE] Add WinCE specific platform/text files to WebCore : [Attachment 34875] modified

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Aug 24 17:26:20 PDT 2009


Eric Seidel <eric at webkit.org> has denied Yong Li <yong.li at torchmobile.com>'s
request for review:
Bug 27371: [WINCE] Add WinCE specific platform/text files to WebCore
https://bugs.webkit.org/show_bug.cgi?id=27371

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
This line is in the middle of nowhere:
 5  * All rights reserved.

Not true:
 2  * This file is part of the DOM implementation for KDE.

Why?
 246 #undef isSentenceStop

I'm not sure what this syntax does:
 59	friend LanguageManager& languageManager();

Why std::map instead of HashMap?
static std::map<UINT, std::string>& codePageCharsets()
 65 {
 66	static std::map<UINT, std::string> cc;
 67	return cc;
 68 }

Indent:
214 TextCodecWince::TextCodecWince(const TextEncoding& encoding)
 215 : m_encoding(encoding)

Comments are likely needed here:
 231	 if (codePage > 50000) {
 232	     if ((codePage >= 50220 && codePage <= 50222)
 233		 || codePage == 50225
 234		 || codePage == 50227
 235		 || codePage == 50229
 236		 || codePage == 52936
 237		 || codePage == 54936
 238		 || (codePage >= 57002 && codePage <= 57001)
 239		 || codePage == 65000 // UTF-7

We generally use full english words for variable names:
 293			     int rtn = encMbToWc(dst, (const unsigned
char*)bytes, srcEnd - bytes);
 294			     if (rtn >= 0)
(obviously we may have to adjust to avoid keywords here)

I dont' think explicit construction is needed here:
66     if (resultLength <= 0)
 467	     return CString("?");

r-, mostly needing explanation as to why the std:: types, generally we only use
std:: algorithms, not types in WebCore.  IIRC that's due to compiling without
exceptions.


More information about the webkit-reviews mailing list