Add [TypeChecking=Interface] to the V8Path2D interface

Add [TypeChecking=Interface] to the V8Path2D interface

This affects the generated code of the constructor, which will now throw
a TypeError if "parameter 1 is not of type 'Path2D'."

However, this code path is unreachable, because there's a type check
(V8Path2D::hasInstance()) to determine that this form of the constructor
should be used.

Promoting [TypeChecking=Interface] to the interface level will make it
possible to make [TypeChecking=Interface] the default without changing
the generated code for this interface.

BUG=462561
Committed: https://crrev.com/884d8f3e48d2b2fd8dc1e75555c0f36f307d44fe
Cr-Commit-Position: refs/heads/master@{#361317}

[philipj_slow, 2015-11-21 20:14:21]

The CQ bit was checked by philipj@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-21 20:14:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1471563002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1471563002/1

[philipj_slow, 2015-11-21 21:28:49]

philipj@opera.com changed reviewers:
+ bashi@chromium.org, jl@opera.com

[philipj_slow, 2015-11-21 21:28:49]

PTAL!

There must be many more cases with redundant type checking, do you think it
would be worth trying to eliminate that in the bindings generator? If so, I can
file a bug.

[commit-bot: I haz the power, 2015-11-22 04:52:14]

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

[commit-bot: I haz the power, 2015-11-22 04:52:15]

Dry run: This issue passed the CQ dry run.

[Justin Novosad, 2015-11-23 15:17:12]

On 2015/11/22 04:52:15, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

We seem to be lacking layout test coverage for this.

[philipj_slow, 2015-11-23 16:14:29]

On 2015/11/23 15:17:12, Justin Novosad wrote:
> On 2015/11/22 04:52:15, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> We seem to be lacking layout test coverage for this.

As noted in the description and comment #4, the change to the generated code
cannot be observed, and ideally shouldn't be emitted at all. There is an
existing test for the constructor in
fast/canvas/script-tests/canvas-path-constructors.js

[bashi, 2015-11-24 00:10:15]

Description was changed from

==========
Add [TypeChecking=Interface] to the V8Path2D interface

This affects the generated code of the constructor, which will now throw
a TypeError if "parameter 1 is not of type 'Path2D'."

However, this code path is unreachable, because there's a type check
(V8Path2D::hasInstance()) to determine that this form of the constructor
should be used.

Promoting [TypeChecking=Interface] to the interface level will make it
possible to make [TypeChecking=Interface] the default without changing
the generated code for this interface.

BUG=462561
==========

to

==========
Add [TypeChecking=Interface] to the V8Path2D interface

This affects the generated code of the constructor, which will now throw
a TypeError if "parameter 1 is not of type 'Path2D'."

However, this code path is unreachable, because there's a type check
(V8Path2D::hasInstance()) to determine that this form of the constructor
should be used.

Promoting [TypeChecking=Interface] to the interface level will make it
possible to make [TypeChecking=Interface] the default without changing
the generated code for this interface.

BUG=462561
==========

[bashi, 2015-11-24 00:10:15]

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

[bashi, 2015-11-24 00:12:13]

On 2015/11/21 21:28:49, philipj wrote:
> PTAL!
> 
> There must be many more cases with redundant type checking, do you think it
> would be worth trying to eliminate that in the bindings generator? If so, I
can
> file a bug.

+yukishiino@ would be a better reviewer than me for type checking :)

[Yuki, 2015-11-24 06:46:56]

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

[Yuki, 2015-11-24 06:46:57]

The CL itself looks good to me.


+haraken@,

I'm now thinking it's better to remove the support of [TypeChecking=Interface]. 
Seeing the spec and our implementation, I think we can enable
[TypeChecking=Interface] by default and get rid of our own extended attribute of
[TypeChecking=Interface].  Node.idl and Element.idl already have
[TypeChecking=Interface].  HTMLElement.idl only has |contextMenu| attribute
setter which will be affected.

I expect that we wouldn't see much performance regression or behavioral
difference.  What do you guys think?  (For the time being, we'll live with other
types of TypeChecking such as [TypeChecking=Unrestricted].)

[Yuki, 2015-11-24 06:47:44]

Oops, "looks good to me" didn't work.  lgtm.

[haraken, 2015-11-24 06:57:48]

LGTM

[philipj_slow, 2015-11-24 07:43:41]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2015-11-24 07:44:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1471563002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1471563002/1

[philipj_slow, 2015-11-24 07:45:32]

On 2015/11/24 06:46:57, Yuki wrote:
> The CL itself looks good to me.
> 
> 
> +haraken@,
> 
> I'm now thinking it's better to remove the support of
[TypeChecking=Interface]. 
> Seeing the spec and our implementation, I think we can enable
> [TypeChecking=Interface] by default and get rid of our own extended attribute
of
> [TypeChecking=Interface].  Node.idl and Element.idl already have
> [TypeChecking=Interface].  HTMLElement.idl only has |contextMenu| attribute
> setter which will be affected.
> 
> I expect that we wouldn't see much performance regression or behavioral
> difference.  What do you guys think?  (For the time being, we'll live with
other
> types of TypeChecking such as [TypeChecking=Unrestricted].)

Getting rid of [TypeChecking=Interface] is indeed the reason I prepared this CL.
Once this and https://codereview.chromium.org/1464133002/ has landed, it's
possible to make [TypeChecking=Interface] the default with no changes to the
generated code, and https://codereview.chromium.org/1466563003/#ps20001 is the
WIP CL for that.

[Yuki, 2015-11-24 08:01:42]

On 2015/11/24 07:45:32, philipj wrote:
> On 2015/11/24 06:46:57, Yuki wrote:
> > The CL itself looks good to me.
> > 
> > 
> > +haraken@,
> > 
> > I'm now thinking it's better to remove the support of
> [TypeChecking=Interface]. 
> > Seeing the spec and our implementation, I think we can enable
> > [TypeChecking=Interface] by default and get rid of our own extended
attribute
> of
> > [TypeChecking=Interface].  Node.idl and Element.idl already have
> > [TypeChecking=Interface].  HTMLElement.idl only has |contextMenu| attribute
> > setter which will be affected.
> > 
> > I expect that we wouldn't see much performance regression or behavioral
> > difference.  What do you guys think?  (For the time being, we'll live with
> other
> > types of TypeChecking such as [TypeChecking=Unrestricted].)
> 
> Getting rid of [TypeChecking=Interface] is indeed the reason I prepared this
CL.
> Once this and https://codereview.chromium.org/1464133002/ has landed, it's
> possible to make [TypeChecking=Interface] the default with no changes to the
> generated code, and https://codereview.chromium.org/1466563003/#ps20001 is the
> WIP CL for that.

Great!  Thanks for working on this.

[commit-bot: I haz the power, 2015-11-24 08:56:01]

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

[commit-bot: I haz the power, 2015-11-24 08:56:02]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[philipj_slow, 2015-11-24 10:02:04]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2015-11-24 10:04:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1471563002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1471563002/1

[commit-bot: I haz the power, 2015-11-24 11:35:15]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-11-24 11:36:43]

Patchset 1 (id:??) landed as
https://crrev.com/884d8f3e48d2b2fd8dc1e75555c0f36f307d44fe
Cr-Commit-Position: refs/heads/master@{#361317}