Closed Bug 1287706 Opened 8 years ago Closed 8 years ago

Support "once" option for addEventListener

Categories

(Core :: DOM: Events, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(8 files, 1 obsolete file)

58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
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?
Priority: -- → P3
Attached patch wip patch (obsolete) — Splinter Review
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 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+
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?
(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...
(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.
(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.
Things like NotifyAboutMainThreadListenerChange (which is mainly for devtools)? Well, that needs to be fired for all the listeners.
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: nobody → xidorn+moz
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/
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/
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/
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/
Attachment #8772713 - Attachment is obsolete: true
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...
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/
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/
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/
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/
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/
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/
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/
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 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 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-
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.
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 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-
Attachment #8774581 - Flags: review?(bugs) → review+
Attachment #8774580 - Flags: review?(bugs) → review-
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.
(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 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 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+
(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?
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.
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).
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.
Attachment #8774574 - Flags: review- → review?(bugs)
Attachment #8774576 - Flags: review- → review?(bugs)
Attachment #8774580 - Flags: review- → review?(bugs)
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.
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/
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/
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/
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/
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/
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/
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/
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/
Attachment #8774579 - Flags: review+ → review?(bugs)
(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 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+
(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 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 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+
Attachment #8774579 - Flags: review?(bugs) → review+
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.
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.
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.
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
You need to log in before you can comment on or make changes to this bug.