Add extended attributes to IdlCallbackFunction

It is necessary to deal with callback function.

BUG=569301
Committed: https://crrev.com/e98eb22361144dcb3237ba6b999fa3dbd91498c6
Cr-Commit-Position: refs/heads/master@{#415256}

[lkawai, 2016-08-30 05:21:07]

lkawai@google.com changed reviewers:
+ bashi@chromium.org, haraken@chromium.org, peria@chromium.org,
yukishiino@chromium.org

[lkawai, 2016-08-30 05:21:07]

PTAL

[haraken, 2016-08-30 05:27:57]

LGTM

https://codereview.chromium.org/2290113002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/2290113002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:184: raise
ValueError('Expected 2 or 3 children, got %s' % num_children)

I'd prefer the following pattern:

if num_children < 2 or num_children > 3:
  raise ValueError(...)
type_node = children[0]
arguments_node = children[1]
if num_children == 3:
  self.extended_attributes = ext_attributes_node_to_extended_attributes(...)
else:
  self.extended_attributes = {}


c.f., see how the IdlConstant class is implemented.

[bashi, 2016-08-30 05:32:47]

lgtm with haraken-san's comment addressed.

Could you update the description like below?

---
[one line summary]

[some lines of explanation]

BUG=[bug number]
---

For example,

---
Add extended attributes to IdlCallbackFunction

It is necessary ...

BUG=...
---

[lkawai, 2016-08-30 05:45:28]

Description was changed from

==========
Add extended attributes to IdlCallbackFunction. It is necessary to
deal with callback function.

BUG=569301
==========

to

==========
It is necessary to deal with callback function.

BUG=569301
==========

[Yuki, 2016-08-30 05:49:11]

LGTM.

https://codereview.chromium.org/2290113002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/2290113002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:184: raise
ValueError('Expected 2 or 3 children, got %s' % num_children)
On 2016/08/30 05:27:57, haraken wrote:
> 
> I'd prefer the following pattern:
> 
> if num_children < 2 or num_children > 3:
>   raise ValueError(...)
> type_node = children[0]
> arguments_node = children[1]
> if num_children == 3:
>   self.extended_attributes = ext_attributes_node_to_extended_attributes(...)
> else:
>   self.extended_attributes = {}
> 
> 
> c.f., see how the IdlConstant class is implemented.

Lisa's code looks more Python's way than haraken's suggestion, and easier for me
to read.  Although it's also reasonable to keep consistency with the rest of
lines in this file.

[lkawai, 2016-08-30 05:57:01]

PTAL

https://codereview.chromium.org/2290113002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/2290113002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:184: raise
ValueError('Expected 2 or 3 children, got %s' % num_children)
On 2016/08/30 05:27:57, haraken wrote:
> 
> I'd prefer the following pattern:
> 
> if num_children < 2 or num_children > 3:
>   raise ValueError(...)
> type_node = children[0]
> arguments_node = children[1]
> if num_children == 3:
>   self.extended_attributes = ext_attributes_node_to_extended_attributes(...)
> else:
>   self.extended_attributes = {}
> 
> 
> c.f., see how the IdlConstant class is implemented.

Done.

https://codereview.chromium.org/2290113002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:184: raise
ValueError('Expected 2 or 3 children, got %s' % num_children)
On 2016/08/30 05:49:11, Yuki wrote:
> On 2016/08/30 05:27:57, haraken wrote:
> > 
> > I'd prefer the following pattern:
> > 
> > if num_children < 2 or num_children > 3:
> >   raise ValueError(...)
> > type_node = children[0]
> > arguments_node = children[1]
> > if num_children == 3:
> >   self.extended_attributes = ext_attributes_node_to_extended_attributes(...)
> > else:
> >   self.extended_attributes = {}
> > 
> > 
> > c.f., see how the IdlConstant class is implemented.
> 
> Lisa's code looks more Python's way than haraken's suggestion, and easier for
me
> to read.  Although it's also reasonable to keep consistency with the rest of
> lines in this file.

Acknowledged.

[Yuki, 2016-08-30 05:59:41]

LGTM.

https://codereview.chromium.org/2290113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/2290113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:186:
self.extended_attributes = {}
You already set self.extended_attributes on line 176.
You don't need this else-clause.

[haraken, 2016-08-30 06:00:58]

LGTM

[lkawai, 2016-08-30 06:20:58]

https://codereview.chromium.org/2290113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/2290113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:186:
self.extended_attributes = {}
On 2016/08/30 05:59:41, Yuki wrote:
> You already set self.extended_attributes on line 176.
> You don't need this else-clause.

Done.

[lkawai, 2016-08-30 06:21:52]

The CQ bit was checked by lkawai@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 06:22:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 08:08:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 08:08:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lkawai, 2016-08-30 08:10:29]

The CQ bit was checked by lkawai@google.com

[lkawai, 2016-08-30 08:10:30]

The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org,
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2290113002/#ps40001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-08-30 08:10:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 09:39:23]

Description was changed from

==========
It is necessary to deal with callback function.

BUG=569301
==========

to

==========
It is necessary to deal with callback function.

BUG=569301
==========

[commit-bot: I haz the power, 2016-08-30 09:39:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-30 09:40:42]

Description was changed from

==========
It is necessary to deal with callback function.

BUG=569301
==========

to

==========
It is necessary to deal with callback function.

BUG=569301

Committed: https://crrev.com/e98eb22361144dcb3237ba6b999fa3dbd91498c6
Cr-Commit-Position: refs/heads/master@{#415256}
==========

[commit-bot: I haz the power, 2016-08-30 09:40:43]

Patchset 3 (id:??) landed as
https://crrev.com/e98eb22361144dcb3237ba6b999fa3dbd91498c6
Cr-Commit-Position: refs/heads/master@{#415256}