455988 - Mac Tdhtml regression from bug 450930

Status: RESOLVED FIXED

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

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This should do it.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Olli Pettay [:smaug][[e-mail]]]

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

[Mats Palmgren (inactive)]

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?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: better fix?

Is this more like what you had in mind, Olli?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 339925 [details] [diff] [review]
better fix?

Looks good to me, with Olli's comments.  sr=mats

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v3

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...)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

nsIELM is useful also outside of gecko, so eliminating it would lead to make
nsELM public.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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/ ?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Good idea.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 0db24c2c6a47. We'll see if this fixes perf.