Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1323493002:
Collect IDL files and exract interface node object's information (Closed)
DescriptionAdd interface_node_path.py
This script collects IDL file paths under the directory and extracts file paths to text file.
Add interface_to_json.py
this script loads the text file which is output of interface_node_path.py and extracts each dictionary of interface node's information.
Patch Set 1 #Patch Set 2 : test upload #
Total comments: 3
Patch Set 3 : comment #
Total comments: 5
Patch Set 4 : Add explanation of each function #
Total comments: 18
Patch Set 5 : fix #Patch Set 6 : Fix code which was pointed out at codereview #
Total comments: 50
Patch Set 7 : fix #Patch Set 8 : Fixed GetListOf('...')[0] -> GetOneOf('...') #
Total comments: 5
Patch Set 9 : #Patch Set 10 : Remove Interface['Name'] #Patch Set 11 : Add Static and Readonly #Patch Set 12 : Add Inherit #Patch Set 13 : Fix style #
Total comments: 10
Patch Set 14 : Fix list comprehention -> list() #Patch Set 15 : Edit comment args #Patch Set 16 : #
Messages
Total messages: 25 (9 generated)
natsukoa@google.com changed reviewers: + bashi@chromium.org, haraken@chromium.org, yukishiino@chromium.org
https://codereview.chromium.org/1323493002/diff/20001/Source/bindings/scripts... File Source/bindings/scripts/interface_to_json.py (right): https://codereview.chromium.org/1323493002/diff/20001/Source/bindings/scripts... Source/bindings/scripts/interface_to_json.py:13: def load_filepaths(path_file): Let's write function comments and make it clear what are expected as arguments and return value.
https://codereview.chromium.org/1323493002/diff/20001/Source/bindings/scripts... File Source/bindings/scripts/interface_to_json.py (right): https://codereview.chromium.org/1323493002/diff/20001/Source/bindings/scripts... Source/bindings/scripts/interface_to_json.py:13: def load_filepaths(path_file): On 2015/09/01 04:35:56, Yuki wrote: > Let's write function comments and make it clear what are expected as arguments > and return value. Comments are fine, but Blink doesn't have a culture to add verbose comments in the code base. If we plan to land this patch to trunk, maybe we want to be clear about what level of comments we want to add (before natsuko-san spends a lot of time on it).
haraken@chromium.org changed reviewers: + shimadaa@google.com
https://codereview.chromium.org/1323493002/diff/20001/Source/bindings/scripts... File Source/bindings/scripts/interface_to_json.py (right): https://codereview.chromium.org/1323493002/diff/20001/Source/bindings/scripts... Source/bindings/scripts/interface_to_json.py:13: def load_filepaths(path_file): On 2015/09/01 04:38:47, haraken wrote: > On 2015/09/01 04:35:56, Yuki wrote: > > Let's write function comments and make it clear what are expected as arguments > > and return value. > > Comments are fine, but Blink doesn't have a culture to add verbose comments in > the code base. If we plan to land this patch to trunk, maybe we want to be clear > about what level of comments we want to add (before natsuko-san spends a lot of > time on it). I talked with Haraken offline, and we think it's good to write comments like the following. """One line summary (optional). Args: param1: very short desc (optional) and type param2: ditto Returns: very short desc (optional) and type """ It often happens that good function name and parameter name describe what they are. So you may not need descriptions for such cases. But I'd recommend to write types for all the cases. Here is an example. https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/cppl...
Patchset #3 (id:40001) has been deleted
I added comments.
For example,
"""
Args:
path: directory path
Return:
str, absolute IDL file path
"""
Let's start with small one. https://codereview.chromium.org/1323493002/diff/60001/Source/bindings/scripts... File Source/bindings/scripts/interface_node_path.py (right): https://codereview.chromium.org/1323493002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/interface_node_path.py:4: The goal of this script is to integrate IDL file path under the directory to text file. Please describe a typical usage here. Developers typically want to know the usage without looking the whole code. Another option would be to use optparse.OptionParser/argparse.ArgumentParser. https://codereview.chromium.org/1323493002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/interface_node_path.py:11: """ Please add one line summary as shiino-san suggested. For example, """Returns a generator which yields absolute paths of idl files. ... """ https://codereview.chromium.org/1323493002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/interface_node_path.py:15: str, absolute IDL file path This function returns a generator, not a str. https://codereview.chromium.org/1323493002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/interface_node_path.py:20: ) Let move |file_type| and |non_idl_set| out from this function. We don't need to create them every time get_idl_files() is called. We usually use CAPITAL_LETTERS for constants, and if they are 'private' (they aren't intended to be used outside of this script), we add '_' prefix. So, how about: _IDL_SUFFIX = '.idl' _NON_IDL_FILES = frozenset([ 'InspectorInstrumentation.idl', ]) ? https://codereview.chromium.org/1323493002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/interface_node_path.py:30: f = open(filename, 'w') How about: with open(filename, 'w') as f: ... This way you don't need to close |f|.
Patchset #4 (id:80001) has been deleted
I edit comments in front of each function.
We had an offline meeting for code review. Here is just random memo without detailed explanation. https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... File Source/bindings/scripts/interface_to_json.py (right): https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:2: Need copyright notice. https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:3: """Usage: interface_to_json.py path_file.txt json_file.json filename: "collect_idls_into_json.py" https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:18: """Returns a generator which yields absolute path of IDL files written in text file. written in |path_file|. https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:21: Return: s/Return/Returns/ https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:29: def get_interfaces(path_file): s/path_file/idl_paths/ https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:38: for node_path in load_filepaths(path_file): s/load_file_paths(path_file)/idl_paths/ https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:95: return node.GetListOf('Type')[0].GetChildren()[0].GetName() def get_attribute_type(attribute_node): .... get_operation_type = get_attribute_type def get_op_or_attr_type(op_or_attr_node): if (type(op_or_attr_node) != Operation and type(op_or_attr_node) != Attribute): raise "..." assert(type(op_or_attr_node) == Operation or type(op_or_attr_node) == Attribute) def get_type(node): if is_constant(node): return get_constant_type(node) else: // Attribute or Operation return node.GetListOf(....).....GetName() https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:116: """ def get_extattrs(node): def get_extattr_nodes(node): // old get_extattributes(node) for extattr_node in get_extattr_nodes(node): yield {'Name': extattr_node.GetName()} https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:301: def export_jsonfile(dictionary, json_file): path = "foo/bar/baz.ext" filepath = "foo/bar/baz.ext" filename = path or "baz.ext" file = file object (or filename)
Patchset #6 (id:140001) has been deleted
I edited code which was pointed out at code review. https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... File Source/bindings/scripts/interface_to_json.py (right): https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:2: On 2015/09/04 10:07:57, Yuki wrote: > Need copyright notice. Done. https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:3: """Usage: interface_to_json.py path_file.txt json_file.json On 2015/09/04 10:07:57, Yuki wrote: > filename: "collect_idls_into_json.py" Done. https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:18: """Returns a generator which yields absolute path of IDL files written in text file. On 2015/09/04 10:07:57, Yuki wrote: > written in |path_file|. Done. https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:21: Return: On 2015/09/04 10:07:57, Yuki wrote: > s/Return/Returns/ Done. https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:29: def get_interfaces(path_file): On 2015/09/04 10:07:57, Yuki wrote: > s/path_file/idl_paths/ Done. https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:38: for node_path in load_filepaths(path_file): On 2015/09/04 10:07:57, Yuki wrote: > s/load_file_paths(path_file)/idl_paths/ Done. https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:95: return node.GetListOf('Type')[0].GetChildren()[0].GetName() On 2015/09/04 10:07:57, Yuki wrote: > def get_attribute_type(attribute_node): > .... > > get_operation_type = get_attribute_type > > > def get_op_or_attr_type(op_or_attr_node): > if (type(op_or_attr_node) != Operation and > type(op_or_attr_node) != Attribute): > raise "..." > assert(type(op_or_attr_node) == Operation or type(op_or_attr_node) == > Attribute) > > > def get_type(node): > if is_constant(node): > return get_constant_type(node) > else: // Attribute or Operation > return node.GetListOf(....).....GetName() Done. https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:116: """ On 2015/09/04 10:07:57, Yuki wrote: > def get_extattrs(node): > def get_extattr_nodes(node): > // old get_extattributes(node) > > for extattr_node in get_extattr_nodes(node): > yield {'Name': extattr_node.GetName()} Done. https://codereview.chromium.org/1323493002/diff/100001/Source/bindings/script... Source/bindings/scripts/interface_to_json.py:301: def export_jsonfile(dictionary, json_file): On 2015/09/04 10:07:57, Yuki wrote: > path = "foo/bar/baz.ext" > filepath = "foo/bar/baz.ext" > > filename = path or "baz.ext" > > file = file object (or filename) Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... File Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:6: """Usage: interface_to_json.py path_file.txt json_file.json s/interface_to_json.py/collect_idls_into_json.py/ https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:8: path_file.txt: output of interface_node_path.py #path We're going to use the file used in GYP. Let's update the comment. I think it's enough to describe the input format. Anyway, we don't care about where the file comes from. https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:22: """Return a generator which yields interface IDL nodes. s/Return/Returns/ https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:24: path_file: text file Update the comment. https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:34: for idl_path in load_filepaths(idl_paths): load_filepaths requries |path_file|, but you're passing |idl_paths|... https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:42: """Return relative path which is contained in interface_node. ... path to the IDL file in which the |interface_node| is defined. is more important than a fact that ... path is contained in |interface_node|. https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:44: interface_node: interface node class object "class object" sounds like |IDLNode|. https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:52: def get_partial(interface_node_list): I don't have a good idea for the name, but I think "filter_partials" is better than "get_partial". https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:55: interface_node_list: generator, interface node class object You don't return a generator object AND an interface node class object. https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:57: a generator which yields interface node class object This comment doesn't make sense. Missing the key point, I think. https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:77: """Return list of Attribute object. What's "Attribute object"? Do we have Attribute class? https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:81: list which is Attribute object list Returns list which is ... list?? Why not simply "Returns a ... list" or "Returns a list of ..."? https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:87: """Return type of attribute or operation's type. type of type? What about argument? https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:100: """Return a generator which yields Extattribute's object dictionary Do we have Extattribute class? https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:100: """Return a generator which yields Extattribute's object dictionary get_attributes() returns a list of IDLNode while get_extattributes() returns a list of dictionaries. It looks inconsistent. https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:102: node: interface, attribute or operation node object#node which has extattr object #node which has extattr object? https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:107: for extattributes in node.GetListOf('ExtAttributes'): extattributes? https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:109: yield extattribute_list This code looks wrong. https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:244: def format_interface_dict(interface_node): I'd say "format an interface node in/to/into/as??? a dictionary." So, I'd write "format_interface_to_dict". https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:251: interface_dict = {} Please be consistent in the same style. https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:274: interface['ExtAttributes'].append(partial['ExtAttributes']) For extended attributes, I doubt that we should add ones of partial interface to non-partial interface. Let's check with Bashi-san. https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:276: if interface['Constant']: Why should you have this if-clause? https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:281: def format_dictionary(dictionary_list): The name and description of this function seems weird... https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:290: dictionary.setdefault(interface_dict['Name'], interface_dict) This is abuse of setdefault. https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:294: # export_to_jsonfile(), + indent command line argument Do you need this line? If you want to write a to do, the format is: # TODO(natsukoa): Supports a command line flag to indent the JSON. https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:295: def export_to_jsonfile(dictionary, json_file): We argued the path vs file problem. https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:296: """Return jsonfile which is dumped each interface_node information dictionary to json. Does this function return something? Note that this function does not have any return statement. https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:303: filename = json_file ? https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:310: path_filename = args[0] path_filename??
I edited unconcise comments and variable names.
I also edit .GetOneOf('...') instead of .GetListOf('...')[0].
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
File Source/bindings/scripts/collect_idls_into_json.py (right):
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:6: """Usage:
interface_to_json.py path_file.txt json_file.json
On 2015/09/07 10:35:26, Yuki wrote:
> s/interface_to_json.py/collect_idls_into_json.py/
Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:8: path_file.txt: output of
interface_node_path.py #path
On 2015/09/07 10:35:25, Yuki wrote:
> We're going to use the file used in GYP.
> Let's update the comment. I think it's enough to describe the input format.
> Anyway, we don't care about where the file comes from.
Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:22: """Return a generator
which yields interface IDL nodes.
On 2015/09/07 10:35:26, Yuki wrote:
> s/Return/Returns/
Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:24: path_file: text file
On 2015/09/07 10:35:26, Yuki wrote:
> Update the comment.
Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:34: for idl_path in
load_filepaths(idl_paths):
On 2015/09/07 10:35:26, Yuki wrote:
> load_filepaths requries |path_file|, but you're passing |idl_paths|...
I changed this function.
I took load_filepathes function outside and import read_file_to_list from
utilities.py.
The function to read file can choose in main function.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:42: """Return relative path
which is contained in interface_node.
On 2015/09/07 10:35:25, Yuki wrote:
> ... path to the IDL file in which the |interface_node| is defined.
>
> is more important than a fact that
>
> ... path is contained in |interface_node|.
Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:52: def
get_partial(interface_node_list):
On 2015/09/07 10:35:25, Yuki wrote:
> I don't have a good idea for the name, but I think "filter_partials" is better
> than "get_partial".
Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:55: interface_node_list:
generator, interface node class object
On 2015/09/07 10:35:26, Yuki wrote:
> You don't return a generator object AND an interface node class object.
Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:77: """Return list of
Attribute object.
On 2015/09/07 10:35:25, Yuki wrote:
> What's "Attribute object"?
> Do we have Attribute class?
Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:81: list which is Attribute
object list
On 2015/09/07 10:35:25, Yuki wrote:
> Returns list which is ... list??
>
> Why not simply "Returns a ... list" or "Returns a list of ..."?
Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:87: """Return type of
attribute or operation's type.
On 2015/09/07 10:35:25, Yuki wrote:
> type of type?
>
> What about argument?
Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:100: """Return a generator
which yields Extattribute's object dictionary
On 2015/09/07 10:35:25, Yuki wrote:
> Do we have Extattribute class?
No, I mixed up definitions.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:102: node: interface,
attribute or operation node object#node which has extattr object
On 2015/09/07 10:35:25, Yuki wrote:
> #node which has extattr object?
Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:107: for extattributes in
node.GetListOf('ExtAttributes'):
On 2015/09/07 10:35:25, Yuki wrote:
> extattributes?
I changed extattribute_list.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:244: def
format_interface_dict(interface_node):
On 2015/09/07 10:35:25, Yuki wrote:
> I'd say "format an interface node in/to/into/as??? a dictionary."
> So, I'd write "format_interface_to_dict".
Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:251: interface_dict = {}
On 2015/09/07 10:35:26, Yuki wrote:
> Please be consistent in the same style.
Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:274:
interface['ExtAttributes'].append(partial['ExtAttributes'])
On 2015/09/07 10:35:25, Yuki wrote:
> For extended attributes, I doubt that we should add ones of partial interface
to
> non-partial interface. Let's check with Bashi-san.
Add todo to merge partial interface of extended Attributes.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:276: if interface['Constant']:
On 2015/09/07 10:35:25, Yuki wrote:
> Why should you have this if-clause?
I thought it might be wrong if interface['Constant'] is empty.
I tried without if statement, and I got error.
This time, I could get the same data without if statement.
I guess I did wrong experimentation.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:290:
dictionary.setdefault(interface_dict['Name'], interface_dict)
On 2015/09/07 10:35:25, Yuki wrote:
> This is abuse of setdefault.
Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:303: filename = json_file
On 2015/09/07 10:35:26, Yuki wrote:
> ?
I named argument without any dealing.
Done.
https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script...
Source/bindings/scripts/collect_idls_into_json.py:310: path_filename = args[0]
On 2015/09/07 10:35:25, Yuki wrote:
> path_filename??
Done.
https://codereview.chromium.org/1323493002/diff/200001/Source/bindings/script... File Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1323493002/diff/200001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:13: chromium_path = os.path.abspath( We should no longer need this hack. https://codereview.chromium.org/1323493002/diff/200001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:27: def load_filepaths(path_file): We should no longer need this function. https://codereview.chromium.org/1323493002/diff/200001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:34: def get_interfaces(path): path? Also inconsistent with the comment.
natsukoa@google.com changed reviewers: - shimadaa@google.com
Clean up the code https://codereview.chromium.org/1323493002/diff/200001/Source/bindings/script... File Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1323493002/diff/200001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:13: chromium_path = os.path.abspath( On 2015/09/10 10:23:04, Yuki wrote: > We should no longer need this hack. Done. https://codereview.chromium.org/1323493002/diff/200001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:27: def load_filepaths(path_file): On 2015/09/10 10:23:04, Yuki wrote: > We should no longer need this function. Done.
I still see some inconsistencies here. Could you review the code for yourself and make it consistent? https://codereview.chromium.org/1323493002/diff/300001/Source/bindings/script... File Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1323493002/diff/300001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:106: def extattr_dict(extattribute): extattr_to_dict https://codereview.chromium.org/1323493002/diff/300001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:130: 'ExtAttributes': [extattr for extattr in extattr_dict(get_extattributes(attribute))], list(extattr_dict(...)) https://codereview.chromium.org/1323493002/diff/300001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:205: def get_inherit(interface_node): Let's remove get_inherit and inherit_dict for the first version. https://codereview.chromium.org/1323493002/diff/300001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:262: def get_name(interface_node): get_name(my_interface) is not shorter or simpler than my_interface.GetName() Do you really want this function? https://codereview.chromium.org/1323493002/diff/300001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:266: def get_dict(interface_node): interface_to_dict
Fix list comprehension without any operation. Remove get_inherit() and get_name() because these can be able to handle within making dictionary function. https://codereview.chromium.org/1323493002/diff/300001/Source/bindings/script... File Source/bindings/scripts/collect_idls_into_json.py (right): https://codereview.chromium.org/1323493002/diff/300001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:106: def extattr_dict(extattribute): On 2015/09/17 05:45:06, Yuki wrote: > extattr_to_dict Done. https://codereview.chromium.org/1323493002/diff/300001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:130: 'ExtAttributes': [extattr for extattr in extattr_dict(get_extattributes(attribute))], On 2015/09/17 05:45:06, Yuki wrote: > list(extattr_dict(...)) Done. https://codereview.chromium.org/1323493002/diff/300001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:205: def get_inherit(interface_node): On 2015/09/17 05:45:06, Yuki wrote: > Let's remove get_inherit and inherit_dict for the first version. Done. https://codereview.chromium.org/1323493002/diff/300001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:262: def get_name(interface_node): On 2015/09/17 05:45:06, Yuki wrote: > get_name(my_interface) > > is not shorter or simpler than > > my_interface.GetName() > > Do you really want this function? Actually, I don't need it. I just removed it. https://codereview.chromium.org/1323493002/diff/300001/Source/bindings/script... Source/bindings/scripts/collect_idls_into_json.py:266: def get_dict(interface_node): On 2015/09/17 05:45:06, Yuki wrote: > interface_to_dict Done.
Patchset #15 (id:340001) has been deleted
Patchset #15 (id:360001) has been deleted
Patchset #16 (id:400001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
