1449631 - Empty out nsIDOMEventTarget

Status: RESOLVED FIXED

Description

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Plan is to move the actual functionality to EventTarget.

Comment 1

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 1. Remove JS uses of nsIDOMEventTarget

MozReview-Commit-ID: CCHCZjMgInu

Comment 2

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 2. Remove nsIDOMEventTarget::RemoveEventListener

MozReview-Commit-ID: 4Y4YGeqTP3z

Comment 3

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 3. Remove nsIDOMEventTarget::RemoveSystemEventListener

MozReview-Commit-ID: F67Od8surQ8

Comment 4

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 4. Remove nsIDOMEventTarget::GetContextForEventHandlers

MozReview-Commit-ID: ID0FDvp28HY

Comment 5

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 5. Move Will/Pre/PostHandleEvent out of nsIDOMEventTarget

MozReview-Commit-ID: K2nOZNPy0XE

Comment 6

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 6. Remove nsIDOMEventTarget::DispatchEvent

MozReview-Commit-ID: 8YMgmMwZkAL

Comment 7

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 7. Remove nsIDOMEventTarget::GetTargetFor* methods

MozReview-Commit-ID: AIzDo67mTDf

Comment 8

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 8. Remove nsIDOMEventTarget::GetEventTargetParent

MozReview-Commit-ID: 5wQ2LYrjUxf

Comment 9

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 9. Remove nsIDOMEventTarget::AddEventListener

Also switch the XPCOM-y version of EventTarget::AddEventListner to a
Nullable<bool> for aWantsUntrusted.

The three-arg overload of AddEventListener in ContentFrameMessageManager was
never called, so all the AddEventListener overloads there are not needed.

MozReview-Commit-ID: 4IhqHmPVWzE

Comment 10

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 10. Devirtualize AddEventListener

The CanCallerAccess check in the "webidl" version of
nsGlobalWindowOuter::AddEventListener was pointless, because bindings never
call things on outer windows.

Comment 11

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 11. Remove nsIDOMEventTarget::AddSystemEventListener

The security checks outer window did here don't seem right, because the whole
point is that this method is only called by C++ code for its own purposes.
We're not adding random untrusted listeners via addSystemEventListener!

Comment 12

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 12. Remove the Nullable smuggling from nsIDOMEventTarget

Comment 13

[Olli Pettay [:smaug]]

Comment on attachment 8963912 [details] [diff] [review]
part 1.  Remove JS uses of nsIDOMEventTarget

>   observe(aSubject, aTopic, aData) {
>     if (aTopic != "domwindowopened") {
>       return;
>     }
> 
>-    let win = aSubject.QueryInterface(Ci.nsIDOMEventTarget);
>+      let win = aSubject;
2 extra spaces before let

Comment 14

[Olli Pettay [:smaug]]

Comment on attachment 8963913 [details] [diff] [review]
part 2.  Remove nsIDOMEventTarget::RemoveEventListener

+  if (aElement) {
+    aElement->RemoveEventListener(NS_LITERAL_STRING("click"),
+				  mEventListener, true);
   }
missing space before mEventListener.


The removed null check in ScrollbarActivity::StopListeningForScrollAreaEvents() looks fine.

Comment 15

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Sorry, I should have noted that in the commit message. The only frame without content is the viewport, so the null-check there was pointless all along.

Comment 16

[Olli Pettay [:smaug]]

Comment on attachment 8963915 [details] [diff] [review]
part 4.  Remove nsIDOMEventTarget::GetContextForEventHandlers

This is a bit trickier, since GetContextForEventHandlers throws after ContentViewer has been closed.
But DataTransferItem::Data looks ok to me

Comment 17

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> 2 extra spaces before let

Fixed.

> missing space before mEventListener.

Tabs, actually.  Silly files without modelines...  Replaced with spaces.

Comment 18

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 13. Remove now-unnecessary forwarding macros

MozReview-Commit-ID: 6teO2KoGqUo

Comment 19

[Olli Pettay [:smaug]]

Comment on attachment 8963917 [details] [diff] [review]
part 6.  Remove nsIDOMEventTarget::DispatchEvent

Passing CallerType::System from native code is a tad annoying, but I guess it is fine.

Comment 20

[Olli Pettay [:smaug]]

Comment on attachment 8963920 [details] [diff] [review]
part 9.  Remove nsIDOMEventTarget::AddEventListener

 MediaDevices::AddEventListener(const nsAString& aType,
-  nsIDOMEventListener* aListener,
-  bool aUseCapture, bool aWantsUntrusted,
-  uint8_t optional_argc)
+			       nsIDOMEventListener* aListener,
+			       bool aUseCapture,
+			       const Nullable<bool>& aWantsUntrusted)
tabs

