Closed Bug 455988 Opened 12 years ago Closed 12 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: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.