Use real IDL types in unit test stubs.

Use real IDL types in unit test stubs.

Instead of using generic stub objects, use actual
types, because that is more accurate.

R=bashi
BUG=673214
Committed: https://crrev.com/32158f86547acf2ae9ba9997674ff2ac376cb631
Cr-Commit-Position: refs/heads/master@{#439048}

[dglazkov, 2016-12-15 04:39:39]

The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 04:39:56]

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

[dglazkov, 2016-12-15 04:40:42]

Description was changed from

==========
Use real IDL types in unit test stubs.

Instead of using generic stub objects, use actual
types, because that is more accurate.

R=bashi
BUG=
==========

to

==========
Use real IDL types in unit test stubs.

Instead of using generic stub objects, use actual
types, because that is more accurate.

R=bashi
BUG=673214
==========

[dglazkov, 2016-12-15 04:40:50]

PTAL.

[haraken, 2016-12-15 04:43:12]

LGTM

[bashi, 2016-12-15 04:51:14]

bashi@chromium.org changed reviewers:
+ bashi@chromium.org

[bashi, 2016-12-15 04:51:15]

lgtm with one comment.

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

https://codereview.chromium.org/2581613002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:454: # Defaults,
overridden below
nit: I think we can remove this comment (it's obvious).

https://codereview.chromium.org/2581613002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:456:
self.extended_attributes = {}
Can we move these two lines before 'if not node:' line? I think having default
values is good to avoid an AttributeError as code generator assumes that there
will be these attributes.

[dglazkov, 2016-12-15 04:54:51]

On 2016/12/15 at 04:51:15, bashi wrote:
> lgtm with one comment.
> 
>
https://codereview.chromium.org/2581613002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/scripts/idl_definitions.py (right):
> 
>
https://codereview.chromium.org/2581613002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/scripts/idl_definitions.py:454: # Defaults,
overridden below
> nit: I think we can remove this comment (it's obvious).
> 
>
https://codereview.chromium.org/2581613002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/scripts/idl_definitions.py:456:
self.extended_attributes = {}
> Can we move these two lines before 'if not node:' line? I think having default
values is good to avoid an AttributeError as code generator assumes that there
will be these attributes.

Sure thing! Done.

[dglazkov, 2016-12-15 04:55:06]

Feedback addressed.

[dglazkov, 2016-12-15 04:55:06]

The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 04:55:18]

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

[peria, 2016-12-15 04:57:08]

peria@chromium.org changed reviewers:
+ peria@chromium.org

[peria, 2016-12-15 04:57:09]

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

https://codereview.chromium.org/2581613002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:451:
self.is_read_only = bool(node.GetProperty('READONLY'))
Could you merge these 3 lines with the new lines?

  self.is_read_only = bool(node....) if node else False
  self.is_static = bool(node...) if node else False
  self.name = node.GetName if node else None

I'm afraid setting no default values will cause errors.

https://codereview.chromium.org/2581613002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:458: children =
node.GetChildren()
ditto.
  children = node.GetChildren() if node else []

[dglazkov, 2016-12-15 05:24:23]

dglazkov@chromium.org changed reviewers:
+ haraken@chromium.org

[dglazkov, 2016-12-15 05:24:24]

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

https://codereview.chromium.org/2581613002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:451:
self.is_read_only = bool(node.GetProperty('READONLY'))
On 2016/12/15 at 04:57:08, peria wrote:
> Could you merge these 3 lines with the new lines?
> 
>   self.is_read_only = bool(node....) if node else False
>   self.is_static = bool(node...) if node else False
>   self.name = node.GetName if node else None
> 
> I'm afraid setting no default values will cause errors.

They are set above, right?

[peria, 2016-12-15 05:33:40]

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

https://codereview.chromium.org/2581613002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:451:
self.is_read_only = bool(node.GetProperty('READONLY'))
On 2016/12/15 05:24:24, dglazkov wrote:
> On 2016/12/15 at 04:57:08, peria wrote:
> > Could you merge these 3 lines with the new lines?
> > 
> >   self.is_read_only = bool(node....) if node else False
> >   self.is_static = bool(node...) if node else False
> >   self.name = node.GetName if node else None
> > 
> > I'm afraid setting no default values will cause errors.
> 
> They are set above, right?

Yes, on this CL.
But my concern is same with bashi-san's comment on #456.
I hope we don't have an early return, if possible.

[commit-bot: I haz the power, 2016-12-15 06:22:01]

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

[commit-bot: I haz the power, 2016-12-15 06:22:02]

Dry run: This issue passed the CQ dry run.

[dglazkov, 2016-12-16 02:49:03]

On 2016/12/15 at 05:33:40, peria wrote:
> I hope we don't have an early return, if possible.

I am curious why? Early returns are a good pattern. Look, even IdlOperation uses
it.

[peria, 2016-12-16 02:53:21]

On 2016/12/16 02:49:03, dglazkov wrote:
> On 2016/12/15 at 05:33:40, peria wrote:
> > I hope we don't have an early return, if possible.
> 
> I am curious why? Early returns are a good pattern. Look, even IdlOperation
uses
> it.

In future updates, it can easily introduce undefined errors
like you fixed in PS2.

[dglazkov, 2016-12-16 03:00:25]

Without early returns.

[dglazkov, 2016-12-16 03:00:26]

The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 03:01:28]

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

[dglazkov, 2016-12-16 03:17:38]

PTAL.

[peria, 2016-12-16 04:09:20]

On 2016/12/16 03:17:38, dglazkov wrote:
> PTAL.

Thank you for the change.
LGTM

[commit-bot: I haz the power, 2016-12-16 06:08:08]

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

[commit-bot: I haz the power, 2016-12-16 06:08:09]

Dry run: This issue passed the CQ dry run.

[dglazkov, 2016-12-16 06:26:07]

The CQ bit was checked by dglazkov@chromium.org

[dglazkov, 2016-12-16 06:26:08]

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

[commit-bot: I haz the power, 2016-12-16 06:26:27]

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-12-16 06:32:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481869567871530,
"parent_rev": "8a45e2ee2841fd432fe2a069f646125f850a7e15", "commit_rev":
"d84f2c117bcd96bd05bde26d06974fa471b33e8b"}

[commit-bot: I haz the power, 2016-12-16 06:33:21]

Description was changed from

==========
Use real IDL types in unit test stubs.

Instead of using generic stub objects, use actual
types, because that is more accurate.

R=bashi
BUG=673214
==========

to

==========
Use real IDL types in unit test stubs.

Instead of using generic stub objects, use actual
types, because that is more accurate.

R=bashi
BUG=673214

Review-Url: https://codereview.chromium.org/2581613002
==========

[commit-bot: I haz the power, 2016-12-16 06:33:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-16 06:35:15]

Description was changed from

==========
Use real IDL types in unit test stubs.

Instead of using generic stub objects, use actual
types, because that is more accurate.

R=bashi
BUG=673214

Review-Url: https://codereview.chromium.org/2581613002
==========

to

==========
Use real IDL types in unit test stubs.

Instead of using generic stub objects, use actual
types, because that is more accurate.

R=bashi
BUG=673214

Committed: https://crrev.com/32158f86547acf2ae9ba9997674ff2ac376cb631
Cr-Commit-Position: refs/heads/master@{#439048}
==========

[commit-bot: I haz the power, 2016-12-16 06:35:16]

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