329127 - Simplify nsDOMEvent::DuplicatePrivateData

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

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

nsDOMEvent::DuplicatePrivateData gets actually implemented in Bug 234455.
We should have a simple mechanism to extend nsDOMEvent::DuplicatePrivateData
to support new event types. Perhaps using macros or something...

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: part.1 Make Assign*EventData() a virtual method

By creating virtual methods of event classes, nsDOMEvent::DuplicatePrivateData() can remove the switch statement.

This patch makes Assign*EventData() to a virtual method AssignEventData(). By this patch, AssignEventData() can be called outside the switch statement.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: part.2 Remove a constructor of WidgetPointerEvent which takes a lot of member values

For making new instance of event class by one line, we need to remove some arguments of constructors which are only used for specific class.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: part.3 Remove a constructor of WidgetTouchEvent which takes another instance with trusted flag

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: part.4 Remove a constructor of InternalAnimationEvent which takes a lot of member values

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: part.5 Remove a constructor of InternalTransitionEvent which takes a lot of member values

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: part.6 Remove unnecessary arguments of a constructor of WidgetSimpleGestureEvent

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: part.7 Remove aDetail of constructor of InternalUIEvent

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: part.8 Separate initializing command of WidgetCommandEvent from its constructor

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: part.9 Remove arguments for reason and context menu type of the constructor of WidgetMouseEvent

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: part.10 Implement WidgetEvent::CreateNewInstance() for duplicating new instance of same inherited class

Finally, creates new instance with same class of existing instance. We can do it with new virtual method, WidgetEvent::CreateNewInstance().

Then, we can make nsDOMEvent::DuplicatePrivateData() simple. And nobody needs to touch it when anybody add new event class.

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=7e31502f5abc

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: part.6 Remove unnecessary arguments of a constructor of WidgetSimpleGestureEvent

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: part.10 Implement WidgetEvent::CreateNewInstance() for duplicating new instance of same inherited class

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: part.6 Remove unnecessary arguments of a constructor of WidgetSimpleGestureEvent

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: part.10 Implement WidgetEvent::CreateNewInstance() for duplicating new instance of same inherited class

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=964f9a4c4195

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Comment on attachment 8360975 [details] [diff] [review]
part.1 Make Assign*EventData() a virtual method

See comment 2.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Comment on attachment 8360978 [details] [diff] [review]
part.2 Remove a constructor of WidgetPointerEvent which takes a lot of member values

See comment 3.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Comment on attachment 8361514 [details] [diff] [review]
part.10 Implement WidgetEvent::CreateNewInstance() for duplicating new instance of same inherited class

See comment 11.

Comment 19

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

Comment on attachment 8360975 [details] [diff] [review]
part.1 Make Assign*EventData() a virtual method

