IDL compiler: addEventListener, removeEventListener methods

IDL compiler: addEventListener, removeEventListener methods

Last methods CL!
(Still lots of cleanup, then on to interfaces.)

Also:
Renaming 'special' to 'exceptional' in comments, as 'special' has technical
meaning in Web IDL (special operations, special keywords, etc.).

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

[Nils Barth (inactive), 2013-11-13 06:15:59]

Last methods CL!

https://codereview.chromium.org/70963003/diff/1/Source/bindings/scripts/code_...
File Source/bindings/scripts/code_generator_v8.pm (left):

https://codereview.chromium.org/70963003/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.pm:2311: 
Blank lines causes problems in Python when we indent the code
(creates indented blank line), so remove.

[haraken, 2013-11-13 08:33:53]

LGTM but we might want to refactor HiddenDependency code in a follow-up.

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/scripts/u...
File Source/bindings/scripts/unstable/v8_types.py (right):

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_types.py:313: 'core/frame/DOMWindow.h']),

Please double-check that these headers are necessary.

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
Source/bindings/templates/methods.cpp:99: {% set lookup_type,
pass_ref_ptr_handling, hidden_dependency_action =

Actually I don't understand why we need these complexity.

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
Source/bindings/templates/methods.cpp:116:
V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithNullCheck>,
stringResource, info[0]);

stringResource => eventName ?

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
Source/bindings/templates/methods.cpp:119:
{{hidden_dependency_action}}HiddenDependency(info.Holder(), info[1],
{{v8_class_name}}::eventListenerCacheIndex, info.GetIsolate());

Probably we need to refactor *HiddenDependency code in V8Utilities.cpp.

- "Hidden dependency" sounds weird to me. As far as I read the code in
V8Utilities, hidden dependency is just an anonymous version of hidden values.
(i.e., in case of hidden values, we need to specify a property name when adding
a hidden value. In case of hidden dependency, we can add hidden values without
property names.)

- Can createHiddenDependency be called with an index other than
eventListenerCacheIndex? If no, we can rename things as follows:

-- eventListenerCacheIndex => anonymousHiddenValues
-- {create,remove,transfer}HiddenDependency =>
{set,unset,move}AnonymousHiddenValues

[Nils Barth (inactive), 2013-11-13 11:33:28]

Thanks!
* clarified the complex section, and
* will look into refactoring HiddenDependency code in a follow-up.

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/scripts/u...
File Source/bindings/scripts/unstable/v8_types.py (right):

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/scripts/u...
Source/bindings/scripts/unstable/v8_types.py:313: 'core/frame/DOMWindow.h']),
On 2013/11/13 08:33:53, haraken wrote:
> 
> Please double-check that these headers are necessary.

Yup, all needed (from template):
BindingSecurity::shouldAllowAccessToFrame(...)
ExceptionState es(info.GetIsolate())
V8EventListenerList::getEventListener
DOMWindow* window = impl->toDOMWindow()

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
Source/bindings/templates/methods.cpp:99: {% set lookup_type,
pass_ref_ptr_handling, hidden_dependency_action =
On 2013/11/13 08:33:53, haraken wrote:
> 
> Actually I don't understand why we need these complexity.

Clarified by:
* adding a comment, and
* making the template parameters correspond to the whole value
(function name etc.), rather than just the differing part.
Does this make it clearer?

The code for addEventListener and removeEventListener is almost the same,
differing only in 3 places.
Thus it's implemented as a single template.

The Perl code makes this very terse by making auxiliary variables
for just the portions that differ, so
ListenerFindOrCreate/ListenerFindOnly becomes:
my $lookupType = ($name eq "addEventListener") ? "OrCreate" : "Only";
ListenerFind$lookupType

Relevant Perl code:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
Source/bindings/templates/methods.cpp:116:
V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithNullCheck>,
stringResource, info[0]);
On 2013/11/13 08:33:53, haraken wrote:
> 
> stringResource => eventName ?

Done.

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
Source/bindings/templates/methods.cpp:119:
{{hidden_dependency_action}}HiddenDependency(info.Holder(), info[1],
{{v8_class_name}}::eventListenerCacheIndex, info.GetIsolate());
On 2013/11/13 08:33:53, haraken wrote:
> Probably we need to refactor *HiddenDependency code in V8Utilities.cpp.

