Closed
Bug 455988
Opened 17 years ago
Closed 17 years ago
Mac Tdhtml regression from bug 450930
Categories
(Core :: DOM: Events, defect, P3)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
People
(Reporter: roc, Assigned: roc)
References
Details
(Keywords: fixed1.9.1)
Attachments
(1 file, 2 obsolete files)
16.35 KB,
patch
|
smaug
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
See bug 450930 comment #45. The events patch there regressed Mac Tdhtml performance. So we need to try adding an optimization to prevent events from dispatching when no-one's listening.
Flags: blocking1.9.1?
Assignee | ||
Comment 1•17 years ago
|
||
This should do it.
Attachment #339398 -
Flags: superreview?(mats.palmgren)
Attachment #339398 -
Flags: review?(Olli.Pettay)
Comment 2•17 years ago
|
||
Comment on attachment 339398 [details] [diff] [review]
fix
This doesn't do the right thing if any of chromehandler's ancestors have listener for MozAfterPaint.
Attachment #339398 -
Flags: review?(Olli.Pettay) → review-
Assignee | ||
Comment 3•17 years ago
|
||
I didn't realize events can bubble up from the chrome event handler :-(.
Then I'm not sure how to interpret https://bugzilla.mozilla.org/show_bug.cgi?id=450930#c34. How is doing what I'm doing here different from calling HasListenersFor on the chrome event handler's nsEventListenerManager? Do I need to be checking the type of the chrome event handler? What kind of object is it normally in Firefox?
Comment 4•17 years ago
|
||
xul:browser is the chromeeventhandler for content |window|. nsWindowRoot is
usually the chromeeventhandler for top level chrome |window|, but in embedded
environment it can be something else too.
So I'd add a bit to nsPIDOMWindow if it or any nodes in it has a listener
for MozAfterPaint and then check all the ancestor windows and top level
chromeeventhandler if they have listener for MozAfterPaint.
Comment 5•17 years ago
|
||
Comment on attachment 339398 [details] [diff] [review]
fix
> nsPresContext::FireDOMPaintEvent()
+ nsCOMPtr<nsPIDOMWindow> ourWindow = mDocument->GetWindow();
I think we need to null-check 'ourWindow' and return early.
And the result of GetChromeEventHandler() too?
Attachment #339398 -
Flags: superreview?(mats.palmgren)
Assignee | ||
Comment 6•17 years ago
|
||
Is this more like what you had in mind, Olli?
Attachment #339398 -
Attachment is obsolete: true
Attachment #339925 -
Flags: superreview?(mats.palmgren)
Attachment #339925 -
Flags: review?(Olli.Pettay)
Updated•17 years ago
|
Attachment #339925 -
Flags: review?(Olli.Pettay) → review+
Comment 7•17 years ago
|
||
Comment on attachment 339925 [details] [diff] [review]
better fix?
>+ /**
>+ * Returns PR_TRUE if there may be a paint event listener registered,
>+ * PR_FALSE if there definitely isn't.
>+ */
>+ PRBool GetMayHavePaintEventListener() { return mMayHavePaintEventListener; }
Just call the method MayHavePaintEventListener()
>+
>+protected:
>+ PRPackedBool mMayHavePaintEventListener;
I'd move this to nsEventListener, since there are still extra bits left in
mNoListenerForEvent.
> /**
>+ * Call this to indicate that some node (this window, its document,
>+ * or content in that document) has a paint event listener.
>+ */
>+ void SetHasPaintEventListenerNode()
>+ {
>+ mMayHavePaintEventListener = PR_TRUE;
>+ }
>+
>+ /**
>+ * Call this to check whether some node (this window, its document,
>+ * or content in that document) has a paint event listener.
>+ */
>+ PRBool GetMayHavePaintEventListenerNode()
>+ {
>+ return mMayHavePaintEventListener;
>+ }
Call these HasPaintEventListeners/SetHasPaintEventListeners, just
to be consistent with mutation listeners.
When adopting nodes you need to set its ELM's MayHavePaintEventListener() to new
window. See nsNodeUtils.cpp:582
With those r=me
Updated•17 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P3
Comment 8•17 years ago
|
||
Comment on attachment 339925 [details] [diff] [review]
better fix?
Looks good to me, with Olli's comments. sr=mats
Attachment #339925 -
Flags: superreview?(mats.palmgren) → superreview+
Assignee | ||
Comment 9•17 years ago
|
||
Olli, is this OK? I ended up moving mNoListenerForEvent up to nsIEventListenerManager so that the accessors can be inline up there. (We should just eliminate/deCOM nsIEventListenerManager in favour of nsEventListenerManager, IMHO...)
Attachment #339925 -
Attachment is obsolete: true
Attachment #342837 -
Flags: superreview+
Attachment #342837 -
Flags: review?(Olli.Pettay)
Comment 10•17 years ago
|
||
nsIELM is useful also outside of gecko, so eliminating it would lead to make
nsELM public.
Comment 11•17 years ago
|
||
Comment on attachment 342837 [details] [diff] [review]
fix v3
>+protected:
>+ PRPackedBool mMayHavePaintEventListener : 1;
>+ PRPackedBool mMayHaveMutationListeners : 1;
>+ // These two member variables are used to cache the information
>+ // about the last event which was handled but for which event listener manager
>+ // didn't have event listeners.
>+ PRUint32 mNoListenerForEvent : 30;
> };
Do all the compilers do the right thing here (IIRC some compiler on Windows wanted
member to be same type in this case). Should you perhaps s/PRPackedBool/PRUint32/ ?
Attachment #342837 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 12•17 years ago
|
||
Good idea.
Assignee | ||
Comment 13•17 years ago
|
||
Pushed 0db24c2c6a47. We'll see if this fixes perf.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Keywords: fixed1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•