># HG changeset patch
># User Masayuki Nakano <[e-mail]>
># Parent c2555af879bdfc57a8cc4e46c38c284283d8a11d
>Bug 329127 part.1 Make Assign*EventData() a virtual method r=
>
>diff --git a/dom/events/MutationEvent.h b/dom/events/MutationEvent.h
>--- a/dom/events/MutationEvent.h
>+++ b/dom/events/MutationEvent.h
>@@ -10,42 +10,48 @@
> #include "nsCOMPtr.h"
> #include "nsIAtom.h"
> #include "nsIDOMNode.h"
> 
> namespace mozilla {
> 
> class InternalMutationEvent : public WidgetEvent
> {
>+  NS_IMPL_EVENT_CLASS_COMMON_VIRTUAL_METHOD(Internal, MutationEvent,
>+                                            WidgetEvent)
> public:
>-  virtual InternalMutationEvent* AsMutationEvent() MOZ_OVERRIDE { return this; }
>-
>   InternalMutationEvent(bool aIsTrusted, uint32_t aMessage) :
>     WidgetEvent(aIsTrusted, aMessage, NS_MUTATION_EVENT),
>     mAttrChange(0)
>   {
>     mFlags.mCancelable = false;
>   }
> 
>   nsCOMPtr<nsIDOMNode> mRelatedNode;
>   nsCOMPtr<nsIAtom>    mAttrName;
>   nsCOMPtr<nsIAtom>    mPrevAttrValue;
>   nsCOMPtr<nsIAtom>    mNewAttrValue;
>   unsigned short       mAttrChange;
> 
>-  void AssignMutationEventData(const InternalMutationEvent& aEvent,
>-                               bool aCopyTargets)
>+  virtual void AssignEventData(const WidgetEvent& aEvent,
>+                               bool aCopyTargets) MOZ_OVERRIDE
>   {
>-    AssignEventData(aEvent, aCopyTargets);
>+    if (eventStructType != aEvent.eventStructType) {
>+      MOZ_CRASH("Called with invalid event instance");
>+    }
It is somewhat ugly to make everything to have 
virtual void  AssignEventData(const WidgetEvent& aEvent, bool aCopyTargets)
but only accept certain kinds of WidgetEvents as input.


For DuplicatePrivateData, could we add new ctors which take the right kind of event.
That way DuplicatePrivateData wouldn't need to call AssignEventData

Comment 20

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

Comment on attachment 8360975 [details] [diff] [review]
part.1 Make Assign*EventData() a virtual method

So r-, but if you think there is really a strong reason for this approach, please
re-ask review.


Btw, would be nice to have also a patch which has all the changes.
That way it is easier to see the big picture.

Comment 21

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

So the approach the patches take effectively split switch-case of event struct types to
several AssignEventData implementation, since each of those have the struct type check.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

> For DuplicatePrivateData, could we add new ctors which take the right kind
> of event.

Of course, I was thinking about that. However, some event classes already have copy constructor and some other event classes' default copy constructor is used in some places.

So, we need to create a constructor which has different argument from default copy constructor. And it copies only private data (i.e., not all data even though its style is copy constructor).

Therefore, I used this approach which duplicates event class instance with WidgetEvent::CreateNewInstance() (see part.10). And keeping separating AssignEventData() for CreateNewInstance() useful for some other purpose.

(In reply to Olli Pettay [:smaug] from comment #21)
> So the approach the patches take effectively split switch-case of event
> struct types to
> several AssignEventData implementation, since each of those have the struct
> type check.

It's possible to check only in debug build with |#ifdef DEBUG| if you want.



I had another idea which creates new copy constructor, but there are some reasons why I don't take the approach:

1. Some classes already implemented copy constructor.
2. Some classes' default constructors are used in some places.

I.e., we need to design another style constructor which takes additional argument but I have no good idea.

3. Using constructor cannot indicate its meaning.

This what is the main reason why I don't use new constructor approach. Even though the new constructor looks like copy constructor, it does NOT copy all members. It only copies necessary members which referred by DOM event classes. This must make other developers confused.

(It might be better AssignEventData() renamed to AssignPrivateEventData() for indicating what it does)

How about you? If you thought it was not problem and we should create new constructor, I'd create new patches, otherwise, I'll request review for these patches again.

Comment 23

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

Could we add
virtual WidgetEvent* Duplicate(); to WidgetEvent, and then all its subclasses would override it, and
call parent class' Duplicate().

It would be quite close to your CreateNewInstance, but would do the necessary
data copying inside it.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Another approach: WidgetEvent::Duplicate()

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Another approach: WidgetEvent::Duplicate()

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=37812601dd5c

WidgetPointerEvent should be tested by test_assign_event_data.html if it's possible. I'll file a bug after landing this.

WidgetTouchEvent copies WidgetGUIEvent::widget. I'm not sure whether this is a problem. If it's not problem, why don't we copy widget of other events?

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

BTW, I'd like to sort out constructors which initialize some members. Initializing by arguments may cause mistake by wrong order and/or redundant when some specific events don't use the members.

The patches part.2 - part.9 do it. Smaug, if you agree with them, I'll file separated bugs for them.

Comment 27

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

Comment on attachment 8364932 [details] [diff] [review]
Another approach: WidgetEvent::Duplicate()


>+  virtual WidgetEvent* Duplicate() const MOZ_OVERRIDE
>+  {
>+    MOZ_ASSERT(eventStructType == NS_TOUCH_EVENT,
>+               "Duplicate() must be overridden by sub class");
>+    // XXX Why does only WidgetTouchEvent copy the widget?

This shouldn't be a problem, since widget is a strong reference, but would need to look at
TouchEvent usage to see why it is needed.



Anyhow, I think this approach is pretty easy to understand.

Comment 28

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

File a followup for the touch event case?

Comment 29

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> The patches part.2 - part.9 do it. Smaug, if you agree with them, I'll file
> separated bugs for them.
I think removing those ctors is a good thing

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb278efeb18a

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

(In reply to Olli Pettay [:smaug] from comment #28)
> File a followup for the touch event case?

Filed bug 964153.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eb278efeb18a

Comment 33

[ywlam8212]

fixed