238773 - (nsdomevent_separate) Separating nsDOMEvent into separate classes

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement)

Description

[Ilya Konstantinov]

nsDOMEvent is a mishmash of code designed to handle all types of DOM events (by
implementing all nsIDOM*Event ifaces). I wish to separate nsDOMEvent into
separate classes in proper hierarchy. (This is preparing the grounds for my DOM
Level 3 events implementation.)

This is roughly what I want (for MouseEvent):

nsDOMMouseEvent implements nsIDOMMouseEvent, extends nsDOMUIEvent
nsDOMUIEvent implements nsIDOMUIEvent, extends nsDOMEvent
nsDOMEvent implements nsIDOMEvent

Compatibility note
------------------
Today, if you take a MouseEvent and get its' "keycode" property, it'll return
-1. With my change, it'll return null. Similar effects would happen when you
treating keyboard events as mouse events. I doubt anyone relies on that behavior.

I would be taking on this code-wise. This bug is for tracking.

Comment 1

[Ilya Konstantinov]

Attachment: Initial patch

A bit unfocused, this patch:
1. Separates events into classes.
2. Adds DOM3 ifaces for Event, UIEvent, KeyboardEvent, MouseEvent, TextEvent
and MutationEvent.
3. Adds the XML Events namespace ID, as required by DOM3 Events.
4. Fixes bug 246447 (can be stripped out).
5. Renames bryner's NS_DOMUI_ACTIVATE to NS_UI_ACTIVATE (and the same goes for
all other occurances of DOMUI), to prevent it from clashing with content/events
and to make a clean separation between the widget/ world (non-DOM-aware) and
the content/ world.
6. Additional VKs added for left/right modifier keys. DOM3 requires us to make
the distinction between them. Compatibility is preserved in KeyEvent by always
serving the old VKs.

Known issues:
1. TextEvent is provided only when input is thru IME. This is not what the spec
dictates.
2. When HTML events originate from the UI, UIEvent should be used. Currently,
Event is used. UIEvent should be always used for Scroll and Resize events.
3. Only gtk widget backend waschanged to make use of the left/right modifier
VKs.

Comment 2

[Ilya Konstantinov]

Attachment: Updated patch

Changes:
1. Merges cleanly against trunk.
2. nsDOMUIEvent::GetView now AddRefs, like it should. (This bug caused
drag-and-drop to crash and burn.)

BTW,
1. nsDOMTextEvent::GetInputRange doesn't addref on the object it returns, but
its only user (the editor) addrefs on it after retrieving it. Ugly, but
apparently ... by design?
2. Is there some horrible bug in the trunk right now that causes it to open all
pages in the first window/tab? I sure hope it's not my code...

Comment 3

[Ilya Konstantinov]

Attachment: Replaces nsEventListenerManager.cpp in the previous patch

- Fixed a real horrible bug, resulting from treating nsString.Equals... as a
strcmp-style comparison function.
- Adapted to merge nsEventListenerManager.cpp changes cleanly into trunk.

Comment 4

[Vidar Haarr (not reading bugmail)]

Ilya: when you are ready, you should set the 'review ?' and 'superreview ?'
request-flags on your patches (in Edit), so that the other Mozilla developers
can review your code and provide feedback. If you think the patches are ready,
that is :)

I guess you should ask peterv, jst, caillon, sicking or bzbarsky for help.

Comment 5

[Ilya Konstantinov]

The current patch also adds DOM 3 support, which should be a separate bug 
that'll take a separate review. Currently I've finished the work on separating 
the DOM3 stuff out of this patch and I will submit it for review soon. 

Comment 6

[Ilya Konstantinov]

Attachment: The nsDOMEvent class refactored into multiple classes, without DOM3 ifaces

This patch is stripped from the DOM3 work I did. The DOM3 ifaces implementation
will come in a separate bug, after this patch goes in.

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 154872 [details] [diff] [review]
The nsDOMEvent class refactored into multiple classes, without DOM3 ifaces

