1205480 - Introduce a generic (templated) implementation of nsITimerCallback

Status: RESOLVED DUPLICATE of bug 1711090

(Core :: XPCOM, defect)

Description

[Botond Ballo [:botond]]

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.

Comment 1

[Botond Ballo [:botond]]

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 2

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1205480 - Move NewNamedTimerCallback to XPCOM.

Review commit: https://reviewboard.mozilla.org/r/123340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123340/

Comment 3

[Chih-Hsuan Kuo [:tommykuo]]

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!

Comment 4

[Botond Ballo [: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.

Comment 5

[Chih-Hsuan Kuo [:tommykuo]]

Hi Benjamin, do you think we need to implement another non-atomic version for this?

Comment 6

[Benjamin Smedberg]

I'm not the best person for this any more: redirecting to erahm.

Comment 7

[Eric Rahm [: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

Comment 8

[Nathan Froyd [:froydnj]]

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

Comment 9

[Botond Ballo [:botond]]

This has largely been superseded by bug 1711090 (not an exact duplicate as the mechanism there uses typed-erased functions, but close enough).