+  return DOMEventTargetHelper::AddEventListener(aType, aListener,
+						aUseCapture,
+						aWantsUntrusted);

tabs

+  return DOMEventTargetHelper::AddEventListener(aType, aListener,
+						aOptions,
+						aWantsUntrusted,
+						aRv);
ditto

Comment 21

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> tabs

Fixed, throughout that file.

Comment 22

[Olli Pettay [:smaug]]

Comment on attachment 8963921 [details] [diff] [review]
part 10.  Devirtualize AddEventListener

ok, MediaQueryList stuff is ugly, and as far as I see even broken - I don't see how RecomputeMatches(); gets called when setting onchange event handler. Or am I missing something?
Could you file a followup if needed, make it block bug 1354441

Given that ComputeDefaultWantsUntrusted has rather different meaning in this patch than what its name hints, could we call it WillAddEventListener? Though, I wonder how we'll want to handle MediaQueryList's onchange case.
Perhaps it is better to fix MediaQueryList first. So, r- for now.

Comment 23

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Part 10 merged on top of fixes to MediaQueryList

Comment 24

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Interdiff for part 10 showing what changed

And part 10 no longer has any MediaQueryList changes, but the interdiff can't really capture that.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9e38b63ba4
part 1.  Remove JS uses of nsIDOMEventTarget.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/34f06ffa0c0e
part 2.  Remove nsIDOMEventTarget::RemoveEventListener.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/059872bf21be
part 3.  Remove nsIDOMEventTarget::RemoveSystemEventListener.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f824c936c25
part 4.  Remove nsIDOMEventTarget::GetContextForEventHandlers.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d05e92371389
part 5.  Move Will/Pre/PostHandleEvent out of nsIDOMEventTarget.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/caa5bef68481
part 6.  Remove nsIDOMEventTarget::DispatchEvent.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/433dba4e7104
part 7.  Remove nsIDOMEventTarget::GetTargetFor* methods.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/15546aaa4d09
part 8.  Remove nsIDOMEventTarget::GetEventTargetParent.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/51fd57c68777
part 9.  Remove nsIDOMEventTarget::AddEventListener.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a73566a5d08e
part 10.  Devirtualize AddEventListener.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf7b2b773db
part 11.  Remove nsIDOMEventTarget::AddSystemEventListener.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/27805d201b90
part 12.  Remove the Nullable smuggling from nsIDOMEventTarget.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d32c2aed2886
part 13.  Remove now-unnecessary forwarding macros.  r=smaug

Comment 26

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/0e9e38b63ba4
https://hg.mozilla.org/mozilla-central/rev/34f06ffa0c0e
https://hg.mozilla.org/mozilla-central/rev/059872bf21be
https://hg.mozilla.org/mozilla-central/rev/1f824c936c25
https://hg.mozilla.org/mozilla-central/rev/d05e92371389
https://hg.mozilla.org/mozilla-central/rev/caa5bef68481
https://hg.mozilla.org/mozilla-central/rev/433dba4e7104
https://hg.mozilla.org/mozilla-central/rev/15546aaa4d09
https://hg.mozilla.org/mozilla-central/rev/51fd57c68777
https://hg.mozilla.org/mozilla-central/rev/a73566a5d08e
https://hg.mozilla.org/mozilla-central/rev/6cf7b2b773db
https://hg.mozilla.org/mozilla-central/rev/27805d201b90
https://hg.mozilla.org/mozilla-central/rev/d32c2aed2886

Comment 27

[Jorg K (GMT+2)]

[2784, Main Thread] WARNING: Fix the caller!: file c:/mozilla-source/comm-central/mozilla/dom/events/EventDispatcher.cpp, line 753

has become a very spammy message in TB :-(

Comment 28

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jorg K (GMT+1) from comment #27)
> [2784, Main Thread] WARNING: Fix the caller!: file
> c:/mozilla-source/comm-central/mozilla/dom/events/EventDispatcher.cpp, line
> 753
> 
> has become a very spammy message in TB :-(

Also on central, I filed bug 1452471 for that. Sorry I didn't see this earlier!