1433008 - Make WidgetEvent movable

Status: RESOLVED FIXED

(Core :: Widget, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

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)

Comment 2

[Hiroyuki Ikezoe (:hiro)]

:arai did confirm the patch can be built with XCode clang on MacOSX.  So I think we can land them now. :)

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1433008 - Make WidgetEvent movable.

The copy-assignment in this patch is used in the copy-constructor.
Note that we can't simply use '= default' implementation since we need to use
MOZ_COUNT_CTOR() in the move constructor.

Review commit: https://reviewboard.mozilla.org/r/216502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216502/

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1433008 - Make AnimationEventInfo, InternalTransitionEvent and InternalAnimationEvent movable.

And disallow using copy constructor and copy assignment for the structs.

Review commit: https://reviewboard.mozilla.org/r/216504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216504/

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8946499 [details]
Bug 1433008 - Make WidgetEvent movable.

https://reviewboard.mozilla.org/r/216502/#review222238

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8946500 [details]
Bug 1433008 - Make AnimationEventInfo, InternalTransitionEvent and InternalAnimationEvent movable.

https://reviewboard.mozilla.org/r/216504/#review222240

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Thank you!

A try run; can be built on mingw. :)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2a544af88f9ee155a527cde3abe08c24f91eae5

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Hmm... I don't know.  How have we ever avoided such mistakes?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

At least add some comments somewhere?

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/59ae8c3043e8
https://hg.mozilla.org/mozilla-central/rev/374419fd9b2e

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Thanks. Sounds reasonable.  I will add comments in a later bug.  This bug was now closed. :)

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Filed bug 1434220.  I will work on it tomorrow.