[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