Closed
Bug 835643
Opened 12 years ago
Closed 12 years ago
Set up the infrastructure to switch EventListener to WebIDL codegen
Categories
(Core :: DOM: Events, defect)
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Depends on 1 open bug)
Details
Attachments
(9 files)
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dougt
:
review+
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
Now that bug 822470 has landed.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #712286 -
Flags: review?(bugs)
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → bzbarsky
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #712287 -
Flags: review?(bugs)
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #712288 -
Flags: review?(bugs)
Assignee | ||
Comment 4•12 years ago
|
||
I'll push this to try when I'm on a network that douesn't block port 22, but local testing looks promising. Olli, are there any performance tests worth running here?
Attachment #712289 -
Flags: review?(bugs)
Comment 5•12 years ago
|
||
Comment on attachment 712287 [details] [diff] [review]
part 2. Store an EventListenerHolder, not an nsIDOMEventListener in nsListenerStruct.
Review of attachment 712287 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/events/src/nsEventListenerManager.cpp
@@ +924,5 @@
> // nsIDOMEvent::currentTarget is set in nsEventDispatcher.
> + if (aListener.HasWebIDLCallback()) {
> + ErrorResult rv;
> + aListener.GetWebIDLCallback()->HandleEvent(aCurrentTarget, aDOMEvent,
> + rv);
Nit: indentation
Assignee | ||
Comment 6•12 years ago
|
||
> Nit: indentation
Fixed.
Assignee | ||
Comment 7•12 years ago
|
||
I'm seeing various test failures here. Still debugging some of them but at the very least:
1) I think we have tests that depend on bug 503244. They now end up with actual
exceptions reported and fail. I have to go dig through them some more to be sure.
2) We definitely have tests that depend on bug 822213 (e.g.
editor/composer/test/test_bug519928.html ). I guess I should just go and fix
bug 765780 before I check this in... :(
Comment 8•12 years ago
|
||
http://mozilla.pettay.fi/moztests/events/event_speed_3.html is a totally artificial test.
If you want to emphasize callback handling, decrease the dom tree depth (default 100).
Comment 9•12 years ago
|
||
Comment on attachment 712286 [details] [diff] [review]
part 1. Give CallbackObject an IID so that random things don't QI to it. now it thinks its IID is the nsISupports IID!
I assume this is actually used in some other patch.
Attachment #712286 -
Flags: review?(bugs) → review+
Comment 10•12 years ago
|
||
Comment on attachment 712287 [details] [diff] [review]
part 2. Store an EventListenerHolder, not an nsIDOMEventListener in nsListenerStruct.
Hmm, so we end up doing double cx push/pop?
One in nsEventListenerManager::HandleEventInternal and one in webidl callback.
That is less than optimal.
Could we perhaps pass a flag to WebIDL callback to bypass cx pushing?
Attachment #712287 -
Flags: review?(bugs) → review-
Comment 11•12 years ago
|
||
Comment on attachment 712287 [details] [diff] [review]
part 2. Store an EventListenerHolder, not an nsIDOMEventListener in nsListenerStruct.
Or am I wrong... reading the generated code.
Attachment #712287 -
Flags: review- → review?(bugs)
Comment 12•12 years ago
|
||
Comment on attachment 712287 [details] [diff] [review]
part 2. Store an EventListenerHolder, not an nsIDOMEventListener in nsListenerStruct.
Ah, yes, the normal Callback setup is there.
Attachment #712287 -
Flags: review?(bugs) → review-
Updated•12 years ago
|
Attachment #712288 -
Flags: review?(bugs) → review+
Comment 13•12 years ago
|
||
Comment on attachment 712289 [details] [diff] [review]
part 4. Switch EventListener to WebIDL codegen.
But did you say some tests are still failing?
Attachment #712289 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 14•12 years ago
|
||
> I assume this is actually used in some other patch.
It's not directly, but it keeps some code that I wrote by accident that was wrong from compiling, iirc.
> Hmm, so we end up doing double cx push/pop?
Hmm. Yes. We really need to kill the JSContext stack. :(
> Could we perhaps pass a flag to WebIDL callback to bypass cx pushing?
We could, but it might have a different cx... I suppose we could pass in a cx to run on to indicate that no push/pop should happen, but it's not immediately obvious to me whether that gives the right behavior in all cases.
I need to think about this.
> But did you say some tests are still failing?
Yep. See https://tbpl.mozilla.org/?tree=Try&rev=3169a0c5d034
Still sorting through the failures, but see comment 7.
Assignee | ||
Comment 15•12 years ago
|
||
OK, the M2 failures seem to be due to buggy tests that depend on bug 503244. I mailed the test authors to find out what the deal with those is.
The M4 failure that's due to these patches is because the test depends on bug 822213.
The webapps test failures in Moth are because those tests depend on bug 503244.
Still looking into the rest of the Moth failures and the bc failure.
Assignee | ||
Comment 16•12 years ago
|
||
> OK, the M2 failures seem to be due to buggy tests that depend on bug 503244.
Well, some of them are. Looking into the others now.
A number of the other Moth failures seem to be tests that are depending on bug 503244. I should fix all those and then see where we stand...
Assignee | ||
Comment 17•12 years ago
|
||
So the remaining browser-chrome orange is due to the test closing a window in the middle of some XBL JS running on that window. This used to not happen because we invoked the event handler on the cx of the event target (which is in that window), but now we do it on the cx of the handler (which is a different window). I can fix this in the test by just doing the close async, but need to think a tad about what the right approach here is.
Assignee | ||
Comment 18•12 years ago
|
||
So at this point the one orange I don't have a planned fix for is this:
13649 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/test_sanityException2.xul | expectUncaughtException was called but no uncaught exception was detected!
which triggers some other knock-on failures. Problem is, I can't reproduce it locally by just running that one test. :(
I _can_ "fix" it by running callbacks on the context of the object they're attached to (so for event listeners the context of the event target) instead of the context of the function object...
Assignee | ||
Comment 19•12 years ago
|
||
Attachment #713902 -
Flags: review?(doug.turner)
Attachment #713902 -
Flags: review?(bugs)
Comment 20•12 years ago
|
||
Comment on attachment 713902 [details] [diff] [review]
Addendum to part 4 to fix missed overriders of add/removeEventListener
Tiny bit copy-pasting in DeviceStorage code, but I guess it is not too bad.
Attachment #713902 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 21•12 years ago
|
||
Yeah. I considered combining them; I'd have to expose versions of add/remove that take an EventListenerHolder in various places. Can be done, if desired, but the end result is just as much code, I think.
Assignee | ||
Comment 22•12 years ago
|
||
Looks like the sanityException2 issue was the same as comment 17, but triggered by an earlier test in the suite (js/xpconnect/tests/chrome/test_bug799348.xul to be precise). I'm hoping bug 841312 will just make all those problems go away.
Comment 23•12 years ago
|
||
Comment on attachment 713902 [details] [diff] [review]
Addendum to part 4 to fix missed overriders of add/removeEventListener
Fix up whitespace:
+ bool aUseCapture,
+ const Nullable<bool>& aWantsUntrusted,
+ ErrorResult& aRv)
Attachment #713902 -
Flags: review?(doug.turner) → review+
Assignee | ||
Comment 24•12 years ago
|
||
> Fix up whitespace:
Done, and added modelines to keep the tabs out.
Assignee | ||
Comment 25•12 years ago
|
||
Attachment #728321 -
Flags: review?(bugs)
Comment 26•12 years ago
|
||
Comment on attachment 728321 [details] [diff] [review]
Update to part 2 to deal with WebIDLification of Event
>diff --git a/content/events/src/nsEventListenerManager.cpp b/content/events/src/nsEventListenerManager.cpp
>--- a/content/events/src/nsEventListenerManager.cpp
>+++ b/content/events/src/nsEventListenerManager.cpp
>@@ -923,18 +923,18 @@ nsEventListenerManager::HandleEventSubTy
> nullptr);
> }
>
> if (NS_SUCCEEDED(result)) {
> nsAutoMicroTask mt;
> // nsIDOMEvent::currentTarget is set in nsEventDispatcher.
> if (aListener.HasWebIDLCallback()) {
> ErrorResult rv;
>- aListener.GetWebIDLCallback()->HandleEvent(aCurrentTarget, aDOMEvent,
>- rv);
>+ aListener.GetWebIDLCallback()->
>+ HandleEvent(aCurrentTarget, *static_cast<nsDOMEvent*>(aDOMEvent), rv);
*(aDOMEvent->InternalDOMEvent()) for now
to guarantee that you get the canonical pointer.
Attachment #728321 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 27•12 years ago
|
||
> *(aDOMEvent->InternalDOMEvent()) for now
Done.
Comment 28•12 years ago
|
||
Comment on attachment 712289 [details] [diff] [review]
part 4. Switch EventListener to WebIDL codegen.
Review of attachment 712289 [details] [diff] [review]:
-----------------------------------------------------------------
Any reason not to implement RemoveEventListener directly on dom::EventTarget?
Assignee | ||
Comment 29•12 years ago
|
||
Hmm. I guess GetListenerManager is on nsIDOMEventTarget... Then no, no reason. I'll do that!
Assignee | ||
Comment 30•12 years ago
|
||
Attachment #737695 -
Flags: review?(bugs)
Assignee | ||
Comment 31•12 years ago
|
||
Comment on attachment 712287 [details] [diff] [review]
part 2. Store an EventListenerHolder, not an nsIDOMEventListener in nsListenerStruct.
Given that the performance is ok now, re-requesting review on this bit...
Attachment #712287 -
Flags: review- → review?(bugs)
Comment 32•12 years ago
|
||
Comment on attachment 737695 [details] [diff] [review]
Addendum to part 4 to do comment 28
Could you move EventTarget::DispatchEvent from nsINode.cpp to
the new EventTarget.cpp
>+EventTarget::RemoveEventListener(const nsAString& aType,
>+ EventListener* aListener,
>+ bool aUseCapture,
>+ ErrorResult& aRv)
You have some tabs here. Please use spaces
Attachment #737695 -
Flags: review?(bugs) → review+
Updated•12 years ago
|
Attachment #712287 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 33•12 years ago
|
||
> You have some tabs here. Please use spaces
Done. Silly editors not noticing I added a modeline. ;)
Assignee | ||
Comment 34•12 years ago
|
||
Attachment #737986 -
Flags: review?(bugs)
Assignee | ||
Comment 35•12 years ago
|
||
Attachment #737991 -
Flags: review?(bugs)
Comment 36•12 years ago
|
||
Comment on attachment 737986 [details] [diff] [review]
Addendum to part 4 needed to silence gcc warnings so we can build with all the -Werror bits
horrible
Attachment #737986 -
Flags: review?(bugs) → review+
Updated•12 years ago
|
Attachment #737991 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 37•12 years ago
|
||
In the interests of this stuff not bitrotting more, I landed it disabled for now:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72c3a9deb098
https://hg.mozilla.org/integration/mozilla-inbound/rev/55cad36868d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d918701c596
https://hg.mozilla.org/integration/mozilla-inbound/rev/603143e31531
Summary: Switch EventListener to WebIDL codegen → Set up the infrastructure to switch EventListener to WebIDL codegen
Comment 38•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/72c3a9deb098
https://hg.mozilla.org/mozilla-central/rev/55cad36868d8
https://hg.mozilla.org/mozilla-central/rev/7d918701c596
https://hg.mozilla.org/mozilla-central/rev/603143e31531
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Updated•11 years ago
|
Depends on: CVE-2013-5616
You need to log in
before you can comment on or make changes to this bug.
Description
•