Implement Assign*EventData() for all ns*Event

RESOLVED FIXED in mozilla26

Status

()

Core
DOM: Events
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(21 attachments)

8.47 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.12 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.34 KB, patch
smaug
: review+
roc
: review+
Details | Diff | Splinter Review
6.26 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.28 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.12 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.52 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.29 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.51 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.19 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.84 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.12 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.55 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.75 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.26 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.94 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.36 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.05 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.69 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.13 KB, patch
smaug
: review+
Details | Diff | Splinter Review
32.94 KB, patch
smaug
: review+
Details | Diff | Splinter Review
This is a follow up bug for bug 910156.
Created attachment 797634 [details] [diff] [review]
part.0 Fix wrong indent in test_assign_event_data.html

Just fix the wrong indent (5 spaces -> 4 spaces).
Attachment #797634 - Flags: review?(bugs)
Created attachment 797635 [details] [diff] [review]
part.1 Implement nsScriptErrorEvent::AssignScriptErrorEventData()

I don't know how to generate "error" event. Do you know it?
Attachment #797635 - Flags: review?(bugs)
Created attachment 797645 [details] [diff] [review]
part.2 Remove nsScrollbarEvent because its member is never used

nsScrollbarEvent::position isn't used...
Comment on attachment 797645 [details] [diff] [review]
part.2 Remove nsScrollbarEvent because its member is never used

nsScrollbarEvent::position isn't used. So, we can remove nsScrollbarEvent and nsGfxScrollFrame should use nsGUIEvent for NS_SCROLL_EVENT.
Attachment #797645 - Flags: review?(roc)
Attachment #797645 - Flags: review?(bugs)
Depends on: 911463

Updated

4 years ago
Attachment #797645 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #797634 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #797635 - Flags: review?(bugs) → review+

Comment 5

4 years ago
Comment on attachment 797635 [details] [diff] [review]
part.1 Implement nsScriptErrorEvent::AssignScriptErrorEventData()

Script error events are odd, and we even handle them specially in 
http://mxr.mozilla.org/mozilla-central/source/dom/src/events/nsJSEventListener.cpp#182.
But unless someone actually copying errorMsg and fileName, perhaps not worth
to handle that case. And I'm not even sure the spec requires copying those.
EventHandler for errors is rather special.
http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#onerroreventhandler
Created attachment 798466 [details] [diff] [review]
part.3 Implement nsScrollPortEvent::AssignScrollPortEventData()
Attachment #798466 - Flags: review?(bugs)
Created attachment 798467 [details] [diff] [review]
part.4 Implement nsScrollAreaEvent::AssignScrollAreaEventData()
Attachment #798467 - Flags: review?(bugs)
Created attachment 798468 [details] [diff] [review]
part.5 Implement nsDragEvent::AssignDragEventData()

I have no idea to test copying drag event.
Attachment #798468 - Flags: review?(bugs)
Created attachment 798469 [details] [diff] [review]
part.6 Implement nsTextEvent::AssignTextEventData() and make nsTextEvent not a derived class of nsInputEvent because nobody uses the stored data

nsTextEvent inherits nsInputEvent. However, its modifier state information isn't available from JS. Additionally, nobody refers the modifier information and most platform widgets don't set modifier information (except only IMM module of Windows).

I believe that nobody needs modifier information for nsTextEvent like nsCompositionEvent. So, this patch make nsTextEvent inherit nsGUIEvent directly.

