523785 - Use NS_DispatchToMainThread instead of nsITimer

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

spun of bug 423737 comment #46

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

I didn't touch timer for events queue. I'll fix this as part of bug 515141.

I hope it's ok we don't cancel pending actions any more. It should be safe.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 422748 [details] [diff] [review]
patch

>diff --git a/accessible/src/base/nsCoreUtils.h b/accessible/src/base

I didn't review this part yet.

>   if (eventType.EqualsLiteral("focus")) {
>     if (aTargetNode == mDOMNode && mDOMNode != gLastFocusedNode) {
>       // Got focus event for the window, we will make sure that an accessible
>       // focus event for initial focus is fired. We do this on a short timer
>       // because the initial focus may not have been set yet.
>-      if (!mFireFocusTimer) {
>-        mFireFocusTimer = do_CreateInstance("@mozilla.org/timer;1");
>-      }
>-      if (mFireFocusTimer) {
>-        mFireFocusTimer->InitWithFuncCallback(FireFocusCallback, this,
>-                                              0, nsITimer::TYPE_ONE_SHOT);
>-      }
>+      NS_DISPATCH_RUNNABLEMETHOD(FireCurrentFocusEvent, this)

This scares me a bit given the preceding comment. Will dispatching to the main thread make this a non-issue. Note the preceding comment reference to "short timer" is now incorrect with this patch :)

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)

> This scares me a bit given the preceding comment. Will dispatching to the main
> thread make this a non-issue. Note the preceding comment reference to "short
> timer" is now incorrect with this patch :)

NS_DispatchToMainThread is always async so it should be the same like 0 timeout. Let's wait for Olli's comments.

I'll fix the comment.

Comment 4

[alexander :surkov (:asurkov)]

There is misspeling in DoCommand, I pass aContent instead of content into runnable. I fiexed this locally.

Comment 5

[alexander :surkov (:asurkov)]

try server build - http://build.mozilla.org/tryserver-builds/[e-mail]-try-56ce38999c6b

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #4)
> There is misspeling in DoCommand, I pass aContent instead of content into
> runnable. I fiexed this locally.

ok

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 422748 [details] [diff] [review]
patch

I can do conditional r=me, but I'd like to see Olli's (and Ginn's) comments before this lands. Thanks.

Comment 8

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

Comment on attachment 422748 [details] [diff] [review]
patch

>+////////////////////////////////////////////////////////////////////////////////
>+// nsRunnable helpers
>+////////////////////////////////////////////////////////////////////////////////
>+
>+/**
>+ * Use NS_DECL_RUNNABLEMETHOD_ macros to declare a runnable class for the given
>+ * method of the given class. There are three macros:
>+ *  NS_DECL_RUNNABLEMETHOD(Class, Method)
>+ *  NS_DECL_RUNNABLEMETHOD_ARG1(Class, Method, Arg1Type)
>+ *  NS_DECL_RUNNABLEMETHOD_ARG2(Class, Method, Arg1Type, Arg2Type)
Could you explain here that Arg1type and Arg2Type must be 
types which keep the objects alive, or copy the value.
It is easy to make a mistake and use something like nsISupports*.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)

> Could you explain here that Arg1type and Arg2Type must be 
> types which keep the objects alive, or copy the value.
> It is easy to make a mistake and use something like nsISupports*.

Sure, I added the comment.

Comment 10

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/b8a1a64e36e0