Open Bug 1205480 Opened 9 years ago Updated 2 years ago

Introduce a generic (templated) implementation of nsITimerCallback

Categories

(Core :: XPCOM, defect)

defect

Tracking

()

Tracking Status
firefox43 --- affected

People

(Reporter: botond, Unassigned)

Details

Attachments

(1 file)

C++11 added lambdas to the language to avoid the verbose practise of declaring a namespace-scope class for every function object passed to a generic algorithm. It would be nice to be able to leverage this brevity for implementations of nsITimerCallback. The third patch in bug 1200063 introduces a templated derived class, GenericTimerCallback, which implements nsITimerCallback, and can be instantiated with any lambda (or other function object) that's callable with no arguments. That utility class is currently located in gfx/layers/apz/util. If people like the idea, I'd like to move it to xpcom/threads.
One snag is that some users may need the refcount of said class to be thread-safe, while others don't. (I just had to change the one in gfx/layers/apz/util to be thread-safe, because its user needed that.) If it'll live in XPCOM, will we want two separate ones that offer either behaviour?
Comment on attachment 8850838 [details] Bug 1205480 - Move NewNamedTimerCallback to XPCOM. Hi botond, I intent to move NewNamedTimerCallback (template) to XPCOM. Can you give me some feedback? Thanks!
Attachment #8850838 - Flags: feedback?(botond)
Comment on attachment 8850838 [details] Bug 1205480 - Move NewNamedTimerCallback to XPCOM. https://reviewboard.mozilla.org/r/123340/#review125962 You'll want to either move the class into namespace mozilla, or prefix its name with "ns". Otherwise, LGTM. The only possible issue is what I mentioned in comment 1: consumers that don't want to pay for atomic refcounts may want a similar class with regular (non-atomic) refcounts. Note that you'll need review from an XPCOM peer to land this.
Attachment #8850838 - Flags: feedback?(botond)
Hi Benjamin, do you think we need to implement another non-atomic version for this?
Flags: needinfo?(benjamin)
Assignee: nobody → kuoe0
I'm not the best person for this any more: redirecting to erahm.
Flags: needinfo?(benjamin) → needinfo?(erahm)
Given this is only used in one place [1] I'm not necessarily convinced of its overall utility. If you could identify some spots where we could get rid of boilerplate code I'd be more interested in moving this to xpcom. In general it seems like there's overlap with our existing NS_NewRunnableFunction interface [2]. I wonder if there's something clever we can do to share the logic, particularly it's nice that we support functions with arguments and handle safely storing their references. Nathan, what do you think? (In reply to Botond Ballo [:botond] from comment #4) > The only possible issue is what I mentioned in comment 1: > consumers that don't want to pay for atomic refcounts may want a similar > class with regular (non-atomic) refcounts. I think we currently only allow thread-safe runnables [3] in nsThreadUtils, so maybe this is okay for now. [1] http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/layout/base/nsPresContext.cpp#3315-3319 [2] http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/xpcom/threads/nsThreadUtils.h#436-442 [3] http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/xpcom/threads/nsThreadUtils.h#239-243
Flags: needinfo?(erahm) → needinfo?(nfroyd)
(In reply to Eric Rahm [:erahm] from comment #7) > In general it seems like there's overlap with our existing > NS_NewRunnableFunction interface [2]. I wonder if there's something clever > we can do to share the logic, particularly it's nice that we support > functions with arguments and handle safely storing their references. Beyond the "both of these classes store function objects to execute later", I'm not sure that there's much of an argument for code-sharing. > (In reply to Botond Ballo [:botond] from comment #4) > > The only possible issue is what I mentioned in comment 1: > > consumers that don't want to pay for atomic refcounts may want a similar > > class with regular (non-atomic) refcounts. Given that refcounting traffic on these instances should be pretty small, and having both variants would be a lot more code, I'm inclined to say we should just stick with the atomic refcounting all the time. Did there used to be more users of this functionality? I can only find the one use in nsPresContext. Please note that any proposed class being moved to XPCOM should live in the mozilla:: namespace.
Flags: needinfo?(nfroyd)
Assignee: kuoe0.tw → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: