Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1376673003:
Collect idls and organize interface node (Closed)
DescriptionCollects IDL file paths under the directory and extracts file paths to text file.
Then extracts each dictionary of interface node's information.
Patch Set 1 : Merge 'Implements' #Patch Set 2 : #
Total comments: 39
Patch Set 3 : Change code architecture #
Total comments: 8
Patch Set 4 : Remove get_interface() and modify filter_partial() and filter_non_partial() #
Total comments: 22
Patch Set 5 : Change inherit_node_to_dict() #Patch Set 6 : Change the variable names #
Messages
Total messages: 17 (6 generated)
natsukoa@google.com changed reviewers: + bashi@chromium.org, yukishiino@chromium.org
natsukoa@google.com changed reviewers: + haraken@chromium.org
upload as a test
Patchset #1 (id:1) has been deleted
I edited and uploaded collect_idls_into_json.py. Now, this script can Collect information of data in interface node Merge partial interface and interface Merge ‘Implements’ and interface Dump json Would you give me some comments for me to improve it? Thank you.
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:79: def get_attribute_node(interface_node): I'd recommend to name this function get_attribute_node**_list**. I believe it helps you avoid possible confusions. Ditto for all other cases. For example, get_extattribute_node. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:138: 'ExtAttributes': [extattr_node_to_dict(extattr) for extattr in get_extattribute_node(attribute_node) if extattr_node_to_dict(extattr)], Do we need |if extattr_node_to_dict(extattr)| ? Ditto for all other cases. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:210: def get_inherit_node(interface_node): By definition, there must be at most one parent interface. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:278: 'Consts': [const_node_to_dict(const) for const in get_const_node(interface_node)], Please choose the order carefully. Why are they sorted in the following order? 1. Attributes 2. Operations 3. ExtAttributes 4. Consts Ditto for all other sorted things.
Patchset #4 (id:80001) has been deleted
Patchset #3 (id:60001) has been deleted
I fixed some code and comments. Would you check the script again? Thank you. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:6: """Usage: collect_idls_into_json.py path_file.txt json_file.json On 2015/09/30 03:13:15, bashi1 wrote: > Please add a description (What this script does). Done. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:11: import json On 2015/09/30 03:13:15, bashi1 wrote: > nit: sort imports in alphabetical order. Done. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:16: _class_name = 'Interface' On 2015/09/30 03:13:15, bashi1 wrote: > nit: use upper case for consts. > > _class_name -> _CLASS_NAME Done. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:17: _partial = 'Partial' On 2015/09/30 03:13:16, bashi1 wrote: > _partial -> _PARTIAL Done. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:21: """Returns interface IDL node. On 2015/09/30 03:13:15, bashi1 wrote: > Is this function used? If not, remove it. I forgot to change the description, but I'm using this function in main(). https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:27: parser = BlinkIDLParser(debug=False) On 2015/09/30 03:13:16, bashi1 wrote: > nit: you can omit debug=False Really? I couldn't remove it because debug information can't dump to json. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:34: def get_interface_node(definition): On 2015/09/30 03:13:15, bashi1 wrote: > Please add a comment to describe what this function does. Done. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:41: return definition On 2015/09/30 03:13:16, bashi1 wrote: > ditto. Done. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:138: 'ExtAttributes': [extattr_node_to_dict(extattr) for extattr in get_extattribute_node(attribute_node) if extattr_node_to_dict(extattr)], On 2015/09/30 04:24:22, Yuki wrote: > Do we need |if extattr_node_to_dict(extattr)| ? > > Ditto for all other cases. I checked the difference between with |if extattr_node_to_dict(extattr)| or not and there is no difference. So I just removed it. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:278: 'Consts': [const_node_to_dict(const) for const in get_const_node(interface_node)], On 2015/09/30 04:24:21, Yuki wrote: > Please choose the order carefully. Why are they sorted in the following order? > 1. Attributes > 2. Operations > 3. ExtAttributes > 4. Consts > > Ditto for all other sorted things. I changed the order that 1. Constants 2. Attributes 3. Operations 4. ExtAttributes https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:284: def merge_dict(interface_dict, partial_dict): On 2015/09/30 03:13:16, bashi1 wrote: > merge_dict -> merge_partial_dicts ? > interface_dict -> interfaces_dict ? > partial_dict -> partials_dict ? Done. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:292: for key in partial_dict.keys(): On 2015/09/30 03:13:16, bashi1 wrote: > key -> interface_name ? Done. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... 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 On 2015/09/30 03:13:16, bashi1 wrote: > (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. I didn't define any specific Consts, but I could merge the dict in this way. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... 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)] On 2015/09/30 03:13:16, bashi1 wrote: > 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. I misunderstood 'predicate'. I fixed it.
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:79: def get_attribute_node(interface_node): On 2015/09/30 04:24:21, Yuki wrote: > I'd recommend to name this function get_attribute_node**_list**. > I believe it helps you avoid possible confusions. > > Ditto for all other cases. For example, get_extattribute_node. Could you address this comment, too? https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:210: def get_inherit_node(interface_node): On 2015/09/30 04:24:21, Yuki wrote: > By definition, there must be at most one parent interface. Could you address this comment, too? https://codereview.chromium.org/1376673003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1376673003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:18: _CLASS_NAME = 'Interface' _CLASS_NAME => _INTERFACE_CLASS_NAME or something like that. In the context of IDLNode, "Attribute", "Type", etc. are class name, too. https://codereview.chromium.org/1376673003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:38: def get_interface_node(definition): get_interface_node => **is_**interface_node https://codereview.chromium.org/1376673003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:99: def get_const_node(interface_node): get_const_node**_list**. Ditto for all other cases. https://codereview.chromium.org/1376673003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:358: file_to_list = utilities.read_file_to_list(path_file) file_to_list => file_list or path_list
https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:27: parser = BlinkIDLParser(debug=False) On 2015/10/05 01:47:10, natsukoa wrote: > On 2015/09/30 03:13:16, bashi1 wrote: > > nit: you can omit debug=False > > Really? I couldn't remove it because debug information can't dump to json. The default value of |debug| is False, which means that yo don't need to set the argument explicitly :) https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
Patchset #4 (id:120001) has been deleted
I removed get_interface_node() and modify filter_partial() and filter_non_partial(). Would you check for me please? Thank you. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:27: parser = BlinkIDLParser(debug=False) On 2015/10/05 07:23:05, bashi1 wrote: > On 2015/10/05 01:47:10, natsukoa wrote: > > On 2015/09/30 03:13:16, bashi1 wrote: > > > nit: you can omit debug=False > > > > Really? I couldn't remove it because debug information can't dump to json. > > The default value of |debug| is False, which means that yo don't need to set the > argument explicitly :) > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... oh... I got it. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:79: def get_attribute_node(interface_node): On 2015/10/05 04:33:43, Yuki wrote: > On 2015/09/30 04:24:21, Yuki wrote: > > I'd recommend to name this function get_attribute_node**_list**. > > I believe it helps you avoid possible confusions. > > > > Ditto for all other cases. For example, get_extattribute_node. > > Could you address this comment, too? Done. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:210: def get_inherit_node(interface_node): On 2015/10/05 04:33:43, Yuki wrote: > On 2015/09/30 04:24:21, Yuki wrote: > > By definition, there must be at most one parent interface. > > Could you address this comment, too? I know there is no interface that have number of inherit node, but it's possible interface node have some inheritances. So I use 'GetListOf' instead of 'GetOneOf' https://codereview.chromium.org/1376673003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1376673003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:18: _CLASS_NAME = 'Interface' On 2015/10/05 04:33:43, Yuki wrote: > _CLASS_NAME => _INTERFACE_CLASS_NAME or something like that. > > In the context of IDLNode, "Attribute", "Type", etc. are class name, too. Done. https://codereview.chromium.org/1376673003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:38: def get_interface_node(definition): On 2015/10/05 04:33:44, Yuki wrote: > get_interface_node => **is_**interface_node I remove get_interface_node(). filter_partial and filter_non_partial have the function that identify interface or not. https://codereview.chromium.org/1376673003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:99: def get_const_node(interface_node): On 2015/10/05 04:33:43, Yuki wrote: > get_const_node**_list**. > > Ditto for all other cases. Done. https://codereview.chromium.org/1376673003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:358: file_to_list = utilities.read_file_to_list(path_file) On 2015/10/05 04:33:43, Yuki wrote: > file_to_list => file_list or path_list Done.
Your code is becoming much better than before. So I'm starting to send minor comments. https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:210: def get_inherit_node(interface_node): On 2015/10/05 08:59:04, natsukoa wrote: > On 2015/10/05 04:33:43, Yuki wrote: > > On 2015/09/30 04:24:21, Yuki wrote: > > > By definition, there must be at most one parent interface. > > > > Could you address this comment, too? > > I know there is no interface that have number of inherit node, but it's possible > interface node have some inheritances. > So I use 'GetListOf' instead of 'GetOneOf' If you think so, then you should reject such wrong data rather than accepting the wrong data. Raising an exception is a good option. If you really insist to support the wrong data, then you should change the function name. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:39: """Returns implement node. Let's update the comments accordingly. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:45: if definition.GetClass() == 'Implements': The code is equivalent to x = definition.GetClass() == 'Implements' if x == True: return True else: # x == False return False Thus, x = definition.GetClass() == 'Implements' # True or False return x And then, return definition.GetClass() == 'Implements' https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:51: def filter_non_partial(definition): filter_non_partial => is_non_partial https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:52: """Returns boolean. "Returns boolean" doesn't make much sense. One of common descriptions of predicates is: "Returns True if ..., otherwise False." https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:56: True: its 'Interface' class node and not 'partial' node For example, Returns: True if |definition| is an interface node and not partial interface. Probably, "its" is a typo of "it's"? If so, it's unclear what "it" means. Better to explicitly say |definition|. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:80: """Returns relative path under the WebKit directory which |interface_node| is defined. For example, Returns the relative path to the IDL file in which |interface_node| is defined. The IDL files are supposed to be placed in the WebKit directory. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:84: str which is |interface_node| file path |interface_node|'s file path https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:91: """Returns Constant object. object? Better to use the same terminology. Your comments are mixture of - Constant vs constant vs Const vs const - node vs object vs node object It would be confusing for readers. Please choose one style and always use it. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:105: node.GetChildren()[0].GetName(): str, constant object's name The comment seems strange. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:123: const_nodes: list of interface node object which has constant nodes => node https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:123: const_nodes: list of interface node object which has constant nodes => node Update the description, too. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:162: attribute_nodes: list of attribute node object attribute_nodes = > attribute_node
I am losing the way how to define valuable's name. Would you give me some advice for me please? Thank you. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:45: if definition.GetClass() == 'Implements': On 2015/10/05 10:03:21, Yuki wrote: > The code is equivalent to > > x = definition.GetClass() == 'Implements' > if x == True: > return True > else: # x == False > return False > > Thus, > > x = definition.GetClass() == 'Implements' # True or False > return x > > And then, > > return definition.GetClass() == 'Implements' Done. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:51: def filter_non_partial(definition): On 2015/10/05 10:03:21, Yuki wrote: > filter_non_partial => is_non_partial Done. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:52: """Returns boolean. On 2015/10/05 10:03:21, Yuki wrote: > "Returns boolean" doesn't make much sense. > > One of common descriptions of predicates is: > "Returns True if ..., otherwise False." Done. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:56: True: its 'Interface' class node and not 'partial' node On 2015/10/05 10:03:21, Yuki wrote: > For example, > Returns: > True if |definition| is an interface node and not partial interface. > > Probably, "its" is a typo of "it's"? > If so, it's unclear what "it" means. Better to explicitly say |definition|. Done. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:80: """Returns relative path under the WebKit directory which |interface_node| is defined. On 2015/10/05 10:03:21, Yuki wrote: > For example, > Returns the relative path to the IDL file in which |interface_node| is > defined. > The IDL files are supposed to be placed in the WebKit directory. Done. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:84: str which is |interface_node| file path On 2015/10/05 10:03:21, Yuki wrote: > |interface_node|'s file path Done. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:91: """Returns Constant object. On 2015/10/05 10:03:21, Yuki wrote: > object? > > Better to use the same terminology. > > Your comments are mixture of > - Constant vs constant vs Const vs const > - node vs object vs node object > > It would be confusing for readers. Please choose one style and always use it. Done. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:105: node.GetChildren()[0].GetName(): str, constant object's name On 2015/10/05 10:03:21, Yuki wrote: > The comment seems strange. Done. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:123: const_nodes: list of interface node object which has constant On 2015/10/05 10:03:21, Yuki wrote: > nodes => node > > Update the description, too. Done. https://codereview.chromium.org/1376673003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:162: attribute_nodes: list of attribute node object On 2015/10/05 10:03:21, Yuki wrote: > attribute_nodes = > attribute_node Done.
On 2015/10/06 07:10:22, natsukoa wrote: > I am losing the way how to define valuable's name. > Would you give me some advice for me please? Could you talk to me or bashi san offline? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
