nsGUIEvent.h related stuff has been sorted out

[Masayuki Nakano]

Hello. I'd like to share the information about new event related stuff
in Gecko.

nsGUIEvent.h defined and implemented all events (except
nsMutationEvent). Additionally, it's included by a lot of files
including some headers. So, this caused changes in the header needing to
waste long time for rebuilding Gecko.

For fixing this issues, now, nsGUIEvent.h is separated to following
files and nsGUIEvent.h has been removed.

* widget/BasicEvents.h
* widget/ContentEvents.h
* widget/MiscEvents.h
* widget/MouseEvents.
* widget/TextEvents.h
* widget/TextRange.h
* widget/TouchEvents.h

And content/events/public/nsMutationEvent.h is renamed to:

* content/events/public/MutationEvent.h

These files are exported under mozilla. So, when you need to include
some of them, you should write #include like:

#include "mozilla/BasicEvents.h"

FYI: BasicEvent.h is included by the other header files. So, you don't
need to include if directly if you've already included one of the others.

If you need forward declaration of event class or related stuff, you
should *not* do it yourself. Instead of that you should add:

#include "mozilla/EventForwards.h"

Actual file of it is widget/EventForwards.h which has all event classes'
forward declaration and related structs' forward declaration.
Additionally, it defines nsEventStatus, mozilla::Modifiers and
mozilla::KeyNameIndex.

Please don't include mozilla/*Events.h files from header file directly
as far as possible!


Additionally, the event names have been completely sorted out. See
following list for the detail:

BasicEvents.h
* nsEvent -> mozilla::WidgetEvent
* nsGUIEvent -> mozilla::WidgetGUIEvent
* nsInputEvent -> mozilla::WidgetInputEvent
* nsUIEvent -> mozilla::InternalUIEvent

ContentEvents.h
* nsScriptErrorEvent -> mozilla::InternalScriptErrorEvent
* nsScrollPortEvent -> mozilla::InternalScrollPortEvent
* nsScrollAreaEvent -> mozilla::InternalScrollAreaEvent
* nsFormEvent -> mozilla::InternalFormEvent
* nsClipboardEvent -> mozilla::InternalClipboardEvent
* nsFocusEvent -> mozilla::InternalFocusEvent
* nsTransitionEvent -> mozilla::InternalTransitionEvent
* nsAnimationEvent -> mozilla::InternalAnimationEvent

MiscEvents.h
* nsContentCommandEvent -> mozilla::WidgetContentCommandEvent
* nsCommandEvent -> mozilla::WidgetCommandEvent
* nsPluginEvent -> mozilla::WidgetPluginEvent

MouseEvents.h
* nsMouseEvent_base -> mozilla::WidgetMouseEventBase
* nsMouseEvent -> mozilla::WidgetMouseEvent
* nsDragEvent -> mozilla::WidgetDragEvent
* nsMouseScrollEvent -> mozilla::WidgetMouseScrollEvent
* mozilla::widget::WheelEvent -> mozilla::WidgetWheelEvent

TextEvents.h
* nsAlternativeCharCode -> mozilla::AlternativeCharCode
* nsKeyEvent -> mozilla::WidgetKeyboardEvent
* nsCompositionEvent -> mozilla::WidgetCompositionEvent
* nsQueryContentEvent -> mozilla::WidgetQueryContentEvent
* nsSelectionEvent -> mozilla::WidgetSelectionEvent

TextRange.h
* nsTextRangeStyle -> mozilla::TextRangeStyle
* nsTextRange -> mozilla::TextRange
* nsTextRangeArray -> mozilla::TextRangeArray

TouchEvents.h
* nsGestureNotifyEvent -> mozilla::WidgetGestureNotifyEvent
* nsSimpleGestureEvent -> mozilla::WidgetSimpleGestureEvent
* nsTouchEvent -> mozilla::WidgetTouchEvent

MutationEvent.h
* nsMutationEvent -> mozilla::InternalMutationEvent

The naming rules of them are:
1. Defined in "mozilla" namespace.
2. Use prefix "Widget" or "Internal". If the event is dispatched from
widget/, use "Widget". Otherwise, i.e., generated by content/, layout/
or something others, use "Internal".


Next, event classes now have vtable. I.e., they have virtual destructor.
Therefore, even if you create an instance and stores it in a pointer of
its superclass, you don't mind to cast the pointer type to the actual
class. For example,

WidgetEvent* event = new WidgetKeyboardEvent(...);
delete event;

is now safe.


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 hope this change makes you happy!

--
Masayuki Nakano <masa...@d-toybox.com>
Manager, Internationalization, Mozilla Japan.

[Ehsan Akhgari]

On 2013-10-23 3:18 AM, Masayuki Nakano wrote:
> I hope this change makes you happy!

Thanks for doing this!

Ehsan

[Jonathan Watt]

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!

[Tim Abraldes]

> 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_cast<WidgetWheelEvent*>(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_cast<WidgetInputEvent*>(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.

[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 ;-)

> 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.

Sure. As*Event() approach has some advantages especially somebody who
works on event stuff. If somebody needs to change event class hierarchy,
it's difficult to find the static_casts which will be broken. However,
if As*Event() is used, being broken part cause crash due to accessing
nullptr. So, it may be detectable in automated tests.

The cost of using vtable must be enough cheap. Additionally, even if
it's not so, we can do same approach which we've taken in As*Event()
implementation.


BTW, I'm researching that if we can hide or remove eventStructType
completely. Checking eventStructType is doing very raw level work.
Therefore, it's causing new bugs when we add new event class. For
example, some code checks eventStructType for checking a class has if
WidgetMouseEventBase. If somebody add new subclass of it, he/she needs
to find and change such code. If he/she failed to find such code even
only one, the change would cause regression.

Although, I don't think that it causes problem when it checks only the
most derived class. Therefore, I think that creating Is*EventClass()
methods which *cannot* be used in switch statement is better for such
use case.

[Cameron McCormack]

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/

[Masayuki Nakano]

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.

[Cameron McCormack]

Masayuki Nakano wrote:
> I have no idea why you thinks eventStructType should be only in debug
> build.

Only because you mentioned that you were looking at possibly removing
it. If that happens, then I was suggesting it might be helpful to keep
around in debug builds for assertions like nsINode::AsElement does.
(Although there it asserts IsElement(), which relies on a non-debug-only
field.)

> 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.

Yeah, especially for cases where you know for sure that the struct will
be of a certain type, it would be nice to be able to avoid the virtual call.