[webkit-reviews] review denied: [Bug 36063] Upload test result json files to app engine server : [Attachment 54756] update proposed patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue May 4 13:40:24 PDT 2010


Eric Seidel <eric at webkit.org> has denied Victor Wang <victorw at chromium.org>'s
request for review:
Bug 36063: Upload test result json files to app engine server
https://bugs.webkit.org/show_bug.cgi?id=36063

Attachment 54756: update proposed patch
https://bugs.webkit.org/attachment.cgi?id=54756&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1044
 +	    for file in json_files:
This is simpler as:
files = [(file, os.path.join(self._options.results_directory, file)) for file
in json_files]

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1054
 +		uploader.upload(attrs, files, 120)
The files array being an array of name/path pairs seems slightly strange.  Why
not just paths?

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.
py:38
 +	    self.browser = Browser()
Shouldn't this be self._browser?

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.
py:37
 +	    self.url = "http://%s" % host
self._url?

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.
py:46
 +	    for (filename, path) in files:
You can always get the name from os.path.basename(path) instead of having to
pass it separately here.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.
py:47
 +		self.browser.add_file(open(path, "rb"), "text/plain", filename,

To address Adam's question, yes this is unicode-safe.  ClientForm (used by
Mechanize), excepts a file-like object returning str (byte array) objects.  So
this is fine.  Are we sure they're text/plane though?  Do we need to give a
more sophisticated mime type including the encoding?  JSON is always utf-8
iirc.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.
py:41
 +	    self.browser.open(self.url + "/testfile/uploadform")
%s is always better than +

r- for nits.


More information about the webkit-reviews mailing list