Support "once" option for addEventListener

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM: Events
P3
normal
RESOLVED FIXED
11 months ago
13 days ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

({dev-doc-complete})

unspecified
mozilla50
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments, 1 obsolete attachment)

58 bytes, text/x-review-board-request
smaug
: review+
Details | Review
58 bytes, text/x-review-board-request
smaug
: review+
Details | Review
58 bytes, text/x-review-board-request
smaug
: review+
Details | Review
58 bytes, text/x-review-board-request
smaug
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
smaug
: review+
Details | Review
58 bytes, text/x-review-board-request
smaug
: review+
Details | Review
58 bytes, text/x-review-board-request
smaug
: review+
Details | Review
(Assignee)

Description

11 months ago
The DOM Standard added an "once" option for addEventListener, which would be quite useful, at least for test writing.

This option already exists in the WebIDL definition of AddEventListenerOptions, but the code to remove listener after its first use is not implemented.
> This option already exists in the WebIDL definition of AddEventListenerOptions, but the code to remove listener after its first use is not implemented.

Does this mean people might feature detect that we support it when we actually don't?

Comment 2

11 months ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=1266194#c5
which was just fixed in 
https://bugzilla.mozilla.org/show_bug.cgi?id=1287760
Priority: -- → P3
(Assignee)

Comment 3

11 months ago
Created attachment 8772713 [details] [diff] [review]
wip patch

Not sure whether this is the right way to go... If it is, I'll write a web-platform test and then request review.
Attachment #8772713 - Flags: feedback?(bugs)

Comment 4

11 months ago
Comment on attachment 8772713 [details] [diff] [review]
wip patch

># HG changeset patch
># User Xidorn Quan <me@upsuper.org>
># Date 1468998570 -36000
>#      Wed Jul 20 17:09:30 2016 +1000
># Node ID bc3d031a335a2bbdb494e66895f45130f65ec074
># Parent  97eb0e11fceb0ebe7513a2926781835dd2628bb5
>Bug 1287706 WIP - Support AddEventListenerOption.once in event code.
>
>MozReview-Commit-ID: 3ulIRxdApr6
>
>diff --git a/dom/events/EventListenerManager.cpp b/dom/events/EventListenerManager.cpp
>--- a/dom/events/EventListenerManager.cpp
>+++ b/dom/events/EventListenerManager.cpp
>@@ -1279,26 +1279,37 @@ EventListenerManager::HandleEventInterna
>                   (*aDOMEvent)->GetEventPhase(&phase);
>                   timelines->AddMarkerForDocShell(docShell, Move(
>                     MakeUnique<EventTimelineMarker>(
>                       typeStr, phase, MarkerTracingType::START)));
>                 }
>               }
>             }
> 
>+            Maybe<ListenerBase> listenerToRemove;
I don't understand why ListenerBase and not Listener

>+            if (listener->mFlags.mOnce) {
>+              listenerToRemove.emplace(*listener);
>+            }
>             aEvent->mFlags.mInPassiveListener = listener->mFlags.mPassive;
>             if (NS_FAILED(HandleEventSubType(listener, *aDOMEvent, aCurrentTarget))) {
>               aEvent->mFlags.mExceptionWasRaised = true;
>             }
>             aEvent->mFlags.mInPassiveListener = false;
> 
>             if (needsEndEventMarker) {
>               timelines->AddMarkerForDocShell(
>                 docShell, "DOMEvent", MarkerTracingType::END);
>             }
>+            if (listenerToRemove.isSome()) {
>+              RemoveEventListenerInternal(listenerToRemove->mListener,
>+                                          listenerToRemove->mEventMessage,
>+                                          listenerToRemove->mTypeAtom,
>+                                          listenerToRemove->mTypeString,
>+                                          listenerToRemove->mFlags, false);
>+            }
You need to remove the listener before the listener is called, otherwise nested event dispatch can use the listener again.
So, you need to keep the listener alive on stack, remove the listener from ELM and then call HandleEventSubType.
Note, the spec even says that listener should be removed before the listener is called.


>-  struct Listener
>+  struct ListenerBase
>   {
>     EventListenerHolder mListener;
>     nsCOMPtr<nsIAtom> mTypeAtom; // for the main thread
>     nsString mTypeString; // for non-main-threads
>     EventMessage mEventMessage;
>+    EventListenerFlags mFlags;
>+  };
> 
>+  struct Listener : public ListenerBase
Yeah, don't understand this ListenerBase stuff.


