[webkit-reviews] review denied: [Bug 75825] webkit-patch land should automatically add svn:mime-type for .png files : [Attachment 138335] proposed fix
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed May 2 14:23:44 PDT 2012
Dirk Pranke <dpranke at chromium.org> has denied Balazs Ankes
<bank at inf.u-szeged.hu>'s request for review:
Bug 75825: webkit-patch land should automatically add svn:mime-type for .png
files
https://bugs.webkit.org/show_bug.cgi?id=75825
Attachment 138335: proposed fix
https://bugs.webkit.org/attachment.cgi?id=138335&action=review
------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=138335&action=review
> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:30
> +import sys
It looks like sys is unused in this file ...
> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:33
> +def check(self, platform, fs):
Is 'platform' a PlatformInfo object? It looks like it is ... maybe this should
take a host or a systemhost instead?
Also, some form of docstring would be good for this function. What is this
function supposed to be checking for, and how should the return value be used?
> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:72
> + if platform.startswith('linux') or platform.startswith('darwin'):
This should probably be
if platform.is_win():
config_file_path = fs.join(os.environ ...
else:
config_file_path = fs.join(fs.expand ...
> Tools/Scripts/webkitpy/style/checkers/png.py:45
> self._platform = platform or sys.platform
Does the style checker have access to a host or a tool object? It should get
the platform info from that ...
> Tools/Scripts/webkitpy/style/checkers/png.py:52
> + errorcode = checksvnconfigfile.check(self, self._platform,
self._fs)
Instead of returning an errorcode that is a bit mask, you should consider
either returning two values in a tuple, or splitting this into two functions.
> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:36
> + self._fs = FileSystem()
You can also get the filesystem from the tool.
More information about the webkit-reviews
mailing list