Closed Bug 1433008 Opened 2 years ago Closed 2 years ago

Make WidgetEvent movable

Categories

(Core :: Widget, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: hiro, Assigned: hiro)

Details

Attachments

(2 files)

For animation events handling, we are using InternalTransitionEvent and InternalAnimationEvent, which are inheritances of WidgetEvent, in nsTArray<> and sorting them when we dispatch the events. 

It would be nice to have move constructor and move assignment for WidgetEvent to prevent copying them in the sort.

Though I haven't checked other inheritances, there might be other situations that can avoid copying.
A try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b3cd7ae290637605b7769fdca59cfd8d068aa6f

It didn't finish yet, but looks good so far.

One thing I am concerned is that Brian noted that std::stable_sort() is not move friendly on MacOSX and Android in bug 1263500  two years ago.  Unfortunately our CI uses cross compile for OSX so I can't confirm whether it still uses copy constructor or not.  I appreciate someone tries to build with the patches in the try on OSX.  (Whereas on Android the build is fine on the try)
:arai did confirm the patch can be built with XCode clang on MacOSX.  So I think we can land them now. :)
Comment on attachment 8946499 [details]
Bug 1433008 - Make WidgetEvent movable.

https://reviewboard.mozilla.org/r/216502/#review222238
Attachment #8946499 - Flags: review?(masayuki) → review+
Comment on attachment 8946500 [details]
Bug 1433008 - Make AnimationEventInfo, InternalTransitionEvent and InternalAnimationEvent movable.

https://reviewboard.mozilla.org/r/216504/#review222240
Attachment #8946500 - Flags: review?(masayuki) → review+
Thank you!

A try run; can be built on mingw. :)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2a544af88f9ee155a527cde3abe08c24f91eae5
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/59ae8c3043e8
Make WidgetEvent movable. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/374419fd9b2e
Make AnimationEventInfo, InternalTransitionEvent and InternalAnimationEvent movable. r=masayuki
Ok, so only the basic WidgetEvent is moveable.
Is there any way we could ensure new member variables are added to the move-ctor, and not only to other ctors?
Hmm... I don't know.  How have we ever avoided such mistakes?
At least add some comments somewhere?
https://hg.mozilla.org/mozilla-central/rev/59ae8c3043e8
https://hg.mozilla.org/mozilla-central/rev/374419fd9b2e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Thanks. Sounds reasonable.  I will add comments in a later bug.  This bug was now closed. :)
Filed bug 1434220.  I will work on it tomorrow.
You need to log in before you can comment on or make changes to this bug.