Closed
Bug 1449631
Opened 6 years ago
Closed 6 years ago
Empty out nsIDOMEventTarget
Categories
(Core :: DOM: Core & HTML, enhancement, P3)
Core
DOM: Core & HTML
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.
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•6 years ago
|
||
MozReview-Commit-ID: CCHCZjMgInu
Attachment #8963912 -
Flags: review?(bugs)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•6 years ago
|
||
MozReview-Commit-ID: 4Y4YGeqTP3z
Attachment #8963913 -
Flags: review?(bugs)
Assignee | ||
Comment 3•6 years ago
|
||
MozReview-Commit-ID: F67Od8surQ8
Attachment #8963914 -
Flags: review?(bugs)
Assignee | ||
Comment 4•6 years ago
|
||
MozReview-Commit-ID: ID0FDvp28HY
Attachment #8963915 -
Flags: review?(bugs)
Assignee | ||
Comment 5•6 years ago
|
||
MozReview-Commit-ID: K2nOZNPy0XE
Attachment #8963916 -
Flags: review?(bugs)
Assignee | ||
Comment 6•6 years ago
|
||
MozReview-Commit-ID: 8YMgmMwZkAL
Attachment #8963917 -
Flags: review?(bugs)
Assignee | ||
Comment 7•6 years ago
|
||
MozReview-Commit-ID: AIzDo67mTDf
Attachment #8963918 -
Flags: review?(bugs)
Assignee | ||
Comment 8•6 years ago
|
||
MozReview-Commit-ID: 5wQ2LYrjUxf
Attachment #8963919 -
Flags: review?(bugs)
Assignee | ||
Comment 9•6 years ago
|
||
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)
Assignee | ||
Comment 10•6 years ago
|
||
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)
Assignee | ||
Comment 11•6 years ago
|
||
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)
Assignee | ||
Comment 12•6 years ago
|
||
Attachment #8963923 -
Flags: review?(bugs)
Comment 13•6 years ago
|
||
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 14•6 years ago
|
||
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+
Assignee | ||
Comment 15•6 years ago
|
||
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.
Updated•6 years ago
|
Attachment #8963914 -
Flags: review?(bugs) → review+
Comment 16•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8963916 -
Flags: review?(bugs) → review+
Updated•6 years ago
|
Attachment #8963918 -
Flags: review?(bugs) → review+
Updated•6 years ago
|
Attachment #8963919 -
Flags: review?(bugs) → review+
Updated•6 years ago
|
Attachment #8963923 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 17•6 years ago
|
||
> 2 extra spaces before let Fixed. > missing space before mEventListener. Tabs, actually. Silly files without modelines... Replaced with spaces.
Assignee | ||
Comment 18•6 years ago
|
||
MozReview-Commit-ID: 6teO2KoGqUo
Attachment #8964097 -
Flags: review?(bugs)
Comment 19•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8964097 -
Flags: review?(bugs) → review+
Comment 20•6 years ago
|
||
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+
Assignee | ||
Comment 21•6 years ago
|
||
> tabs
Fixed, throughout that file.
Comment 22•6 years ago
|
||
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-
Updated•6 years ago
|
Attachment #8963922 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 23•6 years ago
|
||
Attachment #8965303 -
Flags: review?(bugs)
Assignee | ||
Updated•6 years ago
|
Attachment #8963921 -
Attachment is obsolete: true
Assignee | ||
Comment 24•6 years ago
|
||
And part 10 no longer has any MediaQueryList changes, but the interdiff can't really capture that.
Updated•6 years ago
|
Attachment #8965303 -
Flags: review?(bugs) → review+
Comment 25•6 years 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 26•6 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 27•6 years 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)
Comment 28•6 years ago
|
||
(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)
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•