Consider changing nsIDOM*Event interfaces so that they don't inherit nsIDOMEvent

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: smaug, Assigned: aidin)

Tracking

36 Branch
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

4 years ago
Bug 1230092 shows an issue the setup causes.
Internally we should mostly use Event and classes inheriting it, and external code
can always QI to the right interface.
Assignee

Comment 1

4 years ago
I changed the interfaces and inherit them from `nsISupports', except for nsIDOMUIEvent (along with the ones inherited from it, directly or indirectly.) Some of the classes were used methods like `GetInternalNSEvent' which is defined in nsIDOMEvent.

Is that OK to not changing nsIDOMUIEvent and its inheritors?
Assignee: nobody → aidin
Assignee

Updated

4 years ago
Flags: needinfo?(bugs)
Reporter

Comment 2

4 years ago
You can do this in few separate patches, but I think all the interfaces should be converted - or none.
Do you have examples where use of GetInternalNSEvent caused troubles?

As a temporary solution we could even do something like (didn't check the exact .idl syntax)

%{C++
namespace mozilla {
namespace dom {
class Event;
}
}
%}
[ptr] native EventPtr(mozilla::dom::Event);


interface nsIDOMUIEvent : nsISupports
{
...
  [notxpcom, nostdcall] EventPtr AsEvent();
};


which should then in C++ be 
mozilla::dom::Event* AsEvent();


and could be implemented as 
virtual nsIEvent* AsEvent()
{
  return this;
}


And then whatever code is using GetInternalNSEvent() could just do
uiEvent->AsEvent()->GetInternalNSEvent() ...
Reporter

Updated

4 years ago
Flags: needinfo?(bugs)
Assignee

Updated

4 years ago
Attachment #8702320 - Attachment description: MozReview Request: Bug 1230216 - Changing nsIDOM*Event interfaces so that they don't inherit nsIDOMEvent. → MozReview Request: Bug 1230216 - Changing nsIDOM*Event interfaces so that they don't inherit nsIDOMEvent. r?smaug
Attachment #8702320 - Flags: review?(bugs)
Assignee

Comment 4

4 years ago
Comment on attachment 8702320 [details]
MozReview Request: Bug 1230216 - Changing nsIDOM*Event interfaces so that they don't inherit nsIDOMEvent. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29105/diff/1-2/
Assignee

Comment 5

4 years ago
Sorry, on the first push, I forgot to add the `r?smaug' at the end of the commit message.

Thanks for the solution. The syntax was correct (:

Here's a build on tryserver:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6aba614a066f
Reporter

Comment 6

4 years ago
+//  if (MOZ_LOG_TEST(sISMLog, LogLevel::Info)) {
+//    nsAutoString eventType;
+//    aMouseEvent->GetType(eventType);
+//    MOZ_LOG(sISMLog, LogLevel::Info,
+//      ("ISM:   IMEStateManager::OnMouseButtonEventInEditor(), "
+//       "mouse event (type=%s, button=%d) is %s",
+//       NS_ConvertUTF16toUTF8(eventType).get(), internalEvent->button,
+//       consumed ? "consumed" : "not consumed"));
+//  }
Please don't uncomment that code.

 
 
+
 protected:
   ~UIEvent() {}
Extra newline before 'protected'


 #define SET_MODIFIER(aName, aValue) \
   if (aParam.m##aName) { \
     inputEvent->modifiers |= aValue; \
-  } \
+  }
unrelated change

you need to update uuid of all the .idl interfaces you're changing.
'mach' should have some helper command for that.


+%{C++
+  namespace mozilla {
+    namespace dom {
+      class Event;
+      }
+    }
+%}
namespaces don't need indentation in Gecko.

+
+  [notxpcom, nostdcall] EventPtr AsEvent(); 
extra space after ;

-      WidgetGUIEvent* GUIEvent = aKeyEvent->GetInternalNSEvent()->AsGUIEvent();
+      WidgetGUIEvent* GUIEvent = aKeyEvent->AsEvent()->
+          GetInternalNSEvent()->AsGUIEvent();
use 2 spaces for indentation
Reporter

Updated

4 years ago
Attachment #8702320 - Flags: review?(bugs) → review-
Assignee

Comment 7

4 years ago
Comment on attachment 8702320 [details]
MozReview Request: Bug 1230216 - Changing nsIDOM*Event interfaces so that they don't inherit nsIDOMEvent. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29105/diff/2-3/
Attachment #8702320 - Flags: review- → review?(bugs)
Assignee

Comment 8

4 years ago
Thanks for the meticulous review. (:

Here's the tryserver build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba6f43c0a9b8
Reporter

Comment 9

4 years ago
Comment on attachment 8702320 [details]
MozReview Request: Bug 1230216 - Changing nsIDOM*Event interfaces so that they don't inherit nsIDOMEvent. r?smaug

Thanks!


Btw, if you want to do another small cleanup, in a different bug, renaming method
GetInternalNSEvent() to WidgetEvent() would be great.
Attachment #8702320 - Flags: review?(bugs) → review+
Assignee

Comment 10

4 years ago
I will do that. I filed Bug 1235830.
Assignee

Updated

4 years ago
Keywords: checkin-needed
this failed to apply:

patching file dom/plugins/base/nsPluginInstanceOwner.cpp
Hunk #1 FAILED at 49
1 out of 1 hunks FAILED -- saving rejects to file dom/plugins/base/nsPluginInstanceOwner.cpp.rej
patching file layout/xul/nsXULPopupManager.cpp
Hunk #3 succeeded at 2267 with fuzz 2 (offset 1 lines).
patch failed to apply
abort: fix up the merge and run hg transplant --continue
Flags: needinfo?(aidin)
Keywords: checkin-needed
Assignee

Comment 12

4 years ago
Comment on attachment 8702320 [details]
MozReview Request: Bug 1230216 - Changing nsIDOM*Event interfaces so that they don't inherit nsIDOMEvent. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29105/diff/3-4/
Attachment #8702320 - Flags: review+ → review?(bugs)
Reporter

Updated

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

Comment 13

4 years ago
Comment on attachment 8702320 [details]
MozReview Request: Bug 1230216 - Changing nsIDOM*Event interfaces so that they don't inherit nsIDOMEvent. r?smaug

https://reviewboard.mozilla.org/r/29105/#review27287

This kind of whitespace changes only patch wouldn't need a new review, but I don't know whether mozreview deals with this case properly.
Assignee

Comment 14

4 years ago
I don't know either (:
Thanks.
Flags: needinfo?(aidin)
Keywords: checkin-needed

Comment 16

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d83462d4d148
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46

Comment 17

3 years ago
https://hg.mozilla.org/comm-central/rev/0d6b1be6ae48bfc52d57de25ed8a9277b461464c
Fix mintrayr following nsIDOM*Event interface changes in bug 1230216. r=clokep over IRC rs=bustage-fix

Comment 18

3 years ago
https://hg.mozilla.org/comm-central/rev/038cb3717ea14fc785002da2df6b0a341ebfc6f7
Fix mintrayr following nsIDOM*Event interface changes in bug 1230216, v2. rs=bustage-fix
You need to log in before you can comment on or make changes to this bug.