For safety, nsDOMUIEvent::GetModifierStateInternal() should check if the mEvent inherits nsInputEvent actually. But NS_IS_INPUT_EVENT() doesn't include NS_WHEEL_EVENT. This is out of scope of this bug. But it should be fixed in this patch because it causes crash in the possible path.
Attachment #798469 - Flags: review?(bugs)
Created attachment 798470 [details] [diff] [review]
part.7 Implement nsCompositionEvent::AssignCompositionEventData()
Attachment #798470 - Flags: review?(bugs)
Created attachment 798472 [details] [diff] [review]
part.8 Implement nsMouseScrollEvent::AssignMouseScrollEventData()
Attachment #798472 - Flags: review?(bugs)
Created attachment 798473 [details] [diff] [review]
part.9 Implement widget::WheelEvent::AssignWheelEventData()
Attachment #798473 - Flags: review?(bugs)
Created attachment 798475 [details] [diff] [review]
part.10 Implement nsTouchEvent::AssignTouchEventData()
Attachment #798475 - Flags: review?(bugs)
Created attachment 798476 [details] [diff] [review]
part.11 Implement nsFormEvent::AssignFormEventData()
Attachment #798476 - Flags: review?(bugs)
Created attachment 798477 [details] [diff] [review]
part.12 Implement nsCommandEvent::AssignCommandEventData()

I have no idea to test copying nsCommandEvent except we add nsIDOMWindowUtils::SendCommandEvent(). However, I'm not sure if it's worthwhile. Additionally, if the command is handled by chrome first like simple gesture, it's difficult to test it.

This patch doesn't add new test for nsCommandEvent.
Attachment #798477 - Flags: review?(bugs)
Attachment #798477 - Attachment description: bug910978-12.diffpart.12 Implement nsCommandEvent::AssignCommandEventData() → part.12 Implement nsCommandEvent::AssignCommandEventData()
Created attachment 798478 [details] [diff] [review]
part.13 Implement nsClipboardEvent::AssignClipboardEventData()
Attachment #798478 - Flags: review?(bugs)
Created attachment 798480 [details] [diff] [review]
part.14 Implement nsUIEvent::AssignUIEventData()
Attachment #798480 - Flags: review?(bugs)
Created attachment 798481 [details] [diff] [review]
part.15 Implement nsFocusEvent::AssignFocusEventData()
Attachment #798481 - Flags: review?(bugs)
Created attachment 798482 [details] [diff] [review]
part.16 Implement nsSimpleGestureEvent::AssignSimpleGestureEventData()

Simple gesture events never fired on content in normal settings (except debug pref, bug 893973). Additionally, the event is handled by chrome first.

Anyway, it's not so important to add test for copying nsSimpleGestureEvent.
Attachment #798482 - Flags: review?(bugs)
Created attachment 798483 [details] [diff] [review]
part.17 Implement nsTransitionEvent::AssignTransitionEventData()
Attachment #798483 - Flags: review?(bugs)
Created attachment 798484 [details] [diff] [review]
part.18 Implement nsAnimationEvent::AssignAnimationEventData()
Attachment #798484 - Flags: review?(bugs)
Created attachment 798485 [details] [diff] [review]
part.19 Implement nsMutationEvent::AssignMutationEventData()
Attachment #798485 - Flags: review?(bugs)
At creating these patches, I waste a lot of time for rebuilding since nsGUIEvent.h is included by a lot of files. I think that nsGUIEvent.h should be separated for each event. And user files should include only necessary event's header files.
Well, usually nsGUIEvent.h doesn't need to be changed. I wouldn't want to see tons of new files, but
perhaps we could have few ones, like on containing the base structs, one for mouse stuff, one
for key events and one for other stuff.
Sure.

Updated

4 years ago
Attachment #798466 - Flags: review?(bugs) → review+
Comment on attachment 798467 [details] [diff] [review]
part.4 Implement nsScrollAreaEvent::AssignScrollAreaEventData()