Got it; added to TODO, and will look into.

Quick note:
*HiddenDependency code is only used for EventListener,
and it's clearly hacky, so I think we can sensibly make
this simpler and at least more explicitly named.

[commit-bot: I haz the power, 2013-11-13 11:34:39]

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

[haraken, 2013-11-13 11:37:46]

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
Source/bindings/templates/methods.cpp:99: {% set lookup_type,
pass_ref_ptr_handling, hidden_dependency_action =
On 2013/11/13 11:33:29, Nils Barth wrote:
> On 2013/11/13 08:33:53, haraken wrote:
> > 
> > Actually I don't understand why we need these complexity.
> 
> Clarified by:
> * adding a comment, and
> * making the template parameters correspond to the whole value
> (function name etc.), rather than just the differing part.
> Does this make it clearer?
> 
> The code for addEventListener and removeEventListener is almost the same,
> differing only in 3 places.
> Thus it's implemented as a single template.
> 
> The Perl code makes this very terse by making auxiliary variables
> for just the portions that differ, so
> ListenerFindOrCreate/ListenerFindOnly becomes:
> my $lookupType = ($name eq "addEventListener") ? "OrCreate" : "Only";
> ListenerFind$lookupType
> 
> Relevant Perl code:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Can't we somehow get rid of the distinction between ('OrCreate', '', 'create')
and ('Only', '.get()', 'remove') ? Refactoring in the event listener code would
be needed.

[commit-bot: I haz the power, 2013-11-13 12:30:59]

Change committed as 161913

[Nils Barth (inactive), 2013-11-14 03:01:52]

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
Source/bindings/templates/methods.cpp:99: {% set lookup_type,
pass_ref_ptr_handling, hidden_dependency_action =
On 2013/11/13 11:37:46, haraken wrote:
> Can't we somehow get rid of the distinction between ('OrCreate', '', 'create')
> and ('Only', '.get()', 'remove') ?
> Refactoring in the event listener code would be needed.

We probably need to generate *some* different code for addEventListener
vs. removeEventListener, but it should just be a function call in v8/,
not this long generated code.
We can definitely simplify this code by moving it to v8/ or other C++
changes, which would also reduce the number of includes.

[haraken, 2013-11-14 04:33:02]

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
Source/bindings/templates/methods.cpp:99: {% set lookup_type,
pass_ref_ptr_handling, hidden_dependency_action =
On 2013/11/14 03:01:53, Nils Barth wrote:
> On 2013/11/13 11:37:46, haraken wrote:
> > Can't we somehow get rid of the distinction between ('OrCreate', '',
'create')
> > and ('Only', '.get()', 'remove') ?
> > Refactoring in the event listener code would be needed.
> 
> We probably need to generate *some* different code for addEventListener
> vs. removeEventListener, but it should just be a function call in v8/,
> not this long generated code.
> We can definitely simplify this code by moving it to v8/ or other C++
> changes, which would also reduce the number of includes.

Yeah, I haven't yet checked the code of event listeners, I think that we should
clean up the code generator with C++ side changes.

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
Source/bindings/templates/methods.cpp:119:
{{hidden_dependency_action}}HiddenDependency(info.Holder(), info[1],
{{v8_class_name}}::eventListenerCacheIndex, info.GetIsolate());
On 2013/11/13 11:33:29, Nils Barth wrote:
> On 2013/11/13 08:33:53, haraken wrote:
> > Probably we need to refactor *HiddenDependency code in V8Utilities.cpp.
> 
> Got it; added to TODO, and will look into.
> 
> Quick note:
> *HiddenDependency code is only used for EventListener,
> and it's clearly hacky, so I think we can sensibly make
> this simpler and at least more explicitly named.

Regarding hidden dependency, kouhei-san might have thoughts about how to
refactor. So you can ask him offline.

[Nils Barth (inactive), 2013-11-14 05:47:20]

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/70963003/diff/30001/Source/bindings/templates...
Source/bindings/templates/methods.cpp:119:
{{hidden_dependency_action}}HiddenDependency(info.Holder(), info[1],
{{v8_class_name}}::eventListenerCacheIndex, info.GetIsolate());
On 2013/11/14 04:33:02, haraken wrote:
> Regarding hidden dependency, kouhei-san might have thoughts about how to
> refactor. So you can ask him offline.

Thanks, will do!