Add counters for add/removeEventListener() called with one argument

Add counters for add/removeEventListener() called with one argument

This was attempted previously but only worked for no arguments:
https://codereview.chromium.org/815503002
https://codereview.chromium.org/928103002

The enum values are added in their original positions in UseCounter.h,
as they were always unreachable and thus not polluted by anything else.

The reason was that there is no generated impl-> call with one
argument, as EventListener has special handling in the bindings
generator such that it's effectively treated as having a default null
argument.

Give all the arguments default values in the IDL so that there is only
one impl-> call in the generated code, with the prologue and epilogue
immediately before and after it. This makes it more clear that the
prologue will always be called, and makes this purely a bindings bug.

The removed FIXMEs in EventTarget.cpp were wrong, it should be
possible to pass null for the listener argument and per spec that
should do nothing.

Rather than extending support for [Conditional] here, remove it for
[Custom=CallEpilogue] as it's actually never used there, nor tested.

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

[philipj_slow, 2015-07-09 22:08:56]

philipj@opera.com changed reviewers:
+ haraken@chromium.org

[philipj_slow, 2015-07-09 22:08:57]

PTAL

Depends on https://codereview.chromium.org/1227823009/

[haraken, 2015-07-09 23:37:59]

https://codereview.chromium.org/1233483002/diff/1/Source/core/frame/UseCounter.h
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/1233483002/diff/1/Source/core/frame/UseCounte...
Source/core/frame/UseCounter.h:563: AddEventListenerOneArgument = 657,

I'm wondering why the entry 657 and 659 were missing. Isn't there any risk that
we reuse the entry and see mixed results in the UMA dashboard?

[philipj_slow, 2015-07-10 07:46:47]

https://codereview.chromium.org/1233483002/diff/1/Source/core/frame/UseCounter.h
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/1233483002/diff/1/Source/core/frame/UseCounte...
Source/core/frame/UseCounter.h:563: AddEventListenerOneArgument = 657,
On 2015/07/09 23:37:58, haraken wrote:
> 
> I'm wondering why the entry 657 and 659 were missing. Isn't there any risk
that
> we reuse the entry and see mixed results in the UMA dashboard?

These were the original entries for 657 and 659, but it was accidentally dead
code and removed in https://codereview.chromium.org/928103002

I've tried to be clarify in the description about this.

[haraken, 2015-07-10 07:48:37]

On 2015/07/10 07:46:47, philipj wrote:
>
https://codereview.chromium.org/1233483002/diff/1/Source/core/frame/UseCounter.h
> File Source/core/frame/UseCounter.h (right):
> 
>
https://codereview.chromium.org/1233483002/diff/1/Source/core/frame/UseCounte...
> Source/core/frame/UseCounter.h:563: AddEventListenerOneArgument = 657,
> On 2015/07/09 23:37:58, haraken wrote:
> > 
> > I'm wondering why the entry 657 and 659 were missing. Isn't there any risk
> that
> > we reuse the entry and see mixed results in the UMA dashboard?
> 
> These were the original entries for 657 and 659, but it was accidentally dead
> code and removed in https://codereview.chromium.org/928103002
> 
> I've tried to be clarify in the description about this.

Makes sense; LGTM.

[philipj_slow, 2015-07-10 07:55:28]

Count in CallPrologue instead

[philipj_slow, 2015-07-10 07:55:41]

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

[philipj_slow, 2015-07-10 07:55:42]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1233483002/#ps20001
(title: "Count in CallPrologue instead")

[commit-bot: I haz the power, 2015-07-10 07:55:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1233483002/20001

[commit-bot: I haz the power, 2015-07-10 09:18:19]

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

[commit-bot: I haz the power, 2015-07-10 09:18:20]

Dry run: This issue passed the CQ dry run.

[philipj_slow, 2015-07-10 09:26:09]

haraken-san, can you take another look? Your previous LGTM was before the
CallPrologue rewrite.

[haraken, 2015-07-10 09:27:20]

On 2015/07/10 09:26:09, philipj wrote:
> haraken-san, can you take another look? Your previous LGTM was before the
> CallPrologue rewrite.

Sorry, I was intending to write the LG on the other CL (that adds the
UseCounter)... Let me take another look :)

[haraken, 2015-07-10 09:32:21]

LGTM

https://codereview.chromium.org/1233483002/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8EventTargetCustom.cpp (right):

https://codereview.chromium.org/1233483002/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8EventTargetCustom.cpp:43: info.Length() == 0 ?
UseCounter::AddEventListenerNoArguments :
UseCounter::AddEventListenerOneArgument);

You decided not to throw error and just want to take user counters, right?

https://codereview.chromium.org/1233483002/diff/20001/Source/bindings/templat...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/1233483002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface.h:86: {% filter
conditional(method.conditional_string) %}

conditional is deprecated (I think it's used only in webaudio). So let's remove
this.

[philipj_slow, 2015-07-10 09:41:27]

https://codereview.chromium.org/1233483002/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8EventTargetCustom.cpp (right):

https://codereview.chromium.org/1233483002/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8EventTargetCustom.cpp:43: info.Length() == 0 ?
UseCounter::AddEventListenerNoArguments :
UseCounter::AddEventListenerOneArgument);
On 2015/07/10 09:32:20, haraken wrote:
> 
> You decided not to throw error and just want to take user counters, right?

Yes, the purpose of this is to actually collect the use counter data to prove
that making these arguments required will not break. Normally this is way
overkill, but the first attempt to fix it failed, so...

https://codereview.chromium.org/1233483002/diff/20001/Source/bindings/templat...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/1233483002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface.h:86: {% filter
conditional(method.conditional_string) %}
On 2015/07/10 09:32:20, haraken wrote:
> 
> conditional is deprecated (I think it's used only in webaudio). So let's
remove
> this.

Should I remove it for the call epilogue as well?

[haraken, 2015-07-10 09:50:52]

https://codereview.chromium.org/1233483002/diff/20001/Source/bindings/templat...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/1233483002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface.h:86: {% filter
conditional(method.conditional_string) %}
On 2015/07/10 09:41:27, philipj wrote:
> On 2015/07/10 09:32:20, haraken wrote:
> > 
> > conditional is deprecated (I think it's used only in webaudio). So let's
> remove
> > this.
> 
> Should I remove it for the call epilogue as well?

Sounds nicer :)

[philipj_slow, 2015-07-10 11:10:02]

No [Conditional] support

[philipj_slow, 2015-07-10 11:10:57]

https://codereview.chromium.org/1233483002/diff/20001/Source/bindings/templat...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/1233483002/diff/20001/Source/bindings/templat...
Source/bindings/templates/interface.h:86: {% filter
conditional(method.conditional_string) %}
On 2015/07/10 09:50:52, haraken wrote:
> On 2015/07/10 09:41:27, philipj wrote:
> > On 2015/07/10 09:32:20, haraken wrote:
> > > 
> > > conditional is deprecated (I think it's used only in webaudio). So let's
> > remove
> > > this.
> > 
> > Should I remove it for the call epilogue as well?
> 
> Sounds nicer :)

Done.

[philipj_slow, 2015-07-10 11:11:02]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-07-10 11:11:02]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1233483002/#ps40001
(title: "No [Conditional] support")

[commit-bot: I haz the power, 2015-07-10 11:11:13]

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

[commit-bot: I haz the power, 2015-07-10 12:05:01]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198681