But yes, pretty much like this.
The test should definitely test nested dispatching.
Attachment #8772713 - Flags: feedback?(bugs) → feedback+
(Assignee)

Comment 5

11 months ago
I'm a bit worrying about the time complexity here... It is basically a O(N) deletion, so if someone attaches 1000 event listeners with once, that event dispatch would last for seconds... But I guess it isn't something we really need to worry about for the initial implementation?
(Assignee)

Comment 6

11 months ago
(In reply to Olli Pettay [:smaug] from comment #4)
> >
> >diff --git a/dom/events/EventListenerManager.cpp b/dom/events/EventListenerManager.cpp
> >--- a/dom/events/EventListenerManager.cpp
> >+++ b/dom/events/EventListenerManager.cpp
> >@@ -1279,26 +1279,37 @@ EventListenerManager::HandleEventInterna
> >                   (*aDOMEvent)->GetEventPhase(&phase);
> >                   timelines->AddMarkerForDocShell(docShell, Move(
> >                     MakeUnique<EventTimelineMarker>(
> >                       typeStr, phase, MarkerTracingType::START)));
> >                 }
> >               }
> >             }
> > 
> >+            Maybe<ListenerBase> listenerToRemove;
> I don't understand why ListenerBase and not Listener

Because Listener has a non-trivial destructor which I don't want to call by mistake...

Comment 7

