collect idls and organize interface node (issue 1376673003 by natsukoa@google.com)

[nats...@google.com]

Reviewers: Yuki, bashi1, haraken,

Message:
upload as a test

Description:
Collects IDL file paths under the directory and extracts file paths to text
file.
Then extracts each dictionary of interface node's information.

Please review this at https://codereview.chromium.org/1376673003/

Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Affected files (+350, -0 lines):
A third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py
A third_party/WebKit/Source/bindings/scripts/interface_node_path.py

[nats...@google.com]

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/

[ba...@chromium.org]

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/

[yukis...@chromium.org]

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#newcode79
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/Source/bindings/scripts/collect_idls_into_json.py#newcode138
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/Source/bindings/scripts/collect_idls_into_json.py#newcode210
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/Source/bindings/scripts/collect_idls_into_json.py#newcode278
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.

https://codereview.chromium.org/1376673003/

[nats...@google.com]

I fixed some code and comments. Would you check the script again?
Thank you.

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

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/Source/bindings/scripts/collect_idls_into_json.py#newcode11
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/Source/bindings/scripts/collect_idls_into_json.py#newcode16
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/Source/bindings/scripts/collect_idls_into_json.py#newcode17
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/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.

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/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)

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/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):

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/Source/bindings/scripts/collect_idls_into_json.py#newcode41
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/Source/bindings/scripts/collect_idls_into_json.py#newcode138
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/Source/bindings/scripts/collect_idls_into_json.py#newcode278
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/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):

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/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():

On 2015/09/30 03:13:16, bashi1 wrote:
> key -> interface_name ?

Done.

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

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/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)]

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/

[yukis...@chromium.org]

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#newcode79
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/Source/bindings/scripts/collect_idls_into_json.py#newcode210
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/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/100001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode18
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/Source/bindings/scripts/collect_idls_into_json.py#newcode38
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/Source/bindings/scripts/collect_idls_into_json.py#newcode99
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/Source/bindings/scripts/collect_idls_into_json.py#newcode358
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/

[ba...@chromium.org]

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#newcode27
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/Source/bindings/scripts/blink_idl_parser.py&l=381

https://codereview.chromium.org/1376673003/

[nats...@google.com]

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/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#newcode27
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/Source/bindings/scripts/blink_idl_parser.py&l=381

oh...
I got it.

https://codereview.chromium.org/1376673003/diff/40001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode79
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/Source/bindings/scripts/collect_idls_into_json.py#newcode210
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/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/100001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode18
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/Source/bindings/scripts/collect_idls_into_json.py#newcode38
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/Source/bindings/scripts/collect_idls_into_json.py#newcode99
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/Source/bindings/scripts/collect_idls_into_json.py#newcode358
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.

https://codereview.chromium.org/1376673003/

[yukis...@chromium.org]

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/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#newcode210
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/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/140001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode39
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/Source/bindings/scripts/collect_idls_into_json.py#newcode45
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/Source/bindings/scripts/collect_idls_into_json.py#newcode51
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/Source/bindings/scripts/collect_idls_into_json.py#newcode52
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/Source/bindings/scripts/collect_idls_into_json.py#newcode56
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/Source/bindings/scripts/collect_idls_into_json.py#newcode80
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/Source/bindings/scripts/collect_idls_into_json.py#newcode84
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/Source/bindings/scripts/collect_idls_into_json.py#newcode91
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/Source/bindings/scripts/collect_idls_into_json.py#newcode105
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/Source/bindings/scripts/collect_idls_into_json.py#newcode123
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/Source/bindings/scripts/collect_idls_into_json.py#newcode123
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/Source/bindings/scripts/collect_idls_into_json.py#newcode162
third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py:162:
attribute_nodes: list of attribute node object
attribute_nodes = > attribute_node

https://codereview.chromium.org/1376673003/

[nats...@google.com]

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/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/140001/third_party/WebKit/Source/bindings/scripts/collect_idls_into_json.py#newcode45
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/Source/bindings/scripts/collect_idls_into_json.py#newcode51
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/Source/bindings/scripts/collect_idls_into_json.py#newcode52
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/Source/bindings/scripts/collect_idls_into_json.py#newcode56
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/Source/bindings/scripts/collect_idls_into_json.py#newcode80
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/Source/bindings/scripts/collect_idls_into_json.py#newcode84
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/Source/bindings/scripts/collect_idls_into_json.py#newcode91
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/Source/bindings/scripts/collect_idls_into_json.py#newcode105
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/Source/bindings/scripts/collect_idls_into_json.py#newcode123
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/Source/bindings/scripts/collect_idls_into_json.py#newcode162
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.

https://codereview.chromium.org/1376673003/

[yukis...@chromium.org]

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?


https://codereview.chromium.org/1376673003/