># HG changeset patch
># User Masayuki Nakano <masayuki@d-toybox.com>
># Parent eed8b9530f22dd464c53002aefd739f129fc6ae0
>Bug 910978 part.4 Implement nsScrollAreaEvent::AssignScrollAreaEventData() r=
>
>diff --git a/content/events/src/nsDOMEvent.cpp b/content/events/src/nsDOMEvent.cpp
>--- a/content/events/src/nsDOMEvent.cpp
>+++ b/content/events/src/nsDOMEvent.cpp
>@@ -662,18 +662,17 @@ nsDOMEvent::DuplicatePrivateData()
>       break;
>     }
>     case NS_SCROLLAREA_EVENT:
>     {
>       nsScrollAreaEvent* oldScrollAreaEvent =
>         static_cast<nsScrollAreaEvent*>(mEvent);
>       nsScrollAreaEvent* scrollAreaEvent = 
>         new nsScrollAreaEvent(false, msg, nullptr);
>-      scrollAreaEvent->AssignGUIEventData(*oldScrollAreaEvent, true);
>-      scrollAreaEvent->mArea = oldScrollAreaEvent->mArea;
>+      scrollAreaEvent->AssignScrollAreaEventData(*oldScrollAreaEvent, true);
>       newEvent = scrollAreaEvent;
>       break;
>     }
>     case NS_MUTATION_EVENT:
>     {
>       nsMutationEvent* mutationEvent = new nsMutationEvent(false, msg);
>       nsMutationEvent* oldMutationEvent =
>         static_cast<nsMutationEvent*>(mEvent);
>diff --git a/widget/nsGUIEvent.h b/widget/nsGUIEvent.h
>--- a/widget/nsGUIEvent.h
>+++ b/widget/nsGUIEvent.h
>@@ -813,16 +813,24 @@ class nsScrollAreaEvent : public nsGUIEv
> {
> public:
>   nsScrollAreaEvent(bool isTrusted, uint32_t msg, nsIWidget *w)
>     : nsGUIEvent(isTrusted, msg, w, NS_SCROLLAREA_EVENT)
>   {
>   }
> 
>   nsRect mArea;
>+
>+  void AssignScrollAreaEventData(const nsScrollAreaEvent& aEvent,
>+                                 bool aCopyTargets)
>+  {
>+    AssignGUIEventData(aEvent, aCopyTargets);
>+
>+    mArea = aEvent.mArea;
>+  }
> };
> 
> class nsInputEvent : public nsGUIEvent
> {
> protected:
>   nsInputEvent(bool isTrusted, uint32_t msg, nsIWidget *w,
>                nsEventStructType structType)
>     : nsGUIEvent(isTrusted, msg, w, structType),
>diff --git a/widget/tests/test_assign_event_data.html b/widget/tests/test_assign_event_data.html
>--- a/widget/tests/test_assign_event_data.html
>+++ b/widget/tests/test_assign_event_data.html
>@@ -78,16 +78,38 @@ const kTests = [
>     dispatchEvent: function () {
>       document.getElementById("scrolled-div").style.width = "500px";
>     },
>     canRun: function () {
>       return true;
>     },
>     todoMismatch: [],
>   },
>+  { description: "nsScrollAreaEvent (MozScrolledAreaChanged, spreading)",
>+    target: function () { return document; }, eventType: "MozScrolledAreaChanged",
>+    dispatchEvent: function () {
>+      document.getElementById("scrollable-div").style.width = "50000px";
>+      document.getElementById("scrollable-div").style.height = "50000px";
>+    },
>+    canRun: function () {
>+      return true;
>+    },
>+    todoMismatch: [],
>+  },
>+  { description: "nsScrollAreaEvent (MozScrolledAreaChanged, shrinking)",
>+    target: function () { return document; }, eventType: "MozScrolledAreaChanged",
>+    dispatchEvent: function () {
>+      document.getElementById("scrollable-div").style.width = "30px";
>+      document.getElementById("scrollable-div").style.height = "30px";
>+    },
>+    canRun: function () {
>+      return true;
>+    },
>+    todoMismatch: [],
>+  },
>   { description: "nsKeyEvent (keydown of 'a' key without modifiers)",
>     targetID: "input-text", eventType: "keydown",
>     dispatchEvent: function () {
>       document.getElementById(this.targetID).value = "";
>       document.getElementById(this.targetID).focus();
>       if (kIsWin) {
>         gUtils.sendNativeKeyEvent(WIN_KL_US, WIN_VK_A, 0, "a", "a");
>       } else if (kIsMac) {
>@@ -190,17 +212,17 @@ const kTests = [
> function doTest(aTest)
> {
>   if (!aTest.canRun()) {
>     SimpleTest.executeSoon(runNextTest);
>     return;
>   }
>   gEvent = null;
>   gCopiedEvent = [];
>-  var target = document.getElementById(aTest.targetID);
>+  var target = aTest.target ? aTest.target() : document.getElementById(aTest.targetID);
>   target.addEventListener(aTest.eventType, onEvent, true);
>   gCallback = function () {
>     target.removeEventListener(aTest.eventType, onEvent, true);
>     ok(gEvent !== null, aTest.description + ": failed to get duplicated event");
>     ok(gCopiedEvent.length > 0, aTest.description + ": count of attribute of the event must be larger than 0");
>     for (var i = 0; i < gCopiedEvent.length; ++i) {
>       var name = gCopiedEvent[i].name;
>       if (name == "rangeOffset") {
Attachment #798467 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #798468 - Flags: review?(bugs) → review+
Comment on attachment 798469 [details] [diff] [review]
part.6 Implement nsTextEvent::AssignTextEventData() and make nsTextEvent not a derived class of nsInputEvent because nobody uses the stored data

ah, we don't really need anything from nsInputEvent in nsTextEvent.
But I don't understand the change to NS_IS_INPUT_EVENT
Attachment #798469 - Flags: review?(bugs) → review-

Updated

4 years ago
Attachment #798470 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #798472 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #798473 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #798475 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #798476 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #798477 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #798478 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #798480 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #798481 - Flags: review?(bugs) → review+
Comment on attachment 798482 [details] [diff] [review]
part.16 Implement nsSimpleGestureEvent::AssignSimpleGestureEventData()

We actually could test this in chrome tests.
Attachment #798482 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #798483 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #798484 - Flags: review?(bugs) → review+

Updated

4 years ago
Attachment #798485 - Flags: review?(bugs) → review+
Comment on attachment 798469 [details] [diff] [review]
part.6 Implement nsTextEvent::AssignTextEventData() and make nsTextEvent not a derived class of nsInputEvent because nobody uses the stored data

>diff --git a/content/events/src/nsDOMUIEvent.cpp b/content/events/src/nsDOMUIEvent.cpp
>--- a/content/events/src/nsDOMUIEvent.cpp
>+++ b/content/events/src/nsDOMUIEvent.cpp
>@@ -455,16 +455,19 @@ nsDOMUIEvent::ComputeModifierState(const
>   }
> 
>   return modifiers;
> }
> 
> bool
> nsDOMUIEvent::GetModifierStateInternal(const nsAString& aKey)
> {
>+  if (!NS_IS_INPUT_EVENT(mEvent)) {
>+    MOZ_CRASH("mEvent must be nsInputEvent or derived class");
>+  }
>   nsInputEvent* inputEvent = static_cast<nsInputEvent*>(mEvent);
>   if (aKey.EqualsLiteral(NS_DOM_KEYNAME_SHIFT)) {
>     return inputEvent->IsShift();
>   }
>   if (aKey.EqualsLiteral(NS_DOM_KEYNAME_CONTROL)) {
>     return inputEvent->IsControl();
>   }
>   if (aKey.EqualsLiteral(NS_DOM_KEYNAME_META)) {
>diff --git a/widget/nsGUIEvent.h b/widget/nsGUIEvent.h
>--- a/widget/nsGUIEvent.h
>+++ b/widget/nsGUIEvent.h
>@@ -1326,44 +1326,54 @@ struct nsTextRange
> 
>   nsTextRangeStyle mRangeStyle;
> 
>   uint32_t Length() const { return mEndOffset - mStartOffset; }
> };
> 
> typedef nsTextRange* nsTextRangeArray;
> 
>-class nsTextEvent : public nsInputEvent
>+class nsTextEvent : public nsGUIEvent
> {
> private:
>   friend class mozilla::dom::PBrowserParent;
>   friend class mozilla::dom::PBrowserChild;
>   friend class mozilla::plugins::PPluginInstanceChild;
> 
>   nsTextEvent()
>   {
>@@ -1919,20 +1929,20 @@ enum nsDragDropEventStatus {
>   /// The event is drop
>   nsDragDropEventStatus_eDrop  
> };
> 
> #define NS_IS_INPUT_EVENT(evnt) \
>        (((evnt)->eventStructType == NS_INPUT_EVENT) || \
>         ((evnt)->eventStructType == NS_MOUSE_EVENT) || \
>         ((evnt)->eventStructType == NS_KEY_EVENT) || \
>-        ((evnt)->eventStructType == NS_TEXT_EVENT) || \
>         ((evnt)->eventStructType == NS_TOUCH_EVENT) || \
>         ((evnt)->eventStructType == NS_DRAG_EVENT) || \
>         ((evnt)->eventStructType == NS_MOUSE_SCROLL_EVENT) || \
>+        ((evnt)->eventStructType == NS_WHEEL_EVENT) || \
>         ((evnt)->eventStructType == NS_SIMPLE_GESTURE_EVENT))
> 
> #define NS_IS_MOUSE_EVENT(evnt) \
>        (((evnt)->message == NS_MOUSE_BUTTON_DOWN) || \
>         ((evnt)->message == NS_MOUSE_BUTTON_UP) || \
>         ((evnt)->message == NS_MOUSE_CLICK) || \
>         ((evnt)->message == NS_MOUSE_DOUBLECLICK) || \
>         ((evnt)->message == NS_MOUSE_ENTER) || \

> ah, we don't really need anything from nsInputEvent in nsTextEvent.
> But I don't understand the change to NS_IS_INPUT_EVENT

Removing NS_TEXT_EVENT check is, nsTextEvent stops inheriting nsInputEvent by this patch.

Adding NS_WHEEL_EVENT check is, widget::WheelEvent inherits nsInputEvent but it's not been added here. So, we need to fix it in another bug if it's possible. However, this patch changes nsDOMUIEvent::GetModifierStateInternal(). If the method is called with wrong event which doesn't inherit nsInputEvent, it crashes for easy to find a bug if developer forgets to change NS_IS_INPUT_EVENT() when he/she add new event. Additionally, this prevents to reference out of the instance of mEvent. So, if some web pages tries to call getModifierState() of wheel event, this patch causes new crash. Therefore, we need to fix it in this patch.

Well, I should add new test for this new crash.
Attachment #798469 - Flags: review- → review?(bugs)
Comment on attachment 798469 [details] [diff] [review]
part.6 Implement nsTextEvent::AssignTextEventData() and make nsTextEvent not a derived class of nsInputEvent because nobody uses the stored data

Thanks for the explanation.
Attachment #798469 - Flags: review?(bugs) → review+
Thank you for your review. I'll land them soon. However, I'll post new patch, part.20 for testing all events which have getModifierState() not crashed.
Whiteboard: [leave open]
Created attachment 799295 [details] [diff] [review]
part.20 Test calls of getModifierState() of all events not causing crash

This test is useful for other tests which try to test base event implementation from derived event.

If this test is landed, developer who implements new DOM event(s) need to update this test.
Attachment #799295 - Flags: review?(bugs)
Whiteboard: [leave open]
Comment on attachment 799295 [details] [diff] [review]
part.20 Test calls of getModifierState() of all events not causing crash

Why the file name _untrusted? How is that relevant?
Would _synthetic be better.
Attachment #799295 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/901804d26f9f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.