Closed Bug 915757 Opened 6 years ago Closed 6 years ago

Split GetListenerManager(bool) into two functions

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox26 --- affected

People

(Reporter: khuey, Assigned: khuey)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
This gets rid of an ugly boolean parameter and lets us be more const-correct.

The XPIDL hackery is a little ugly though.
Attachment #803819 - Flags: review?(bugs)
Attachment #803819 - Flags: review?(bugs) → review+
Blocks a blocker.
blocking-b2g: --- → koi+
Attached patch Patch v2 (obsolete) — Splinter Review
I had to change this around because things inherit from nsIDOMEventTarget, so mucking up the vtable breaks B2G.
Attachment #810462 - Flags: review?(bugs)
Hmm, we have a tiny problem. ListenerManager() on outer window can return null.
We probably should just create listener manager for outer window in that case. A warning wouldn't harm.
mTarget of that ELM could be null (which means you'd need to remove an assert from ELM ctor) and
perhaps null check mTarget in nsEventListenerManager::HandleEventInternal.
And I don't understand why ListenerManager() and GetExistingListenerManager() can't be in
nsIDOMEventTarget. What breaks in B2G, and why won't they break with this new setup?
Attachment #810462 - Flags: review?(bugs) → review-
What breaks is that the C++ vtable and what xpconnect _thinks_ that vtable is disagree, for any interface that inherits from nsIDOMEventTarget.

And that happens because of virtual methods in a {C++} block, which xpconnect hence doesn't know about.  And they're in a C++ block because XPIDL has no way to declare a const method.

The only reason B2G is involved is that's the only place we still ship interfaces that inherit from nsIDOMEventTarget whose implementation is not on WebIDL bindings.
Well, then we should drop the const, and keep ListenerManager() and GetExistingListenerManager() in nsIDOMEventTarget, IMO.
Or is there some particular reason why we need const?
We want it to avoid needing const_cast various places, no?
The patch removes 2 const_cast. That is not very significant.
(I would like to see EventTarget and nsIDOMEventTarget to merge eventually, so push methods higher up in the inheritance tree.)
> I would like to see EventTarget and nsIDOMEventTarget to merge eventually

I think eventually we should kill off nsIDOMEventTarget...
The point of this (to me) is to add const to GetListenerManager(false) so that memory reporting code (which I'm adding more of) doesn't need to do const_cast.  If we don't fix that I have no use for this patch.
I don't care too much about whether the new methods live on nsIDOM* or *.
But please fix nsGlobalWindow.
Attached patch Patch v3Splinter Review
As we discussed.
Attachment #803819 - Attachment is obsolete: true
Attachment #810462 - Attachment is obsolete: true
Attachment #813361 - Flags: review?(bugs)
Comment on attachment 813361 [details] [diff] [review]
Patch v3

>+nsContentUtils::ListenerManagerForNode(nsINode *aNode)
should be
nsContentUtils::GetListenerManagerForNode
Attachment #813361 - Flags: review?(bugs) → review+
blocking-b2g: koi+ → ---
https://hg.mozilla.org/mozilla-central/rev/04610078280c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Backed out for frequent Android Armv6 mochitest-8 crashes caused by either this or bug 919885.
https://hg.mozilla.org/mozilla-central/rev/30afbcdcec4d

https://tbpl.mozilla.org/php/getParsedLog.php?id=29092370&tree=Mozilla-Central
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla27 → ---
Retriggers are green post-backout.
https://hg.mozilla.org/mozilla-central/rev/c9879b47c32c
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Comment on attachment 823221 [details] [diff] [review]
Aurora Patch

Memshrink needs this for bug 904720.
Attachment #823221 - Flags: approval-mozilla-b2g18+
Comment on attachment 823221 [details] [diff] [review]
Aurora Patch

Bah, I'm an idiot.

Anyways, approving for 1.2.
Attachment #823221 - Flags: approval-mozilla-b2g18+ → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.