Change EventTarget callback APIs for add/RemoveEventListenerInternal.

Change EventTarget callback APIs for add/RemoveEventListenerInternal.

It will be necessary to know what RegisteredEventListener was added or
removed with the AddEventListenerOptions IDL definition coming.

Change the way the API callbacks occur to pass the registeredEventListener
in.

No functional change is intended with this change.

BUG=602735
Committed: https://crrev.com/908fce48e7365aebede69d3275ad88817b8cf8b4
Cr-Commit-Position: refs/heads/master@{#392055}

[dtapuska, 2016-05-02 20:03:06]

Description was changed from

==========
Change EventTarget callback APIs for add/RemoveEventListenerInternal.

It will be necessary to know what RegisteredEventListener was added or
removed with the AddEventListenerOptions IDL definition coming.

Change the way the API callbacks occur to pass the registeredEventListener
in.

No functional change is intended with this change.

BUG=602735
==========

to

==========
Change EventTarget callback APIs for add/RemoveEventListenerInternal.

It will be necessary to know what RegisteredEventListener was added or
removed with the AddEventListenerOptions IDL definition coming.

Change the way the API callbacks occur to pass the registeredEventListener
in.

No functional change is intended with this change.

BUG=602735
==========

[dtapuska, 2016-05-02 20:03:07]

dtapuska@chromium.org changed reviewers:
+ bokan@chromium.org

[dtapuska, 2016-05-02 20:16:30]

I had to keep add/RemoveEventListenerInternal for two cases
1) SVGElement.cpp and 2) ServiceWorkerGlobalScope.

1) Adds listeners to shadow DOM on these calls
2) Logs to the console; it looks like it should do this regardless of the
success of adding a listener.

[bokan, 2016-05-03 15:52:47]

>I had to keep add/RemoveEventListenerInternal for two cases
1) SVGElement.cpp and 2) ServiceWorkerGlobalScope.

