[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