11 months ago
(In reply to Xidorn Quan [:xidorn] (away until July 25) from comment #5)
> I'm a bit worrying about the time complexity here... It is basically a O(N)
> deletion, so if someone attaches 1000 event listeners with once, that event
> dispatch would last for seconds... 
Well, removal time doesn't affect to that O(N)

> But I guess it isn't something we really
> need to worry about for the initial implementation?
Another option is to mark listener 'removed' and after iterating listeners for callback, iterate them again (if some was removed) and remove the marked ones. 'removed' flag should be check in places like AddEventListener and HasListenerFor etc. Becomes a tad complicated.

Though, I guess simpler would be to take a copy of the once-listener,  clear all the member variables of the listener in the array, then call the callback.
Then after iterating all the listener for the event, if there were once-listeners, compact the array by taking away the cleared listeners. That might not be to hard.
I guess compacting could be to create a new array and copy/move the real Listener entries from the old one to there and then swap the array with the old one.
(Assignee)

Comment 8

11 months ago
(In reply to Olli Pettay [:smaug] from comment #7)
> (In reply to Xidorn Quan [:xidorn] (away until July 25) from comment #5)
> > But I guess it isn't something we really
> > need to worry about for the initial implementation?
> Another option is to mark listener 'removed' and after iterating listeners
> for callback, iterate them again (if some was removed) and remove the marked
> ones. 'removed' flag should be check in places like AddEventListener and
> HasListenerFor etc. Becomes a tad complicated.

Maintaining the array is simple. What I'm concerned is all the callbacks invoked in RemoveEventListenerInternal.

Comment 9

11 months ago
Things like NotifyAboutMainThreadListenerChange (which is mainly for devtools)? Well, that needs to be fired for all the listeners.
Keywords: dev-doc-needed
(Assignee)

Comment 10

11 months ago
Created attachment 8774574 [details]
Bug 1287706 part 1 - Make CallbackObjectHolder movable (and actually move-only).

So that we can avoid unnecessary refcount changes.

Review commit: https://reviewboard.mozilla.org/r/67000/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67000/
Attachment #8774574 - Flags: review?(bugs)
Attachment #8774575 - Flags: review?(bugs)
Attachment #8774576 - Flags: review?(bugs)
Attachment #8774577 - Flags: review?(bugs)
Attachment #8774578 - Flags: review?(nfroyd)
Attachment #8774579 - Flags: review?(bugs)
Attachment #8774580 - Flags: review?(bugs)
Attachment #8774581 - Flags: review?(bugs)
(Assignee)

Comment 11

11 months ago
Created attachment 8774575 [details]
Bug 1287706 part 2 - Remove useless runtime check of EventListenerManager::Listener.

Review commit: https://reviewboard.mozilla.org/r/67002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67002/
(Assignee)

Comment 12

11 months ago
Created attachment 8774576 [details]
Bug 1287706 part 3 - Make EventListenerHandler::Listener movable.

Review commit: https://reviewboard.mozilla.org/r/67004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67004/
(Assignee)

Comment 13

11 months ago
Created attachment 8774577 [details]
Bug 1287706 part 4 - Unify B2G device-type events to be handled in the same way as others device-type events.

Review commit: https://reviewboard.mozilla.org/r/67006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67006/
(Assignee)

Comment 14

11 months ago
Created attachment 8774578 [details]
Bug 1287706 part 5 - Add RemoveElementsBy method to nsTArray and nsTObserverArray to allow filtering elements by predicate function.

Review commit: https://reviewboard.mozilla.org/r/67008/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67008/
(Assignee)

Comment 15

11 months ago
Created attachment 8774579 [details]
Bug 1287706 part 6 - Separate notifying listener removal to an independent method.

Review commit: https://reviewboard.mozilla.org/r/67010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67010/
(Assignee)

Comment 16

11 months ago
Created attachment 8774580 [details]
Bug 1287706 part 7 - Add support of AddEventListenerOptions.once.

Review commit: https://reviewboard.mozilla.org/r/67012/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67012/
(Assignee)

Comment 17

11 months ago
Created attachment 8774581 [details]
Bug 1287706 part 8 - Add web-platform-test for once option.

Review commit: https://reviewboard.mozilla.org/r/67014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67014/
(Assignee)

Updated

11 months ago
Assignee: nobody → xidorn+moz
(Assignee)

Comment 18

11 months ago
Comment on attachment 8774578 [details]
Bug 1287706 part 5 - Add RemoveElementsBy method to nsTArray and nsTObserverArray to allow filtering elements by predicate function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67008/diff/1-2/
(Assignee)

Comment 19

11 months ago
Comment on attachment 8774579 [details]
Bug 1287706 part 6 - Separate notifying listener removal to an independent method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67010/diff/1-2/
(Assignee)

Comment 20

11 months ago
Comment on attachment 8774580 [details]
Bug 1287706 part 7 - Add support of AddEventListenerOptions.once.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67012/diff/1-2/
(Assignee)

Comment 21

11 months ago
Comment on attachment 8774581 [details]
Bug 1287706 part 8 - Add web-platform-test for once option.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67014/diff/1-2/
(Assignee)

Updated

11 months ago
Attachment #8772713 - Attachment is obsolete: true
(Assignee)

Comment 22

11 months ago
https://reviewboard.mozilla.org/r/67000/#review63920

::: dom/bindings/CallbackObject.h:519
(Diff revision 1)
> -    // NS_IF_RELEASE because we might have been unlinked before
> +    // We might have been unlinked before
> +    if (mPtrBits) {
> -    nsISupports* ptr = GetISupports();
> +      nsISupports* ptr = GetISupports();
> -    NS_IF_RELEASE(ptr);
> +      NS_RELEASE(ptr);
> -    mPtrBits = 0;
> +      mPtrBits = 0;
> -  }
> +    }

This change needs to be removed... It actually breaks things...
(Assignee)

Comment 23

11 months ago
Comment on attachment 8774574 [details]
Bug 1287706 part 1 - Make CallbackObjectHolder movable (and actually move-only).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67000/diff/1-2/
(Assignee)

Comment 24

11 months ago
Comment on attachment 8774575 [details]
Bug 1287706 part 2 - Remove useless runtime check of EventListenerManager::Listener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67002/diff/1-2/
(Assignee)

Comment 25

11 months ago
Comment on attachment 8774576 [details]
Bug 1287706 part 3 - Make EventListenerHandler::Listener movable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67004/diff/1-2/
(Assignee)

Comment 26

11 months ago
Comment on attachment 8774577 [details]
Bug 1287706 part 4 - Unify B2G device-type events to be handled in the same way as others device-type events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67006/diff/1-2/
(Assignee)

Comment 27

11 months ago
Comment on attachment 8774578 [details]
Bug 1287706 part 5 - Add RemoveElementsBy method to nsTArray and nsTObserverArray to allow filtering elements by predicate function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67008/diff/2-3/
(Assignee)

Comment 28

11 months ago
Comment on attachment 8774579 [details]
Bug 1287706 part 6 - Separate notifying listener removal to an independent method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67010/diff/2-3/
(Assignee)

Comment 29

11 months ago
Comment on attachment 8774580 [details]
Bug 1287706 part 7 - Add support of AddEventListenerOptions.once.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67012/diff/2-3/
(Assignee)

Comment 30

11 months ago
Comment on attachment 8774581 [details]
Bug 1287706 part 8 - Add web-platform-test for once option.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67014/diff/2-3/

Comment 31

11 months ago
Comment on attachment 8774575 [details]
Bug 1287706 part 2 - Remove useless runtime check of EventListenerManager::Listener.

https://reviewboard.mozilla.org/r/67002/#review64034
Attachment #8774575 - Flags: review?(bugs) → review+

Comment 32

11 months ago
Comment on attachment 8774574 [details]
Bug 1287706 part 1 - Make CallbackObjectHolder movable (and actually move-only).

https://reviewboard.mozilla.org/r/67000/#review64036

Since I have no idea why we'd want this, r-
Attachment #8774574 - Flags: review?(bugs) → review-

Comment 33

11 months ago
https://reviewboard.mozilla.org/r/67000/#review64038

feel free to explain why this improves code maintenance and makes writing code less error prone and then ask review again.

Comment 34

11 months ago
I wish I understood all the move stuff. Why is that needed? Move in general doesn't make code easier to read.
And why passing stuff by value and not by ref? What guarantees we aren't creating extra objects when using.
And in some cases Move(foo) is passed to ctor, in some cases Foo(param).
What happens in case one forgets to use Move()? Don't we create extra objects.

Comment 35

11 months ago
Comment on attachment 8774576 [details]
Bug 1287706 part 3 - Make EventListenerHandler::Listener movable.

https://reviewboard.mozilla.org/r/67004/#review64044

::: dom/events/EventListenerManager.cpp:672
(Diff revision 2)
>  {
>    MOZ_ASSERT(aEventMessage == aEvent->mMessage ||
>               aEventMessage == GetLegacyEventMessage(aEvent->mMessage),
>               "aEvent and aEventMessage should agree, modulo legacyness");
>  
> +  // The listener has been removed, it cannot handle anything.

You need similar check to AddEventListener so that adding the listener again works.
Attachment #8774576 - Flags: review?(bugs) → review-

Updated

11 months ago
Attachment #8774581 - Flags: review?(bugs) → review+

Comment 36

11 months ago
Comment on attachment 8774581 [details]
Bug 1287706 part 8 - Add web-platform-test for once option.

https://reviewboard.mozilla.org/r/67014/#review64140

Updated

11 months ago
Attachment #8774580 - Flags: review?(bugs) → review-

Comment 37

11 months ago
Comment on attachment 8774580 [details]
Bug 1287706 part 7 - Add support of AddEventListenerOptions.once.

https://reviewboard.mozilla.org/r/67012/#review64144

r- because I don't think this stuff should use Move, given that nothing is really moved, mostly just cloned and then the original Listener's state is changed.

::: dom/events/EventListenerManager.cpp:1264
(Diff revision 3)
> +            Maybe<Listener> listenerHolder;
> +            if (listener->mFlags.mOnce) {
> +              // Move the listener to the stack before handling the event.
> +              // The order is important, otherwise the listener could be
> +              // called again inside the listener.
> +              listenerHolder.emplace(Move(*listener));

I don't understand the logic here.
listener is something which is stored in the array. How can you "Move" it? Move usage makes this hard to understand IMO, since nothing is really moved here.

Comment 38

11 months ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #14)
> Created attachment 8774578 [details]
> Bug 1287706 part 5 - Add RemoveElementsBy method to nsTArray and
> nsTObserverArray to allow filtering elements by predicate function.
FWIW, I like this approach.

Comment 39

11 months ago
Comment on attachment 8774579 [details]
Bug 1287706 part 6 - Separate notifying listener removal to an independent method.

https://reviewboard.mozilla.org/r/67010/#review64156

::: dom/events/EventListenerManager.cpp:609
(Diff revision 3)
>        break;
>    }
>  }
>  
>  void
> +EventListenerManager::NotifyEventListenerRemoved(nsIAtom* aUserType)

Please add a comment that there are also other cases when listener can be removed and notified.

I wonder if EventListenerManager::RemoveEventHandler could use this new method.
Attachment #8774579 - Flags: review?(bugs) → review+

Comment 40

11 months ago
Comment on attachment 8774577 [details]
Bug 1287706 part 4 - Unify B2G device-type events to be handled in the same way as others device-type events.

https://reviewboard.mozilla.org/r/67006/#review64178
Attachment #8774577 - Flags: review?(bugs) → review+
(Assignee)

Comment 41

11 months ago
(In reply to Olli Pettay [:smaug] from comment #34)
> I wish I understood all the move stuff. Why is that needed? Move in general
> doesn't make code easier to read.

Move is a concept that moving the ownership of resource from one instance to another, and reset the original instance to its initial state or leave it in an (sounded) unusable state. For example, if our nsCOMPtr has a move constructor (which it doesn't yet), then
> nsCOMPtr somePtr(Move(anotherPtr));
would just be identical to
> nsCOMPtr somePtr(anotherPtr.forget());

And if nsTArray has a move construct (which it doesn't yet, either), then
> nsTArray<Foo> foos(Move(bars));
should be just set the foos.mHdr to bars.mHdr, and make bars.mHdr refer to the empty hdr.

> And why passing stuff by value and not by ref? What guarantees we aren't
> creating extra objects when using.

Passing CallbackObjectHolder by value because it is just a pointer-length type, which is not more expensive than a reference (which is a pointer as well), and passing by value potentially gives compilers more chance to optimize because compilers know that the original variable no longer owns the resource anyway. (If it is passed by reference, compilers cannot guarantee that if the callee is opaque in this translation unit.)

How could extra object be created? CallbackObjectHolder does not create object at all. In our current code, we use copy constructor, which means every time you pass it in, you have an extra AddRef / Release on the held object. However, with move, the old CallbackObjectHolder would just hand over its ownership to the new one, and no AddRef / Release happens.

Let's use a simplified RefPtr as an example (which is almost the same as CallbackObjectHolder), its copy constructor would be something like:
> RefPtr(const RefPtr& aOther) : mPtr(aOther.mPtr) { NS_IF_ADDREF(mPtr); }
there is an AddRef. And if a class needs that pointer, and we have
> Foo(const RefPtr& aPtr) : mPtr(aPtr) {}
and at the callsite
> { RefPtr ptr = something; Foo(ptr); }
we would call the destructor on ptr at the end of the scope, which calls NS_IF_RELEASE to the object it holds. However, this pair of AddRef / Release is not needed at all.

So in the move case, what would happen is, the move constructor would be something like
> RefPtr(RefPtr&& aOther) : mPtr(aOther.mPtr) { aOther.mPtr = nullptr; }
no AddRef here, aOther is just reset to nullptr. And the class constructor would be
> Foo(RefPtr aPtr) : mPtr(Move(aPtr)) {}
and at the callsite
> { RefPtr ptr = something; Foo(Move(ptr)); }
For the class constructor, the move constructor would be used to initialize aPtr from the callsite ptr, so no AddRef, and mPtr would use the move constructor as well, so no AddRef. All the initialization is just copying a pointer-length value from one place to another. And at the end of the callsite scope, ptr is reset to nullptr, so no Release there. And given that the compiler knows ptr is guaranteed to be reset to nullptr, it may just remove the destructor call completely.

In addition, as far as we make the object move-only (mark the copy constructor deleted), we would never AddRef / Release the held object when moving it between variables by accident.

> And in some cases Move(foo) is passed to ctor, in some cases Foo(param).
> What happens in case one forgets to use Move()? Don't we create extra
> objects.

If we forget Move() for Move(foo):
* if Foo is move-only, than the compiling would fail, because it cannot find a constructor for it;
* if Foo is copyable, then the copy constructor would be used.

A constructor accepts a rvalue reference (Foo&&) is called a move constructor, and it is expected to transfer the ownership from the parameter to itself (as showed above). A type which has move constructor but no copy constructor is a move-only type.

Roughly speaking, a lvalue reference (Foo&) is not implicitly-convertible to a rvalue reference (Foo&&), and thus would not be accepted by a move constructor. Expressions return a lvalue reference usually include forms of `foo`, `*foo`, and `foos[i]`. So if Foo is move-only, compiler would simply reject Foo(foo). In that case, Move() is used to convert the type from Foo& to Foo&& to explicitly state that we do want to move the ownership of the held resource from one variable to somewhere else.

And expression returns Foo would have type Foo&&, because the return value of an expression is just a temporary value, which is not bound to any variable, and thus can be moved freely. And that was the reason why Foo(param) can be used, because it is an expression creating a temporary value, which would be moved to the parameter.

FWIW, Foo&& can be implicitly converted to `const Foo&`, and thus if a type lacks move constructor, it would fallback to a copy constructor.


Is the explanation clear enough to you?
(Assignee)

Comment 42

11 months ago
https://reviewboard.mozilla.org/r/67004/#review64044

> You need similar check to AddEventListener so that adding the listener again works.

Actually it doesn't, because the move constructor of Listener resets all the fields (especially mListener), so the listener can surely be added again.
(Assignee)

Comment 43

11 months ago
https://reviewboard.mozilla.org/r/67012/#review64144

> I don't understand the logic here.
> listener is something which is stored in the array. How can you "Move" it? Move usage makes this hard to understand IMO, since nothing is really moved here.

If "move" all the resource it holds to the stack, and leave the original place an unusable placeholder (which ideally would be skipped everywhere).
(Assignee)

Comment 44

11 months ago
https://reviewboard.mozilla.org/r/67012/#review64144

> If "move" all the resource it holds to the stack, and leave the original place an unusable placeholder (which ideally would be skipped everywhere).

I mean, it "move"s all the resources.
(Assignee)

Updated

11 months ago
Attachment #8774574 - Flags: review- → review?(bugs)
(Assignee)

Updated

11 months ago
Attachment #8774576 - Flags: review- → review?(bugs)
(Assignee)

Updated

11 months ago
Attachment #8774580 - Flags: review- → review?(bugs)
(Assignee)

Comment 45

11 months ago
https://reviewboard.mozilla.org/r/67010/#review64156

> Please add a comment that there are also other cases when listener can be removed and notified.
> 
> I wonder if EventListenerManager::RemoveEventHandler could use this new method.

I don't really understand what do you want me to add. Is that about listing the callsites of this function? I don't think that's worth a comment.

And it seems yes, EventListenerManager::RemoveEventHandler can use this new method. I'll update the patch and use it there.
(Assignee)

Comment 46

11 months ago
Comment on attachment 8774574 [details]
Bug 1287706 part 1 - Make CallbackObjectHolder movable (and actually move-only).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67000/diff/2-3/
(Assignee)

Comment 47

11 months ago
Comment on attachment 8774575 [details]
Bug 1287706 part 2 - Remove useless runtime check of EventListenerManager::Listener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67002/diff/2-3/
(Assignee)

Comment 48

11 months ago
Comment on attachment 8774576 [details]
Bug 1287706 part 3 - Make EventListenerHandler::Listener movable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67004/diff/2-3/
(Assignee)

Comment 49

11 months ago
Comment on attachment 8774577 [details]
Bug 1287706 part 4 - Unify B2G device-type events to be handled in the same way as others device-type events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67006/diff/2-3/
(Assignee)

Comment 50

11 months ago
Comment on attachment 8774578 [details]
Bug 1287706 part 5 - Add RemoveElementsBy method to nsTArray and nsTObserverArray to allow filtering elements by predicate function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67008/diff/3-4/
(Assignee)

Comment 51

11 months ago
Comment on attachment 8774579 [details]
Bug 1287706 part 6 - Separate notifying listener removal to an independent method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67010/diff/3-4/
(Assignee)

Comment 52

11 months ago
Comment on attachment 8774580 [details]
Bug 1287706 part 7 - Add support of AddEventListenerOptions.once.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67012/diff/3-4/
(Assignee)

Comment 53

11 months ago
Comment on attachment 8774581 [details]
Bug 1287706 part 8 - Add web-platform-test for once option.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67014/diff/3-4/
(Assignee)

Updated

11 months ago
Attachment #8774579 - Flags: review+ → review?(bugs)

Comment 54

11 months ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #41)
> (In reply to Olli Pettay [:smaug] from comment #34)
> > I wish I understood all the move stuff. Why is that needed? Move in general
> > doesn't make code easier to read.
> 
> Move is a concept that moving the ownership of resource from one instance to
> another, and reset the original instance to its initial state or leave it in
> an (sounded) unusable state.
Sure, but the cases here aren't quite like that.


> > And why passing stuff by value and not by ref? What guarantees we aren't
> > creating extra objects when using.
> 
> Passing CallbackObjectHolder by value because it is just a pointer-length
> type, which is not more expensive than a reference (which is a pointer as
> well), and passing by value potentially gives compilers more chance to
> optimize because compilers know that the original variable no longer owns
> the resource anyway. (If it is passed by reference, compilers cannot
> guarantee that if the callee is opaque in this translation unit.)
What if more member variables are added to CallbackObjectHolder? 
Very unusual to pass by value.

> How could extra object be created? CallbackObjectHolder does not create
> object at all.
What you mean? If you create CallbackObjectHolder object, you sure create one :)
nsIDocument::CreateTreeWalker(*root, aWhatToShow, NodeFilterHolder(aFilter), rv)
creates a NodeFilterHolder object, and then one the callee side the object is created as well
(&& ctor is invoked). Sure compiler may or may not optimize the extra object out.


> > RefPtr(RefPtr&& aOther) : mPtr(aOther.mPtr) { aOther.mPtr = nullptr; }
> no AddRef here, aOther is just reset to nullptr. And the class constructor
> would be
> > Foo(RefPtr aPtr) : mPtr(Move(aPtr)) {}
This kind of ctor on Foo would be a bug IMO, since it let's one to pass lvalues, in which case extra addref/release could happen. 


> If we forget Move() for Move(foo):
> * if Foo is move-only, than the compiling would fail, because it cannot find
> a constructor for it;
> * if Foo is copyable, then the copy constructor would be used.
Yes. That is surprising and a bug, IMO.

> Is the explanation clear enough to you?
Not really. Doesn't explain why you want to change this all to use move semantics, especially in this bug.

Comment 55

11 months ago
Comment on attachment 8774576 [details]
Bug 1287706 part 3 - Make EventListenerHandler::Listener movable.

https://reviewboard.mozilla.org/r/67004/#review64146

If default and move ctors leave the object to the same state, r+

::: dom/events/EventListenerManager.h:225
(Diff revision 2)
> +      , mAllEvents(aOther.mAllEvents)
> +      , mIsChrome(aOther.mIsChrome)
> +    {
> +      aOther.mTypeString.Truncate();
> +      aOther.mEventMessage = eVoidEvent;
> +      aOther.mListenerType = eNoListener;



::: dom/events/EventListenerManager.h:225
(Diff revision 3)
> +      , mAllEvents(aOther.mAllEvents)
> +      , mIsChrome(aOther.mIsChrome)
> +    {
> +      aOther.mTypeString.Truncate();
> +      aOther.mEventMessage = eVoidEvent;
> +      aOther.mListenerType = eNoListener;

This is really surprising that only when Move ctor is used, the other object suddenly gets eNoListener status which is never ever otherwise would have.
This gives 'move' quite a new semantics:
"move and change state", instead of the normal 
"move and reset".

I could live with this setup if the default ctor would explicitly initialize values similarly to move ctor.
Attachment #8774576 - Flags: review?(bugs) → review+

Comment 56

11 months ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #45)
> https://reviewboard.mozilla.org/r/67010/#review64156
> 
> > Please add a comment that there are also other cases when listener can be removed and notified.
> > 
> > I wonder if EventListenerManager::RemoveEventHandler could use this new method.
> 
> I don't really understand what do you want me to add. Is that about listing
> the callsites of this function? I don't think that's worth a comment.
No. I mean that listener being removed is notified also elsewhere. The new method doesn't cover all the cases.

Comment 57

11 months ago
Comment on attachment 8774574 [details]
Bug 1287706 part 1 - Make CallbackObjectHolder movable (and actually move-only).

https://reviewboard.mozilla.org/r/67000/#review64508

ok, I guess I can live with this, but please add that comment to CallbackObjectHolder ctor.

::: dom/bindings/CallbackObject.h:427
(Diff revision 3)
>    explicit operator bool() const
>    {
>      return GetISupports();
>    }
>  
> +  CallbackObjectHolder Clone() const

gah, I hate C++'s move. It is so broken concept.
Ok, here we don't cause extra addref/release, since result ends up being assigned to another instance of CallbackObjectHolder using move ctor.

::: dom/events/EventListenerManager.cpp:303
(Diff revision 3)
>    } else if ((wjs = do_QueryInterface(aListenerHolder.GetXPCOMCallback()))) {
>      listener->mListenerType = Listener::eWrappedJSListener;
>    } else {
>      listener->mListenerType = Listener::eNativeListener;
>    }
> +  listener->mListener = Move(aListenerHolder);

Hmm, so first EventListener holder is moved to a temporary object which is then used to construct the argument for AddEventListener and that is then moved to create a temporary object which is used to create the argument for AddEventListenerByType which is then moved to create temporary object for AddEventListenerInternal to create the argument for it which is then moved (if the listener isn't already registered) to listener->mListener.
Compiler may of course optimize out many steps and in this case given that CallbackHolder is simple, it should be all fast. But this is a bit hazard to any possible changes to CallbackObjectHolder.
In case anything more complicated was passed, & or * param would probably be faster.
So, please add a comment to
CallbackObjectHolder(CallbackObjectHolder&& aOther) that it must not do anything complicated, or the setup should be changed to use pointer or reference passing.
Attachment #8774574 - Flags: review?(bugs) → review+

Comment 58

11 months ago
Comment on attachment 8774580 [details]
Bug 1287706 part 7 - Add support of AddEventListenerOptions.once.

https://reviewboard.mozilla.org/r/67012/#review64520

ok, move then. (Swap would be so much more readable.)
Attachment #8774580 - Flags: review?(bugs) → review+

Updated

11 months ago
Attachment #8774579 - Flags: review?(bugs) → review+

Comment 59

11 months ago
Comment on attachment 8774579 [details]
Bug 1287706 part 6 - Separate notifying listener removal to an independent method.

https://reviewboard.mozilla.org/r/67010/#review64522

::: dom/events/EventListenerManager.cpp:610
(Diff revision 4)
>    }
>  }
>  
>  void
> +EventListenerManager::NotifyEventListenerRemoved(nsIAtom* aUserType)
> +{

So somewhere here mention that if this code is changed, other callers of EventListenerRemoved and NotifyAboutMainThreadListenerChange should be changed too.
It is just that the name NotifyEventListenerRemoved hints that this is the way to notify about changes, but there are also other cases.

Updated

11 months ago
Attachment #8774578 - Flags: review?(nfroyd) → review+
Comment on attachment 8774578 [details]
Bug 1287706 part 5 - Add RemoveElementsBy method to nsTArray and nsTObserverArray to allow filtering elements by predicate function.

https://reviewboard.mozilla.org/r/67008/#review64530

::: xpcom/glue/nsTArray.h:1598
(Diff revision 3)
> +  // This method removes elements based on the return value of the
> +  // callback function aPredicate. If the function returns true for
> +  // an element, the element is removed. aPredicate will be called
> +  // for each element in order. It is not safe to access the array
> +  // inside aPredicate.
> +  void RemoveElementsBy(mozilla::function<bool(const elem_type&)> aPredicate);

We are going to have to make mozilla::function allocation-free at some point if we're going to start adding a bunch of interfaces like this.

::: xpcom/glue/nsTArray.h:1919
(Diff revision 3)
> +        copy_type::MoveNonOverlappingRegion(Elements() + j, Elements() + i,
> +                                            1, sizeof(elem_type));

It would be kind of nice to do block copies where possible, but I think keeping track of that would significantly complicate the implementation.
(Assignee)

Comment 61

11 months ago
https://reviewboard.mozilla.org/r/67008/#review64530

> We are going to have to make mozilla::function allocation-free at some point if we're going to start adding a bunch of interfaces like this.

It's supprising that mozilla::function needs allocation...

> It would be kind of nice to do block copies where possible, but I think keeping track of that would significantly complicate the implementation.

For the only currently use, block copy wouldn't make anything better (because we need to call ctor and dtor for each element). I considered doing block copy when I wrote this, but I thought that is a bit too complicated, and I'd like to start with a simple one. But yeah, we should definitely consider makeing it do block copies later. I'll file a bug.

Comment 62

11 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/003da22902a6
part 1 - Make CallbackObjectHolder movable (and actually move-only). r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee0b27b208d1
part 2 - Remove useless runtime check of EventListenerManager::Listener. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b858cd9f95fb
part 3 - Make EventListenerHandler::Listener movable. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f26a98e3e7b0
part 4 - Unify B2G device-type events to be handled in the same way as others device-type events. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a53cb28a89a1
part 5 - Add RemoveElementsBy method to nsTArray and nsTObserverArray to allow filtering elements by predicate function. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/2439dff9e9c4
part 6 - Separate notifying listener removal to an independent method. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc5c362afc7c
part 7 - Add support of AddEventListenerOptions.once. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/63b876fe012a
part 8 - Add web-platform-test for once option. r=smaug

Comment 63

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/003da22902a6
https://hg.mozilla.org/mozilla-central/rev/ee0b27b208d1
https://hg.mozilla.org/mozilla-central/rev/b858cd9f95fb
https://hg.mozilla.org/mozilla-central/rev/f26a98e3e7b0
https://hg.mozilla.org/mozilla-central/rev/a53cb28a89a1
https://hg.mozilla.org/mozilla-central/rev/2439dff9e9c4
https://hg.mozilla.org/mozilla-central/rev/dc5c362afc7c
https://hg.mozilla.org/mozilla-central/rev/63b876fe012a
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Doc updated:
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener
and
https://developer.mozilla.org/en-US/Firefox/Releases/50#DOM_HTML_DOM
Keywords: dev-doc-needed → dev-doc-complete

Updated

10 months ago
Duplicate of this bug: 1267505

Updated

13 days ago
Depends on: 1367372
You need to log in before you can comment on or make changes to this bug.