Use NS_DispatchToMainThread instead of nsITimer

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Comment 1

9 years ago
Created attachment 422748 [details] [diff] [review]
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.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #422748 - Flags: superreview?(Olli.Pettay)
Attachment #422748 - Flags: review?(ginn.chen)
Attachment #422748 - Flags: review?(bolterbugz)
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 :)
(Assignee)

Comment 3

9 years ago
(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.
(Assignee)

Comment 4

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

ok
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.
Attachment #422748 - Flags: review?(bolterbugz) → review+
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*.
Attachment #422748 - Flags: superreview?(Olli.Pettay) → superreview+

Updated

9 years ago
Attachment #422748 - Flags: review?(ginn.chen) → review+
(Assignee)

Comment 9

9 years ago
(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.
(Assignee)

Comment 10

9 years ago
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/b8a1a64e36e0
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.