| Summary: | wrap json in a function call to afford cross-domain loading | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ojan Vafai <ojan> | ||||||
| Component: | New Bugs | Assignee: | Ojan Vafai <ojan> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | mihaip, tony | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Other | ||||||||
| OS: | OS X 10.5 | ||||||||
| Attachments: |
|
||||||||
|
Description
Ojan Vafai
2011-02-27 23:15:41 PST
Created attachment 84022 [details]
Patch
Comment on attachment 84022 [details] Patch 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)? Created attachment 84185 [details]
Patch
Comment on attachment 84185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84185&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:46 > +JSON_PREFIX = "ADD_RESULTS(" > +JSON_SUFFIX = ");" Nit: Maybe prefix these with _ to mark them module private? Comment on attachment 84185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84185&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:45 > +JSON_PREFIX = "ADD_RESULTS(" Is there any way to have the test result server append these instead when it serves files? Otherwise they're not really valid JSON, and you can't parse them with simplejson without doing string munging. > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:60 > + # Specify separators in order to get compact encoding. I thought indent=None was the way to get compact output out of simplejson. Comment on attachment 84185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84185&action=review >> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:45 >> +JSON_PREFIX = "ADD_RESULTS(" > > Is there any way to have the test result server append these instead when it serves files? Otherwise they're not really valid JSON, and you can't parse them with simplejson without doing string munging. I considered this, and I'm still open to it, but it won't work for the case of us wanting to have results.html pull full_results.json in. The only way we could make that work locally in Chrome, is to run a local server, which I don't think we want to require just for loading the results.html page. If we can workaround that issue, then this would definitely be better. Changing the test result server is simple. I'm gonna commit this for now to keep things progressing. Happy to change it if we come up with a better solution. >> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:46 >> +JSON_SUFFIX = ");" > > Nit: Maybe prefix these with _ to mark them module private? sg >> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:60 >> + # Specify separators in order to get compact encoding. > > I thought indent=None was the way to get compact output out of simplejson. Hm. I'm not sure. That obviously remove the indent, but does it remove spaces around colons and commas? I'll test it out. If it does, I'll switch over to that. Comment on attachment 84185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84185&action=review >>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:60 >>> + # Specify separators in order to get compact encoding. >> >> I thought indent=None was the way to get compact output out of simplejson. > > Hm. I'm not sure. That obviously remove the indent, but does it remove spaces around colons and commas? I'll test it out. If it does, I'll switch over to that. indent=None does not remove the spaces around , and :. Committed r80090: <http://trac.webkit.org/changeset/80090> |