[webkit-reviews] review denied: [Bug 55353] wrap json in a function call to afford cross-domain loading : [Attachment 84022] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 55353: wrap json in a function call to afford cross-domain loading
https://bugs.webkit.org/show_bug.cgi?id=55353

Attachment 84022: Patch
https://bugs.webkit.org/attachment.cgi?id=84022&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84022&action=review

r- for code duplication

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:56
> +from json_layout_results_generator import JSONLayoutResultsGenerator
>  from result_summary import ResultSummary
>  from test_input import TestInput
>  
>  import dump_render_tree_thread
> -import json_layout_results_generator
>  import message_broker
>  import printing
>  import test_expectations

The imports in this file are weird.  While you're here, can you convert all
these to absolute imports (as recommended by PEP8) and sort them?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:847
> -	       simplejson.dump(new_results, file, sort_keys=True,
separators=(',', ':'))
> +	       json_data = simplejson.dumps(new_results, sort_keys=True,
separators=(',', ':'))
> +	       json_string = JSONLayoutResultsGenerator.JSON_PREFIX + json_data
+ JSONLayoutResultsGenerator.JSON_SUFFIX
> +	       file.write(json_string)

Can you just call self._fs.write_text_file?  Should we try to factor this code
and _generate_json_file into a helper function (or just call
_generate_json_file directly)?

> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:423
> +	       content = results_json_file.read()
> +	       content =
content[len(JSONLayoutResultsGenerator.JSON_PREFIX):len(content) -
len(JSONLayoutResultsGenerator.JSON_SUFFIX)]
> +	       results_json = simplejson.loads(content)

Can we make a helper method for this (load a json file while skipping over the
json prefix/suffix)?