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

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 1

[Aidin Gharibnavaz]

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?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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() ...

Comment 3

[Aidin Gharibnavaz]

Attachment: MozReview Request: Bug 1230216 - Changing nsIDOM*Event interfaces so that they don't inherit nsIDOMEvent. r?smaug

Review commit: https://reviewboard.mozilla.org/r/29105/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29105/

Comment 4

[Aidin Gharibnavaz]

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/

Comment 5

[Aidin Gharibnavaz]

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

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

+//  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

Comment 7

[Aidin Gharibnavaz]

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/

Comment 8

[Aidin Gharibnavaz]

Thanks for the meticulous review. (:

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

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 10

[Aidin Gharibnavaz]

I will do that. I filed Bug 1235830.

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Aidin Gharibnavaz]

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/

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 14

[Aidin Gharibnavaz]

I don't know either (:
Thanks.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d83462d4d148

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d83462d4d148

Comment 17

[aleth [:aleth]]

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

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/038cb3717ea14fc785002da2df6b0a341ebfc6f7
Fix mintrayr following nsIDOM*Event interface changes in bug 1230216, v2. rs=bustage-fix