[Webkit-unassigned] [Bug 37765] REGRESSION(57531): the commit-queue still hates Tor Arne Vestbø

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37765





--- Comment #21 from Tony Chang (Google) <tony at chromium.org>  2010-04-18 17:48:25 PST ---
(From update of attachment 53626)
In response to comment #2, I think it would work if you use decode instead of
encode (the string you passed in is already utf8 encoded, you want to convert
it to a unicode object, right?).

FWIW, I would probably have tried to keep everything as a utf8 encoded str, but
using unicode everywhere seems fine too (you get some better type checking this
way).  In either case, thanks for taking this on.

Some small style nits below.

> diff --git a/WebKitTools/Scripts/webkitpy/common/checkout/changelog.py b/WebKitTools/Scripts/webkitpy/common/checkout/changelog.py
> index e93896f..862dc48 100644
> --- a/WebKitTools/Scripts/webkitpy/common/checkout/changelog.py
> +++ b/WebKitTools/Scripts/webkitpy/common/checkout/changelog.py
> @@ -102,12 +102,13 @@ class ChangeLog(object):
>          date_line_regexp = re.compile(ChangeLogEntry.date_line_regexp)
>          entry_lines = []
>          # The first line should be a date line.
> -        first_line = changelog_file.readline()
> +        first_line = unicode(changelog_file.readline(), "utf-8")

Nit: I find using .decode('utf-8') easier to read than calling unicode(), but
either is fine with me.  There's another instance of this in the same file.

> diff --git a/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py b/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py
> index bb22d14..2dd4d20 100644
> --- a/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py
> +++ b/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py
>      def _parse_attachment_element(self, element, bug_id):
> +

Nit: Did you mean to add this blank line?

> diff --git a/WebKitTools/Scripts/webkitpy/common/system/executive.py b/WebKitTools/Scripts/webkitpy/common/system/executive.py
> index b6126e4..cbe0e1f 100644
> --- a/WebKitTools/Scripts/webkitpy/common/system/executive.py
> +++ b/WebKitTools/Scripts/webkitpy/common/system/executive.py
> @@ -145,7 +157,7 @@ class Executive(object):
>              # os.kill isn't available on Windows.  However, when I tried it
>              # using Cygwin, it worked fine.  We should investigate whether
>              # we need this platform specific code here.
> -            subprocess.call(('taskkill.exe', '/f', '/pid', str(pid)),
> +            subprocess.call(('taskkill.exe', '/f', '/pid', unicode(pid)),

Nit: This can probably stay as str().  It's a bit weird to have the other
strings be str and the pid be unicode.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.