IDL: Add [Custom=CallEpilogue] to eliminate CG special cases

IDL: Add [Custom=CallEpilogue] to eliminate CG special cases

The effect is similar to having [Custom] on a method, but instead of
completely replacing the generated method implementation, the custom
callback is just called after the regular method implementation call.

Use this to reimplement EventTarget.{add,remove}EventListener and
MediaQueryList.{add,remove}Listener's code generation special cases.

Also use this to reduce the custom code for History.{push,replace}State
to such a call epilogue. While doing so, update History.idl so that the
now generated code is behaves the same as the old custom code.

BUG=345519
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192146

[Jens Widell, 2015-03-18 11:41:19]

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

[Jens Widell, 2015-03-18 11:41:19]

PTAL

Note that I'm not completely convinced myself this is something we should do
(like this), and so I'm open to "let's not".

Here's how I'm thinking:

- It's marginally nicer with custom code than code generation special cases;
e.g. because it makes the custom stuff stand out more and thus easier to spot.
(Currently, EventTarget.idl contains no hints that magic things happen in the CG
for its methods, for instance.)

- Reducing V8HistoryCustom.cpp to just that which is actually different from
what normal CG produces is nice.

On the other hand:

- [Custom=CallEpilogue] is used so rarely it is itself a CG special case, pretty
much.

- All of these epilogues could simply be moved into the implementations (perhaps
by calling back to helpers in bindings/.)

[Jens Widell, 2015-03-18 11:50:36]

On 2015/03/18 11:41:19, Jens Widell wrote:
> - All of these epilogues could simply be moved into the implementations
(perhaps
> by calling back to helpers in bindings/.)

Actually, this might not be trivially true. The custom epilogues do currently
depend on such things as info.Holder() and an argument's actual v8::Value (not
the converted value) and thus might require additional work/redesign at some
level to be moved to the implementations.

[haraken, 2015-03-18 13:28:46]

I'd +1 to the idea if there are more use cases :)

[vivekg, 2015-03-18 13:44:37]

On 2015/03/18 at 13:28:46, haraken wrote:
> I'd +1 to the idea if there are more use cases :)

I like the epilogue approach which can eliminate the need of handling cases such
as, https://codereview.chromium.org/1007113002, explicitly. WDYT?

[vivekg, 2015-03-18 13:48:14]

On 2015/03/18 at 13:44:37, vivekg_ wrote:
> On 2015/03/18 at 13:28:46, haraken wrote:
> > I'd +1 to the idea if there are more use cases :)
> 
> I like the epilogue approach which can eliminate the need of handling cases
such as, https://codereview.chromium.org/1007113002, explicitly. WDYT?

Just to be on the same page, would the [Custom=CallEpilogue] generate the code
for primitive checks such as minimum arity etc. just like non-custom methods? 
If we can also have some test coverage to peek at the generated code from such
an epilogue, that would be great as well.

[Jens Widell, 2015-03-18 13:51:20]

On 2015/03/18 13:48:14, vivekg_ wrote:
> On 2015/03/18 at 13:44:37, vivekg_ wrote:
> > On 2015/03/18 at 13:28:46, haraken wrote:
> > > I'd +1 to the idea if there are more use cases :)
> > 
> > I like the epilogue approach which can eliminate the need of handling cases
> such as, https://codereview.chromium.org/1007113002, explicitly. WDYT?
> 
> Just to be on the same page, would the [Custom=CallEpilogue] generate the code
> for primitive checks such as minimum arity etc. just like non-custom methods? 
> If we can also have some test coverage to peek at the generated code from such
> an epilogue, that would be great as well.

Essentially, a method with [Custom=CallEpilogue] generates the same code as a
regular method, except that at the end it has the call
"V8<class>::<method>MethodEpilogueCustom(info)".

I'll add a test, if we agree to do this.

[vivekg, 2015-03-18 13:54:29]

