Closed Bug 1449631 Opened 7 years ago Closed 7 years ago

Empty out nsIDOMEventTarget

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(14 files, 1 obsolete file)

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.
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+
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
[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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: