[DeprecateAs] if call addEventListener or removeEventListener without enough arguments

[DeprecateAs] if call addEventListener or removeEventListener without enough arguments

No changes to behavior, other than adding a deprecation message.

Also, must run:
update_use_counter_feature_enum.py
in:
chromium/src/tools/metrics/histograms/
to update the UMA mapping.

This adds [DeprecateAs] to deprecated behavior, per suggestion of Tab:
https://codereview.chromium.org/329053002/#msg5

Also updates IDL (EventListener args are nullable).

For background, see:
Throw TypeError when addEventListener or removeEventListener are called without enough arguments
https://codereview.chromium.org/329053002/

Current behavior (not throwing a TypeError) was for compatibility in 2012.
WebKit bug specifically special-casing this in JavaScriptCore bindings is:
[JSC] Regression: addEventListener() and removeEventListener() raise an exception on missing args
https://bugs.webkit.org/show_bug.cgi?id=85928

WebKit/Safari also has legacy behavior (matches current Blink/Chrome):
"For compatibility with legacy content, the EventListener calls are generated without GenerateArgumentsCountCheck."
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm#L2823

However, IE and Firefox *do* throw (correctly), and this discrepancy is causing
compatibility problems:
removeEventListener with only one passed parameter...
http://lists.w3.org/Archives/Public/public-script-coord/2014AprJun/0166.html

Thus we'd like to align with other browsers and spec, but not break legacy
content, hence a [DeprecateAs] to measure first.

R=haraken
BUG=353484
TEST=fast/dom/event-target-arguments.html
TEST=fast/dom/node-legacy-event-listener.html
TEST=fast/dom/Window/window-legacy-event-listener.html
TEST=fast/dom/XMLHttpRequest-legacy-event-listener.html

[Nils Barth (inactive), 2014-06-12 02:08:36]

[DeprecateAs] instead of removing, to avoid breaking content.

[haraken, 2014-06-12 02:15:45]

https://codereview.chromium.org/329223004/diff/20001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/329223004/diff/20001/Source/bindings/template...
Source/bindings/templates/methods.cpp:24: {% if is_check_security_for_window %}

It doesn't make sense to introduce [CheckSecurity=Window]. The EventTarget is
checking security against Frame, not Window. Look at line 26 calling
shouldAllowAccessTo*Frame*().

The only difference between EventTarget and other interfaces that have
[CheckSecurity=Frame] is the way to retrieve a frame. EventTarget retrieves a
frame via impl->toDOMWindow()->frame(), whereas other interfaces retrieves a
frame via impl->frame().

Shall we add EventTarget::frame() so that we can merge the two code?

[Nils Barth (inactive), 2014-06-12 02:51:52]

Revised to remove the cleanup, which is distracting;
instead opened bug and added FIXMEs.

https://codereview.chromium.org/329223004/diff/20001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/329223004/diff/20001/Source/bindings/template...
Source/bindings/templates/methods.cpp:24: {% if is_check_security_for_window %}
On 2014/06/12 02:15:45, haraken wrote:
> It doesn't make sense to introduce [CheckSecurity=Window]. The EventTarget is
> checking security against Frame, not Window. Look at line 26 calling
> shouldAllowAccessTo*Frame*().

It's slightly trickier:
EventTarget also has a !window->document() check.

[CheckSecurity=Frame] also affects other code, like
security_check_functions
(indexedSecurityCheck and namedSecurityCheck)
and instanceTemplate->SetAccessCheckCallbacks
...which also need to be handled differently.

Cleaning this up is a bit involved, so I've removed this part of the CL and
opened a separate bug:
Eliminate special handling of EventTarget and EventListener
https://code.google.com/p/chromium/issues/detail?id=383699

[haraken, 2014-06-12 04:23:23]

https://codereview.chromium.org/329223004/diff/100001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/329223004/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:94: if argument.idl_type.name ==
'EventListenerOrNull')

Shouldn't this be:

  if argument.idl_type.name == 'EventListener' or argument.idl_type.name ==
'EventListenerOrNull'

?

(BTW, I was assuming that we have idl_type.name and idl_type.nullable
separately. idl_type.name stores the type name (without "OrNull") and
idl_type.nullable stores whether the type is nullable or not.)

https://codereview.chromium.org/329223004/diff/100001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/329223004/diff/100001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.h:455: EventTargetMissingArgs = 454,

This should be 455.

[Nils Barth (inactive), 2014-06-12 04:55:40]

https://codereview.chromium.org/329223004/diff/100001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/329223004/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:94: if argument.idl_type.name ==
'EventListenerOrNull')
On 2014/06/12 04:23:23, haraken wrote:
> Shouldn't this be:
> 
>   if argument.idl_type.name == 'EventListener' or argument.idl_type.name ==
> 'EventListenerOrNull'
> 
> ?

The only use of EventListener is as a nullable type, so
argument.idl_type.name == 'EventListener'
is dead code.

Python note:
For multiple matches, use |in|:
if argument.idl_type.name in ('EventListener', 'EventListenerOrNull'):


> (BTW, I was assuming that we have idl_type.name and idl_type.nullable
> separately. idl_type.name stores the type name (without "OrNull") and
> idl_type.nullable stores whether the type is nullable or not.)

No, the type name includes nullability:
http://heycam.github.io/webidl/#idl-nullable-type
"""
The type name of a nullable type is the concatenation of the type name of the
inner type T and the string “OrNull”.
"""

We have IdlType.base_type to hold the base type of nullable/array/etc. types;
(in the spec, for nullable this is "inner type", for array this is "element
type", but we're not distinguishing these currently)
I'm using IdlType.name here to be tighter, as this is an ugly special case.

https://codereview.chromium.org/329223004/diff/100001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/329223004/diff/100001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.h:455: EventTargetMissingArgs = 454,
On 2014/06/12 04:23:23, haraken wrote:
> This should be 455.

(>.<)
Thanks, fixed!
(use counters need frequent updating when rebasing)

[haraken, 2014-06-12 06:00:17]

Thanks for the clarification. LGTM.

[haraken, 2014-06-12 06:00:41]

On 2014/06/12 06:00:17, haraken wrote:
> Thanks for the clarification. LGTM.

I want to have Tab double-confirm :)

[Nils Barth (inactive), 2014-06-12 06:01:32]

On 2014/06/12 06:00:41, haraken wrote:
> On 2014/06/12 06:00:17, haraken wrote:
> > Thanks for the clarification. LGTM.
> 
> I want to have Tab double-confirm :)

Thanks!
(Will wait on Tab.)

[Tab Atkins, 2014-06-12 18:30:29]

On 2014/06/12 06:01:32, Nils Barth wrote:
> On 2014/06/12 06:00:41, haraken wrote:
> > On 2014/06/12 06:00:17, haraken wrote:
> > > Thanks for the clarification. LGTM.
> > 
> > I want to have Tab double-confirm :)
> 
> Thanks!
> (Will wait on Tab.)

LGTM, though I'm also fine with just removing them. ^_^

[Nils Barth (inactive), 2014-06-23 10:09:46]

On 2014/06/12 18:30:29, Tab Atkins wrote:
> LGTM, though I'm also fine with just removing them. ^_^

Oops, I misunderstood.
Got it, will just remove (that's makes life easier).

[Nils Barth (inactive), 2014-06-24 04:50:54]

On 2014/06/23 10:09:46, Nils Barth wrote:
> On 2014/06/12 18:30:29, Tab Atkins wrote:
> > LGTM, though I'm also fine with just removing them. ^_^
> 
> Oops, I misunderstood.
> Got it, will just remove (that's makes life easier).

Oops, removing requires API owners review/approval
(with, sadly, does not include Tab saying "ok"),
and we probably want to deprecate first anyway
to measure impact, so doing this CL instead.

[tkent, 2014-06-24 05:12:22]

lgtm

[Nils Barth (inactive), 2014-06-25 04:11:57]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-06-25 04:13:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nbarth@chromium.org/329223004/150001

[commit-bot: I haz the power, 2014-06-25 05:32:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/12680)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/13469)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/21234)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/25325)

[commit-bot: I haz the power, 2014-06-25 05:40:36]

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

[commit-bot: I haz the power, 2014-06-25 05:40:37]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[Nils Barth (inactive), 2014-06-25 07:54:01]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-06-25 07:54:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nbarth@chromium.org/329223004/170001

[commit-bot: I haz the power, 2014-06-25 09:09:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/12702)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/13492)

[commit-bot: I haz the power, 2014-06-25 09:38:21]

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

[commit-bot: I haz the power, 2014-06-25 09:38:22]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)