Closed
Bug 910978
Opened 11 years ago
Closed 11 years ago
Implement Assign*EventData() for all ns*Event
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
Attachments
(21 files)
This is a follow up bug for bug 910156.
Assignee | ||
Comment 1•11 years ago
|
||
Just fix the wrong indent (5 spaces -> 4 spaces).
Attachment #797634 -
Flags: review?(bugs)
Assignee | ||
Comment 2•11 years ago
|
||
I don't know how to generate "error" event. Do you know it?
Attachment #797635 -
Flags: review?(bugs)
Assignee | ||
Comment 3•11 years ago
|
||
nsScrollbarEvent::position isn't used...
Assignee | ||
Comment 4•11 years ago
|
||
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?(roc) → review+
Updated•11 years ago
|
Attachment #797645 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Attachment #797634 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Attachment #797635 -
Flags: review?(bugs) → review+
Comment 5•11 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
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #798466 -
Flags: review?(bugs)
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #798467 -
Flags: review?(bugs)
Assignee | ||
Comment 8•11 years ago
|
||
I have no idea to test copying drag event.
Attachment #798468 -
Flags: review?(bugs)
Assignee | ||
Comment 9•11 years ago
|
||
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)
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #798470 -
Flags: review?(bugs)
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #798472 -
Flags: review?(bugs)
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #798473 -
Flags: review?(bugs)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #798475 -
Flags: review?(bugs)
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #798476 -
Flags: review?(bugs)
Assignee | ||
Comment 15•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #798477 -
Attachment description: bug910978-12.diffpart.12 Implement nsCommandEvent::AssignCommandEventData() → part.12 Implement nsCommandEvent::AssignCommandEventData()
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #798478 -
Flags: review?(bugs)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #798480 -
Flags: review?(bugs)
Assignee | ||
Comment 18•11 years ago
|
||
Attachment #798481 -
Flags: review?(bugs)
Assignee | ||
Comment 19•11 years ago
|
||
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)
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #798483 -
Flags: review?(bugs)
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #798484 -
Flags: review?(bugs)
Assignee | ||
Comment 22•11 years ago
|
||
Attachment #798485 -
Flags: review?(bugs)
Assignee | ||
Comment 23•11 years ago
|
||
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.
Comment 24•11 years ago
|
||
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.
Assignee | ||
Comment 25•11 years ago
|
||
Sure.
Updated•11 years ago
|
Attachment #798466 -
Flags: review?(bugs) → review+
Comment 26•11 years ago
|
||
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•11 years ago
|
Attachment #798468 -
Flags: review?(bugs) → review+
Comment 27•11 years ago
|
||
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•11 years ago
|
Attachment #798470 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Attachment #798472 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Attachment #798473 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Attachment #798475 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Attachment #798476 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Attachment #798477 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Attachment #798478 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Attachment #798480 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Attachment #798481 -
Flags: review?(bugs) → review+
Comment 28•11 years ago
|
||
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•11 years ago
|
Attachment #798483 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Attachment #798484 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Attachment #798485 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 29•11 years ago
|
||
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 30•11 years ago
|
||
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+
Assignee | ||
Comment 31•11 years ago
|
||
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]
Assignee | ||
Comment 32•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/936da6b82904 https://hg.mozilla.org/integration/mozilla-inbound/rev/63e1c8695f29 https://hg.mozilla.org/integration/mozilla-inbound/rev/9e6f0a173b4c https://hg.mozilla.org/integration/mozilla-inbound/rev/e05c3cbf3abf https://hg.mozilla.org/integration/mozilla-inbound/rev/1051695a0baf https://hg.mozilla.org/integration/mozilla-inbound/rev/3e0016fca7a4 https://hg.mozilla.org/integration/mozilla-inbound/rev/b9a2fad74c51 https://hg.mozilla.org/integration/mozilla-inbound/rev/29721efb7fce https://hg.mozilla.org/integration/mozilla-inbound/rev/a458752f59d0 https://hg.mozilla.org/integration/mozilla-inbound/rev/ae9234f3529f https://hg.mozilla.org/integration/mozilla-inbound/rev/0dd47ac83aca https://hg.mozilla.org/integration/mozilla-inbound/rev/993842a0d8c0 https://hg.mozilla.org/integration/mozilla-inbound/rev/d7aa67d15177 https://hg.mozilla.org/integration/mozilla-inbound/rev/86c9e84ebfcb https://hg.mozilla.org/integration/mozilla-inbound/rev/1eebd1fcabe3 https://hg.mozilla.org/integration/mozilla-inbound/rev/651abc2bbc02 https://hg.mozilla.org/integration/mozilla-inbound/rev/3650a56de516 https://hg.mozilla.org/integration/mozilla-inbound/rev/c88fb7eb6390 https://hg.mozilla.org/integration/mozilla-inbound/rev/2cba31116b16 https://hg.mozilla.org/integration/mozilla-inbound/rev/b2c06efbc145
Comment 33•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/936da6b82904 https://hg.mozilla.org/mozilla-central/rev/63e1c8695f29 https://hg.mozilla.org/mozilla-central/rev/9e6f0a173b4c https://hg.mozilla.org/mozilla-central/rev/e05c3cbf3abf https://hg.mozilla.org/mozilla-central/rev/1051695a0baf https://hg.mozilla.org/mozilla-central/rev/3e0016fca7a4 https://hg.mozilla.org/mozilla-central/rev/b9a2fad74c51 https://hg.mozilla.org/mozilla-central/rev/29721efb7fce https://hg.mozilla.org/mozilla-central/rev/a458752f59d0 https://hg.mozilla.org/mozilla-central/rev/ae9234f3529f https://hg.mozilla.org/mozilla-central/rev/0dd47ac83aca https://hg.mozilla.org/mozilla-central/rev/993842a0d8c0 https://hg.mozilla.org/mozilla-central/rev/d7aa67d15177 https://hg.mozilla.org/mozilla-central/rev/86c9e84ebfcb https://hg.mozilla.org/mozilla-central/rev/1eebd1fcabe3 https://hg.mozilla.org/mozilla-central/rev/651abc2bbc02 https://hg.mozilla.org/mozilla-central/rev/3650a56de516 https://hg.mozilla.org/mozilla-central/rev/c88fb7eb6390 https://hg.mozilla.org/mozilla-central/rev/2cba31116b16 https://hg.mozilla.org/mozilla-central/rev/b2c06efbc145
Flags: in-testsuite+
Assignee | ||
Comment 34•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [leave open]
Comment 35•11 years ago
|
||
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+
Comment 36•11 years ago
|
||
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.
Description
•