[webkit-reviews] review denied: [Bug 23296] add Android platform-specific files to WebCore/platform : [Attachment 31059] new patch part 3 with ChangeLog
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 8 14:33:34 PDT 2009
Eric Seidel <eric at webkit.org> has denied Feng Qian <feng at chromium.org>'s
request for review:
Bug 23296: add Android platform-specific files to WebCore/platform
https://bugs.webkit.org/show_bug.cgi?id=23296
Attachment 31059: new patch part 3 with ChangeLog
https://bugs.webkit.org/attachment.cgi?id=31059&action=review
------- Additional Comments from Eric Seidel <eric at webkit.org>
Various style violations:
35 if (m_filenames.size() == 0) {
36 return String();
37 }
isEmpty() no? Also no extra {}
Don't we have methods to do ellipses already?
40 String output = m_filenames[0].copy();
41 while (font.width(TextRun(output.impl())) > width && output.length() >
4) {
42 output = output.replace(output.length() - 4, 4, String("..."));
43 }
String has a printf method if you like:
54 String path = sPluginPath;
55 path.append("/");
56 path.append(prefix);
57 path.append(String::number(number));
Style:
CString fileSystemRepresentation(const String& path) {
46 return path.utf8();
47 }
Style:
59 const char *fstr = filename.data();
Style:
if (handle != -1) {
64 return filename;
65 }
Why the date comment here?
98 // new as of SVN change 36269, Sept 8, 2008
99 String homeDirectoryPath()
Or here?
// new as of webkit4, Feb 28, 2009
105 Vector<String> listDirectory(const String& path, const String& filter)
106 {
Why the extra space?
112 struct dirent * dp;
Single line if style:
115 if (strcmp(name, ".") == 0 || strcmp(name, "..") == 0) {
116 continue;
117 }
118 if (fnmatch(cfilter.data(), name, 0) != 0) {
119 continue;
120 }
WebKit uses ! instead of == NULL:
13 while ((dp = readdir(dir)) != NULL) {
Otherwise this seems sane enough. Doing so many patches on one bug makes it
really hard to read the comments after a while.
More information about the webkit-reviews
mailing list