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