Closed Bug 455988 Opened 17 years ago Closed 17 years ago

Mac Tdhtml regression from bug 450930

Categories

(Core :: DOM: Events, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 2 obsolete files)

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?
Attached patch fix (obsolete) — Splinter Review
This should do it.
Attachment #339398 - Flags: superreview?(mats.palmgren)
Attachment #339398 - Flags: review?(Olli.Pettay)
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-
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?
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 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)
Attached patch better fix? (obsolete) — Splinter Review
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)
Attachment #339925 - Flags: review?(Olli.Pettay) → review+
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
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P3
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+
Attached patch fix v3Splinter Review
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)
nsIELM is useful also outside of gecko, so eliminating it would lead to make nsELM public.
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+
Pushed 0db24c2c6a47. We'll see if this fixes perf.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: