https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py
File
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py
(right):
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode6
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:6:
"""Usage: collect_idls_into_json.py path_file.txt json_file.json
Please add a description (What this script does).
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode11
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:11:
import json
nit: sort imports in alphabetical order.
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode16
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:16:
_class_name = 'Interface'
nit: use upper case for consts.
_class_name -> _CLASS_NAME
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode17
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:17:
_partial = 'Partial'
_partial -> _PARTIAL
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode21
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:21:
"""Returns interface IDL node.
Is this function used? If not, remove it.
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode27
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:27:
parser = BlinkIDLParser(debug=False)
nit: you can omit debug=False
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode34
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:34:
def get_interface_node(definition):
Please add a comment to describe what this function does.
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode36
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:36:
return definition
What should be a return value when definition.GetClass() != _class_name
?
If None, lets return None explicitly.
BTW, I think this should be a predicate. See the comment on main().
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode41
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:41:
return definition
ditto.
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode284
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:284:
def merge_dict(interface_dict, partial_dict):
merge_dict -> merge_partial_dicts ?
interface_dict -> interfaces_dict ?
partial_dict -> partials_dict ?
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode292
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:292:
for key in partial_dict.keys():
key -> interface_name ?
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode293
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:293:
interface_dict[key]['Attributes'].extend(partial_dict[key]['Attributes'])
if partial_dict[key]['Attributes'] != [] else None
(1) What if |interface_dict| doesn't have |key|? Doesn't it never
happen?
(2) It would be better to use a temporary variable for the target
interface object.
(3) You are doing the same things for 'Attributes', 'Operations' and
'Consts'. You can share the code.
So, I would re-write this function like:
for interface_name, partial in partial_dict.iteritems():
interface = interface_dict.get(interface_name
if not interface:
continue
for member in ['Attributes', 'Operations', 'Consts']:
interface[member].extend(partial.get(member))
You might want to define some consts for the member list.
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode301
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:301:
for key in implement_nodes:
See comment on merge_dict(). Almost the same can be said here.
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode325
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:325:
implement_nodes = [get_implements_node(definition) for definition in
get_definitions(file_to_list) if get_implements_node(definition)]
Could you format this (and below dicts) so that we can easily read what
are you doing?
for example,
implement_nodes = [get_implements_node(definition)
for definition in get_definitions(file_to_list)
if get_implements_node(definition)]
BTW, I think get_implements_node() should be a predicate which return
True when definition's class is 'Implements'.
implement_nodes = [definition
for definition in get_definitions(file_to_list)
if is_implements_definition(definition)]
Same for below dicts.
https://codereview.chromium.org/1376673003/