Re: nsGUIEvent.h related stuff has been sorted out
On 2013/10/26 12:47, Cameron McCormack wrote: Tim Abraldes: So my question is this; is it preferable to keep the As*Event functions, or to use regular C++ casts to accomplish the same behavior? We could use static_cast unconditionally for upcasts, static_cast with a check of |eventStructType| for downcasts to most-derived event types, and dynamic_cast for downcasts to non-most-derived event types. Masayuki Nakano: Please use As*Event() in case #1 and #2. As you said, we can use static_cast since there is eventStructType. However, it's NOT safe because we *might* take mistakes. Actually, there was such bug. If somebody takes mistake, it may cause breaking memory if it sets value to the member. # Anyway, we don't need to do anything in case #3. Additionally, the user code becomes simple since we can avoid checking eventStructType member ;-) Would it make sense to make eventStructType exist only in debug builds, and to assert its value inside the As*Event() functions? I have no idea why you thinks eventStructType should be only in debug build. Unfortunately, eventStructType indicate only only leaf class type. There are NS_SVGZOOM_EVENT and NS_SMIL_TIME_EVENT. These constants are used for WidgetGUIEvent and InternalUIEvent. And when creating DOM event, these values cause creating different DOM event class from NS_GUI_EVENT and NS_UI_EVENT. I think that they should be removed and used message values at creating the events, though. If we can fix above issue, I guess that we can completely hide the member. Then, if we chose to use virtual method for implementing Is*Event(), we can remove it. But I think that it's better to use eventStructType in them since they are called a lot of times. So, non-virtual methods give better performance for us. -- Masayuki Nakano masay...@d-toybox.com Manager, Internationalization, Mozilla Japan. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: nsGUIEvent.h related stuff has been sorted out
Tim Abraldes: So my question is this; is it preferable to keep the As*Event functions, or to use regular C++ casts to accomplish the same behavior? We could use static_cast unconditionally for upcasts, static_cast with a check of |eventStructType| for downcasts to most-derived event types, and dynamic_cast for downcasts to non-most-derived event types. Masayuki Nakano: Please use As*Event() in case #1 and #2. As you said, we can use static_cast since there is eventStructType. However, it's NOT safe because we *might* take mistakes. Actually, there was such bug. If somebody takes mistake, it may cause breaking memory if it sets value to the member. # Anyway, we don't need to do anything in case #3. Additionally, the user code becomes simple since we can avoid checking eventStructType member ;-) Would it make sense to make eventStructType exist only in debug builds, and to assert its value inside the As*Event() functions? -- Cameron McCormack ≝ http://mcc.id.au/ ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: nsGUIEvent.h related stuff has been sorted out
I hope this change makes you happy!
It most certainly does! I'm a huge proponent of code cleanup and organization,
and this looks like a beautiful example of those.
Finally, the root class, WidgetEvent, has As*Event class. The *
doesn't include the prefix. I.e., for WidgetMouseEvent, the method name
is WidgetEvent::AsMouseEvent(). This returns the pointer of the instance
only when the instance is the class or its derived class. Otherwise,
returns nullptr.
E.g., WidgetDragEvent is defines as:
WidgetEvent
+- WidgetGUIEvent
+- WidgetInputEvent
+- WidgetMouseEventBase
+- WidgetMouseEvent
+- WidgetDragEvent
If an instance is WidgetDragEvent, AsGUIEvent(), AsInputEvent(),
AsMouseEventBase(), AsMouseEvent() and AsDragEvent() returns its
pointer. The other As*Event() methods return nullptr.
You should not use static_cast for Widget*Event and Internal*Event
anymore because it may cause wrong casting bug with some mistakes
(actually, there was such bug!).
I do have a question about this part. It seems that there are 3 distinct
situations where you might be casting pointers around in this inheritance tree.
1. Downcasting to a most-derived class (a leaf in the tree)
2. Downcasting to a non-leaf-node (i.e. a class that is not a most-derived
class)
3. Upcasting
For 1 and 3, I believe we can accomplish these safely using static_cast. For 3,
this is always true. For 1, I believe that the static_cast is safe as long as
you have checked the |eventStructType| member.
That is, the following should be safe:
WidgetWheelEvent *event1 = nullptr;
// Assume that aEvent is a WidgetEvent* that we received from elsewhere
if (NS_WHEEL_EVENT == aEvent-eventStructType) {
// We can now trust this cast
event1 = static_castWidgetWheelEvent*(aEvent);
}
For 2, the |eventStructType| member by itself doesn't give us enough
information to know if a static_cast is safe (you'd have to keep a map of
most-derived classes to all of their parent classes... yuck). In these cases, I
feel that the As*Event functions are providing the same functionality as
dynamic_cast.
That is, the following are equivalent:
// Assume that aEvent is a WidgetEvent* that we received from elsewhere
WidgetInputEvent *event2 = aEvent-AsWheelEvent();
WidgetInputEvent *event3 = dynamic_castWidgetInputEvent*(aEvent);
// At this point, |event2 == event3| should be true
So my question is this; is it preferable to keep the As*Event functions, or to
use regular C++ casts to accomplish the same behavior? We could use static_cast
unconditionally for upcasts, static_cast with a check of |eventStructType| for
downcasts to most-derived event types, and dynamic_cast for downcasts to
non-most-derived event types.
I don't think there would be a meaningful performance impact (case 3 would be
faster, case 2 would be a dynamic cast vs a virtual function call, case 1 would
be a branch vs a virtual function call), but I think it would clean up our
event types even further (again, great work cleaning them up this much!) and it
takes advantage of built-in support from the language to aid us in managing our
event types.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: nsGUIEvent.h related stuff has been sorted out
On 2013-10-23 3:18 AM, Masayuki Nakano wrote: I hope this change makes you happy! Thanks for doing this! Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: nsGUIEvent.h related stuff has been sorted out
On 23/10/2013 09:18, Masayuki Nakano wrote: I hope this change makes you happy! It does - this is great stuff! Thank you for all your work on this! -- Masayuki Nakano masay...@d-toybox.com Manager, Internationalization, Mozilla Japan. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