Not sure I understand why 1 can't be changed but agree with 2. Could we just
make ServiceWorkerGlobalScope (and SVGElement if I'm wrong) override
addEventListener directly and merge add/removeEventListener and
add/removeEventListenerInternal? It seems having these `internal` overridables
adds nothing but complexity.

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventListenerMap.cpp (right):

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventListenerMap.cpp:133: static bool
removeListenerFromVector(EventListenerVector* listenerVector, const
EventListener* listener, const EventListenerOptions& options, size_t&
indexOfRemovedListener, RegisteredEventListener* registeredListener)
Pass indexOfRemovedListener by pointer since it's an out param
(https://google.github.io/styleguide/cppguide.html#Reference_Arguments)

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventListenerMap.cpp:136: for (const auto&
eventListener : *listenerVector) {
Why did you have to transform vector->find into a hand rolled loop?

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventTarget.h:156: virtual void
addedEventListener(const AtomicString& eventType, const
RegisteredEventListener&);
Please add comments describing what these are for.

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElement.cpp:737: if
(!Node::addEventListenerInternal(eventType, listener, options))
I'm probably missing something, but why can't these be turned into
added/removedEventListener? Looks like it returns if it fails to add the
listener, no?

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:100:
WebBluetooth* webbluetooth =
BluetoothSupplement::fromExecutionContext(getExecutionContext());
This used to get called even if the listener failed to be added. Seems to me
like that might have been an oversight but would you mind adding someone who
worked on this to double check?

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webmidi/MIDIInput.cpp (right):

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webmidi/MIDIInput.cpp:73: open();
Ditto here, this used to be called on success or failure. Should have a local
expert look this over.

[dtapuska, 2016-05-03 16:23:24]

There are 3 base (non-virtual) implementations of add/removeEventListener each.

The values on the objects are not initialized to their default values until
add/removeEventListenerInternal is called. So the internal APIs have fully
initialized objects. If we allow an override of the API; then the values in the
objects may not correctly be populated if a default is provided dynamically.
(which it is today based on a runtime flag)

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventListenerMap.cpp (right):

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventListenerMap.cpp:136: for (const auto&
eventListener : *listenerVector) {
On 2016/05/03 15:52:46, bokan wrote:
> Why did you have to transform vector->find into a hand rolled loop?

This was done to correct the const behaviour. EventListener* listener can be
const EventListener* on the remove except for it's usage here if it was to
create a RegisteredEventListener object.

[bokan, 2016-05-03 17:41:05]

On 2016/05/03 16:23:24, dtapuska wrote:
> There are 3 base (non-virtual) implementations of add/removeEventListener
each.
> 
> The values on the objects are not initialized to their default values until
> add/removeEventListenerInternal is called. So the internal APIs have fully
> initialized objects. If we allow an override of the API; then the values in
the
> objects may not correctly be populated if a default is provided dynamically.
> (which it is today based on a runtime flag)
> 

Ah, ok, I missed that (and that these were used from V8 bindings) so this is
fine as is.

[bokan, 2016-05-03 17:42:02]

Ok, lgtm, all my major points were addressed. The remainder are nits and
suggestions.

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventListenerMap.cpp (right):

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventListenerMap.cpp:136: for (const auto&
eventListener : *listenerVector) {
On 2016/05/03 15:52:46, bokan wrote:
> Why did you have to transform vector->find into a hand rolled loop?

Thanks, makes sense. IMO, this would be clearer with std::find_if but you can
judge (e.g.
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...)

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElement.cpp:737: if
(!Node::addEventListenerInternal(eventType, listener, options))
On 2016/05/03 15:52:46, bokan wrote:
> I'm probably missing something, but why can't these be turned into
> added/removedEventListener? Looks like it returns if it fails to add the
> listener, no?

Ok, chatted off-review, because addedEventListener passes
RegisteredEventListener by const ref.

I think making addedEventListener pass by non-const ref would be fine. I didn't
understand your concern about call depth.

[dtapuska, 2016-05-03 20:41:42]

dtapuska@chromium.org changed reviewers:
+ ortuno@chromium.org

[dtapuska, 2016-05-03 20:43:24]

dtapuska@chromium.org changed reviewers:
+ imcheng@chromium.org

[dtapuska, 2016-05-03 20:44:20]

dtapuska@chromium.org changed reviewers:
+ toyoshim@chromium.org

[dtapuska, 2016-05-03 20:53:04]

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventListenerMap.cpp (right):

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventListenerMap.cpp:133: static bool
removeListenerFromVector(EventListenerVector* listenerVector, const
EventListener* listener, const EventListenerOptions& options, size_t&
indexOfRemovedListener, RegisteredEventListener* registeredListener)
On 2016/05/03 15:52:46, bokan wrote:
> Pass indexOfRemovedListener by pointer since it's an out param
> (https://google.github.io/styleguide/cppguide.html#Reference_Arguments)

Done.

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventListenerMap.cpp:136: for (const auto&
eventListener : *listenerVector) {
On 2016/05/03 17:42:02, bokan wrote:
> On 2016/05/03 15:52:46, bokan wrote:
> > Why did you have to transform vector->find into a hand rolled loop?
> 
> Thanks, makes sense. IMO, this would be clearer with std::find_if but you can
> judge (e.g.
>
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...)

Done.

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventTarget.h:156: virtual void
addedEventListener(const AtomicString& eventType, const
RegisteredEventListener&);
On 2016/05/03 15:52:46, bokan wrote:
> Please add comments describing what these are for.

Done.

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:100:
WebBluetooth* webbluetooth =
BluetoothSupplement::fromExecutionContext(getExecutionContext());
On 2016/05/03 15:52:46, bokan wrote:
> This used to get called even if the listener failed to be added. Seems to me
> like that might have been an oversight but would you mind adding someone who
> worked on this to double check?

Added ortuno@

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webmidi/MIDIInput.cpp (right):

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webmidi/MIDIInput.cpp:73: open();
On 2016/05/03 15:52:47, bokan wrote:
> Ditto here, this used to be called on success or failure. Should have a local
> expert look this over.

Added toyoshim@

[ortuno, 2016-05-03 20:59:56]

bluetooth lgtm

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/1942723004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:100:
WebBluetooth* webbluetooth =
BluetoothSupplement::fromExecutionContext(getExecutionContext());
On 2016/05/03 at 20:53:04, dtapuska wrote:
> On 2016/05/03 15:52:46, bokan wrote:
> > This used to get called even if the listener failed to be added. Seems to me
> > like that might have been an oversight but would you mind adding someone who
> > worked on this to double check?
> 
> Added ortuno@

I wasn't aware addEventListenerInternal was called even if the registration
failed. So thanks for the fix!

https://codereview.chromium.org/1942723004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1942723004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventTarget.h:157: // Called when an event
listener has succesfully added.
nit: s/has succesfully added/has been successfully added

https://codereview.chromium.org/1942723004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventTarget.h:160: // Called when an event
listener is removed. The original registration parameters with this
nit: s/parameters with/parameters of/

[dtapuska, 2016-05-03 21:07:31]

https://codereview.chromium.org/1942723004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1942723004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventTarget.h:157: // Called when an event
listener has succesfully added.
On 2016/05/03 20:59:56, ortuno wrote:
> nit: s/has succesfully added/has been successfully added

oh boy clearly I didn't read what I typed.

https://codereview.chromium.org/1942723004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventTarget.h:160: // Called when an event
listener is removed. The original registration parameters with this
On 2016/05/03 20:59:56, ortuno wrote:
> nit: s/parameters with/parameters of/

ditto.

[haraken, 2016-05-04 03:50:44]

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

[haraken, 2016-05-04 03:50:45]

https://codereview.chromium.org/1942723004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventListenerMap.cpp (right):

https://codereview.chromium.org/1942723004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventListenerMap.cpp:140: });

Drive-by nit: I'd avoid introducing complex C++ though. listenerVector->find()
would just work?

[dtapuska, 2016-05-04 12:48:38]

On 2016/05/04 03:50:45, haraken wrote:
>
https://codereview.chromium.org/1942723004/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/events/EventListenerMap.cpp (right):
> 
>
https://codereview.chromium.org/1942723004/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/events/EventListenerMap.cpp:140: });
> 
> Drive-by nit: I'd avoid introducing complex C++ though. listenerVector->find()
> would just work?

No it wouldn't work. As const EventListener* is passed in and you can't create a
RegisteredEventListener on the stack for the find because of the const operator.

[bokan, 2016-05-04 12:50:19]

On 2016/05/04 12:48:38, dtapuska wrote:
> On 2016/05/04 03:50:45, haraken wrote:
> >
>
https://codereview.chromium.org/1942723004/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/events/EventListenerMap.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1942723004/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/events/EventListenerMap.cpp:140: });
> > 
> > Drive-by nit: I'd avoid introducing complex C++ though.
listenerVector->find()
> > would just work?
> 
> No it wouldn't work. As const EventListener* is passed in and you can't create
a
> RegisteredEventListener on the stack for the find because of the const
operator.

A comment to that effect might be useful here since it's hard to see why we're
doing this.

[dtapuska, 2016-05-04 19:15:27]

dtapuska@chromium.org changed reviewers:
+ yhirano@chromium.org

[dtapuska, 2016-05-04 19:15:51]

On 2016/05/04 12:50:19, bokan wrote:
> On 2016/05/04 12:48:38, dtapuska wrote:
> > On 2016/05/04 03:50:45, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1942723004/diff/80001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/events/EventListenerMap.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1942723004/diff/80001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/events/EventListenerMap.cpp:140: });
> > > 
> > > Drive-by nit: I'd avoid introducing complex C++ though.
> listenerVector->find()
> > > would just work?
> > 
> > No it wouldn't work. As const EventListener* is passed in and you can't
create
> a
> > RegisteredEventListener on the stack for the find because of the const
> operator.
> 
> A comment to that effect might be useful here since it's hard to see why we're
> doing this.

+yhirano as toyoshim@ seems OOO

[yhirano, 2016-05-06 03:38:13]

modules/webmidi LGTM

[dtapuska, 2016-05-06 03:46:38]

On 2016/05/06 03:38:13, yhirano wrote:
> modules/webmidi LGTM

haraken@ ping; are you ok with this now? (As per my comment it couldn't really
be done).

[haraken, 2016-05-06 03:47:21]

On 2016/05/06 03:46:38, dtapuska wrote:
> On 2016/05/06 03:38:13, yhirano wrote:
> > modules/webmidi LGTM
> 
> haraken@ ping; are you ok with this now? (As per my comment it couldn't really
> be done).

Adding a comment sounds nice. LGTM.

[dtapuska, 2016-05-06 04:08:49]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-05-06 04:08:50]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
ortuno@chromium.org, haraken@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1942723004/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-06 04:08:59]

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

[commit-bot: I haz the power, 2016-05-06 04:37:54]

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

[commit-bot: I haz the power, 2016-05-06 04:37:56]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dtapuska, 2016-05-06 13:04:53]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-05-06 13:04:54]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
ortuno@chromium.org, haraken@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1942723004/#ps140001
(title: "Fix win32 signed/unsigned issue")

[commit-bot: I haz the power, 2016-05-06 13:05:02]

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

[commit-bot: I haz the power, 2016-05-06 14:27:03]

Description was changed from

==========
Change EventTarget callback APIs for add/RemoveEventListenerInternal.

It will be necessary to know what RegisteredEventListener was added or
removed with the AddEventListenerOptions IDL definition coming.

Change the way the API callbacks occur to pass the registeredEventListener
in.

No functional change is intended with this change.

BUG=602735
==========

to

==========
Change EventTarget callback APIs for add/RemoveEventListenerInternal.

It will be necessary to know what RegisteredEventListener was added or
removed with the AddEventListenerOptions IDL definition coming.

Change the way the API callbacks occur to pass the registeredEventListener
in.

No functional change is intended with this change.

BUG=602735
==========

[commit-bot: I haz the power, 2016-05-06 14:27:05]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-06 14:28:20]

Description was changed from

==========
Change EventTarget callback APIs for add/RemoveEventListenerInternal.

It will be necessary to know what RegisteredEventListener was added or
removed with the AddEventListenerOptions IDL definition coming.

Change the way the API callbacks occur to pass the registeredEventListener
in.

No functional change is intended with this change.

BUG=602735
==========

to

==========
Change EventTarget callback APIs for add/RemoveEventListenerInternal.

It will be necessary to know what RegisteredEventListener was added or
removed with the AddEventListenerOptions IDL definition coming.

Change the way the API callbacks occur to pass the registeredEventListener
in.

No functional change is intended with this change.

BUG=602735

Committed: https://crrev.com/908fce48e7365aebede69d3275ad88817b8cf8b4
Cr-Commit-Position: refs/heads/master@{#392055}
==========

[commit-bot: I haz the power, 2016-05-06 14:28:21]

Patchset 8 (id:??) landed as
https://crrev.com/908fce48e7365aebede69d3275ad88817b8cf8b4
Cr-Commit-Position: refs/heads/master@{#392055}