>--- content/base/src/nsGenericElement.cpp	30 Jul 2004 13:00:35 -0000	3.343
>+++ content/base/src/nsGenericElement.cpp	31 Jul 2004 18:08:34 -0000
>@@ -1156,8 +1156,9 @@ nsGenericElement::InternalIsSupported(co
>              PL_strcasecmp(f, "CSS") == 0 ||
>              PL_strcasecmp(f, "CSS2") == 0 ||
>              PL_strcasecmp(f, "Events") == 0 ||
>-//           PL_strcasecmp(f, "UIEvents") == 0 ||
>+             PL_strcasecmp(f, "UIEvents") == 0 ||
>              PL_strcasecmp(f, "MouseEvents") == 0 ||

I'm not sure we should claim to support UIEvents when we don't do DOMFocusIn
and DOMFocusOut...

Other than that, r=me.

Comment 8

[Ilya Konstantinov]

DOMFocusIn and DOMFocusOut? Sure we do! Didn't you contribute this in the first
place (nsEvent message type NS_DOMUI_FOCUS... which I've renamed to NS_UI_FOCUS...)?

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 154872 [details] [diff] [review]
The nsDOMEvent class refactored into multiple classes, without DOM3 ifaces

What bryner said, and I'd prefer to see you add your email address in the
contributor section in the comments, like others tend to do.

sr=jst

Comment 10

[Johnny Stenback (:jst)]

Duh, sorry, missed your last comment. Yeah, seems like we do support all that's
needed for claiming support for UIEvents now. Sweet! :-)

Comment 11

[Ilya Konstantinov]

Attachment: Previous patch, with my email address

No changes since the previous patch, except for adding my email address to the
contributors list (as per jst's comment).

Comment 12

[Ilya Konstantinov]

Attachment: Same patch, without the nsIPresContext bitrot

Same patch, without the s/nsIPresContext/nsPresContext/ bitrot. This patch
should apply cleanly on the trunk.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I find it hard to believe that recyling event objects matters at all for
performance. I propose just throwing that out, unless someone can demonstrate a
plausible need for it.

Comment 14

[Peter Van der Beken [:peterv]]

See comment 12 in bug 64696. Though I suppose we could try to take it out and
see if things have changed since then.

Comment 15

[Ilya Konstantinov]

Checked into trunk. We'll see about aviary once it bakes in the trunk for a 
while. Meanwhile I continue to the DOM3 bug (bug 256333). 

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This regressed Tdhtml and probably Tp a bit.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: reduce forwarding a little

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #14)
> See comment 12 in bug 64696. Though I suppose we could try to take it out and
> see if things have changed since then.

Interesting. I still find it hard to believe.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I strongly recommend against doing this for Aviary or 1.7.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This caused bug 256464 (major IME regression).

Comment 21

[Wladimir Palant]

There is one more regression - document.createEvent("KeyEvents") creates mouse
events now. Patch for nsEventListenerManager.cpp should be:

     if (str.EqualsIgnoreCase("KeyboardEvent") ||
         str.EqualsIgnoreCase("KeyEvents"))
-       return NS_NewDOMMouseEvent(aDOMEvent, aPresContext,
NS_STATIC_CAST(nsKeyEvent*,aEvent));
+       return NS_NewDOMKeyboardEvent(aDOMEvent, aPresContext,
NS_STATIC_CAST(nsKeyEvent*,aEvent));
    if (str.EqualsIgnoreCase("MutationEvent") ||
        str.EqualsIgnoreCase("MutationEvents"))

Comment 22

[Wladimir Palant]

Attachment: Regression fix

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 157795 [details] [diff] [review]
Regression fix

sr=jst

Comment 24

[Ilya Konstantinov]

bryner's last patch looks good. (Hmmm...The compiler didn't warn me cause 
NS_NewDOMMouseEvent accepts an nsInputEvent, which nsKeyEvent inherits from.) 

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 157795 [details] [diff] [review]
Regression fix

Make that r+sr=jst

Comment 26

[Johnny Stenback (:jst)]

Regression fix checked in.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 154872 [details] [diff] [review]
The nsDOMEvent class refactored into multiple classes, without DOM3 ifaces

>+      case NS_POPUP_EVENT:
>+        return NS_NewDOMPopupBlockedEvent(aDOMEvent, aPresContext, NS_STATIC_CAST(nsPopupBlockedEvent*,aEvent));

Shouldn't this have been NS_POPUPBLOCKED_EVENT?

Comment 28

[Ilya Konstantinov]

From a short LXR search, it looks like all popup blocked events are now
generated with mEvent->eventStructType == NS_POPUPBLOCKED_EVENT, so both
content/events/src/nsDOMUIEvent.cpp and
content/events/src/nsEventListenerManager.cpp should be changed not to refer to
NS_POPUP_EVENT anymore.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Ilya, post a patch (in a new bug?)?