Closed Bug 910978 Opened 11 years ago Closed 11 years ago

Implement Assign*EventData() for all ns*Event

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(21 files)

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.
Just fix the wrong indent (5 spaces -> 4 spaces).
Attachment #797634 - Flags: review?(bugs)
I don't know how to generate "error" event. Do you know it?
Attachment #797635 - Flags: review?(bugs)
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)
Attachment #797645 - Flags: review?(bugs) → review+
Attachment #797634 - Flags: review?(bugs) → review+
Attachment #797635 - Flags: review?(bugs) → review+
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
I have no idea to test copying drag event.
Attachment #798468 - Flags: review?(bugs)
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)
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()
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)
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.
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+
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-
Attachment #798470 - Flags: review?(bugs) → review+
Attachment #798472 - Flags: review?(bugs) → review+
Attachment #798473 - Flags: review?(bugs) → review+
Attachment #798475 - Flags: review?(bugs) → review+
Attachment #798476 - Flags: review?(bugs) → review+
Attachment #798477 - Flags: review?(bugs) → review+
Attachment #798478 - Flags: review?(bugs) → review+
Attachment #798480 - Flags: review?(bugs) → review+
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+
Attachment #798483 - Flags: review?(bugs) → review+
Attachment #798484 - Flags: review?(bugs) → review+
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]
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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: