[webkit-reviews] review denied: [Bug 32075] Add workaround for json parsed as unicode : [Attachment 44162] master.cfg patch

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has denied Marc-Antoine Ruel
<maruel at chromium.org>'s request for review:
Bug 32075: Add workaround for json parsed as unicode
https://bugs.webkit.org/show_bug.cgi?id=32075

Attachment 44162: master.cfg patch
https://bugs.webkit.org/attachment.cgi?id=44162&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
> Index: WebKitTools/BuildSlaveSupport/build.webkit.org-config/master.cfg
> ===================================================================
> --- WebKitTools/BuildSlaveSupport/build.webkit.org-config/master.cfg 
(revision 51602)
> +++ WebKitTools/BuildSlaveSupport/build.webkit.org-config/master.cfg 
(working copy)
> @@ -317,10 +317,32 @@ class BuildAndTestLeaksFactory(BuildAndT
>      TestClass = RunWebKitLeakTests
>  
>  
> -def loadBuilderConfig(c):
> -    passwords = simplejson.load(open('passwords.json'))
> +def convertToString(json_data):

This method name is way too general and doesn’t provide enough information
about what it actually does.

> +    if isinstance(json_data, dict):
> +	   retval = {}
> +	   for (k,v) in json_data.iteritems():
> +	       retval[str(k)] = convertToString(v)
> +	   return retval
> +    elif isinstance(json_data, list):
> +	   retval = []
> +	   for i in json_data:
> +	       retval.append(convertToString(i))
> +	   return retval
> +    elif isinstance(json_data, unicode):
> +	   return str(json_data)

This isn’t safe as it will encode the unicode string using the system encoding.
 On many machines this is ASCII which means it will throw if any non-ASCII data
is present.

> +    else:
> +	   return json_data

The variable naming convention within this code is internally inconsistent. 
It’s probably best to match the existing convention used within this file.

> +def loadBuilderConfig(c, config_file, passwords_file):
> +    passwords = convertToString(simplejson.load(open(passwords_file)))
> +
> +    config = convertToString(simplejson.load(open(config_file)))

Ditto here.


Per your comment the only reason we’re doing this is so that the dictionary
keys are instances of str rather than unicode.	Given that it seems as though
it would be much cleaner to do something like:

def hook(newObject):
    for key in newObject:
	value = newObject.pop(key)
	newObject[key.encode('utf-8')] = value
    return newObject

simplejson.load(open('config.json'), object_hook=hook)

Is there some reason this wouldn’t be suitable?