Empty out nsIDOMEventTarget

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(14 attachments, 1 obsolete attachment)

15.12 KB, patch
smaug
: review+
Details | Diff | Splinter Review
38.07 KB, patch
smaug
: review+
Details | Diff | Splinter Review
12.81 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.82 KB, patch
smaug
: review+
Details | Diff | Splinter Review
14.48 KB, patch
smaug
: review+
Details | Diff | Splinter Review
107.86 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.30 KB, patch
smaug
: review+
Details | Diff | Splinter Review
87.72 KB, patch
smaug
: review+
Details | Diff | Splinter Review
40.17 KB, patch
smaug
: review+
Details | Diff | Splinter Review
18.79 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.84 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.62 KB, patch
smaug
: review+
Details | Diff | Splinter Review
30.84 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.24 KB, patch
Details | Diff | Splinter Review
Plan is to move the actual functionality to EventTarget.

Updated

a year ago
Priority: -- → P3
MozReview-Commit-ID: CCHCZjMgInu
Attachment #8963912 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
MozReview-Commit-ID: 4Y4YGeqTP3z
Attachment #8963913 - Flags: review?(bugs)
MozReview-Commit-ID: F67Od8surQ8
Attachment #8963914 - Flags: review?(bugs)
MozReview-Commit-ID: ID0FDvp28HY
Attachment #8963915 - Flags: review?(bugs)
MozReview-Commit-ID: K2nOZNPy0XE
Attachment #8963916 - Flags: review?(bugs)
MozReview-Commit-ID: 8YMgmMwZkAL
Attachment #8963917 - Flags: review?(bugs)
MozReview-Commit-ID: AIzDo67mTDf
Attachment #8963918 - Flags: review?(bugs)
MozReview-Commit-ID: 5wQ2LYrjUxf
Attachment #8963919 - Flags: review?(bugs)
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
Attachment #8963920 - Flags: review?(bugs)
The CanCallerAccess check in the "webidl" version of
nsGlobalWindowOuter::AddEventListener was pointless, because bindings never
call things on outer windows.
Attachment #8963921 - Flags: review?(bugs)
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!
Attachment #8963922 - Flags: review?(bugs)
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
Attachment #8963912 - Flags: review?(bugs) → review+
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.
Attachment #8963913 - Flags: review?(bugs) → review+
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.
Attachment #8963914 - Flags: review?(bugs) → review+
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
Attachment #8963915 - Flags: review?(bugs) → review+
Attachment #8963916 - Flags: review?(bugs) → review+
Attachment #8963918 - Flags: review?(bugs) → review+
Attachment #8963919 - Flags: review?(bugs) → review+
Attachment #8963923 - Flags: review?(bugs) → review+
> 2 extra spaces before let

Fixed.

> missing space before mEventListener.

Tabs, actually.  Silly files without modelines...  Replaced with spaces.
MozReview-Commit-ID: 6teO2KoGqUo
Attachment #8964097 - Flags: review?(bugs)
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.
Attachment #8963917 - Flags: review?(bugs) → review+
Attachment #8964097 - Flags: review?(bugs) → review+
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
Attachment #8963920 - Flags: review?(bugs) → review+
> tabs

Fixed, throughout that file.
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.
Attachment #8963921 - Flags: review?(bugs) → review-
Attachment #8963922 - Flags: review?(bugs) → review+
Depends on: 1451199
Attachment #8963921 - Attachment is obsolete: true
And part 10 no longer has any MediaQueryList changes, but the interdiff can't really capture that.
Attachment #8965303 - Flags: review?(bugs) → review+

Comment 25

a year ago
Pushed by bzbarsky@mozilla.com:
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 27

a year ago
[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 :-(
Flags: needinfo?(bzbarsky)
(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!
Flags: needinfo?(bzbarsky)
Depends on: 1451966
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.