On 2015/03/18 at 13:51:20, jl wrote:
> On 2015/03/18 13:48:14, vivekg_ wrote:
> > On 2015/03/18 at 13:44:37, vivekg_ wrote:
> > > On 2015/03/18 at 13:28:46, haraken wrote:
> > > > I'd +1 to the idea if there are more use cases :)
> > > 
> > > I like the epilogue approach which can eliminate the need of handling
cases
> > such as, https://codereview.chromium.org/1007113002, explicitly. WDYT?
> > 
> > Just to be on the same page, would the [Custom=CallEpilogue] generate the
code
> > for primitive checks such as minimum arity etc. just like non-custom
methods? 
> > If we can also have some test coverage to peek at the generated code from
such
> > an epilogue, that would be great as well.
> 
> Essentially, a method with [Custom=CallEpilogue] generates the same code as a
regular method, except that at the end it has the call
"V8<class>::<method>MethodEpilogueCustom(info)".
> 
> I'll add a test, if we agree to do this.

I am supportive of such an addition (non-owner though :) ). WDYT, haraken@?

[Jens Widell, 2015-03-18 13:58:11]

On 2015/03/18 13:28:46, haraken wrote:
> I'd +1 to the idea if there are more use cases :)

I think V8MessageChannel::constructorCustom() could potentially benefit from
this. That's the only other use case I could find in the current set of custom
code.

Being a constructor the [Custom=CallEpilogue] syntax doesn't work, of course.

[haraken, 2015-03-18 14:02:55]

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

[haraken, 2015-03-18 14:02:55]

bashi-san, shiino-san: What do you think?

[Yuki, 2015-03-18 14:29:23]

On 2015/03/18 14:02:55, haraken wrote:
> bashi-san, shiino-san: What do you think?

Interesting idea.  Looks better than the current situation.

IIUC, the custom function is called after impl->method() is called.
So we can pass |impl| to the custom function in addition to |info|,
can't we?  Then, we don't need to extract impl from info twice.

[Jens Widell, 2015-03-18 14:38:17]

On 2015/03/18 14:29:23, Yuki wrote:
> On 2015/03/18 14:02:55, haraken wrote:
> > bashi-san, shiino-san: What do you think?
> 
> Interesting idea.  Looks better than the current situation.
> 
> IIUC, the custom function is called after impl->method() is called.
> So we can pass |impl| to the custom function in addition to |info|,
> can't we?  Then, we don't need to extract impl from info twice.

Yeah, we could pass impl there without much trouble.

[bashi, 2015-03-19 01:34:39]

I like this idea :)

[haraken, 2015-03-19 02:01:26]

On 2015/03/19 01:34:39, bashi1 wrote:
> I like this idea :)

+1.

[Jens Widell, 2015-03-19 07:45:55]

PS#4 adjusts History.idl to match the changes made in r192095 (second arguments
to pushState/replaceState no longer optional).

So, do we have an agreement that this is in fact an improvement? :-)

[Yuki, 2015-03-19 08:05:16]

lgtm

[vivekg, 2015-03-19 08:07:36]

On 2015/03/19 at 07:45:55, jl wrote:
> PS#4 adjusts History.idl to match the changes made in r192095 (second
arguments to pushState/replaceState no longer optional).
> 
> So, do we have an agreement that this is in fact an improvement? :-)

yay!! looks great. Thanks for the idea. lgtm :)

[haraken, 2015-03-19 08:58:32]

LGTM. I believe this will lead to less bugs :)

[Jens Widell, 2015-03-19 09:00:04]

Thank you all for reviewing!

[Jens Widell, 2015-03-19 09:00:09]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2015-03-19 09:00:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1019453002/60001

[vivekg, 2015-03-19 09:05:59]

Can you or someone with access to
www.chromium.org/blink/webidl/blink-idl-extended-attributes update the
information about this new addition?

[commit-bot: I haz the power, 2015-03-19 09:56:55]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192146