The new architecture of Quantum DOM makes all runnables with their names. Currently, `NewTimerCallback()` can not create a named timer callback. So I think we should have another function `NewNamedTimerCallback()` to do that. Reference: http://searchfox.org/mozilla-central/source/gfx/layers/apz/util/APZThreadUtils.h#95-99

Did you mean to flag somebody for review on this patch? Or are you still working on this bug? (Seems like you need to update callers to pass in a useful name?)

Comment on attachment 8848428 [details] Bug 1348221 - Implement NewNamedTimerCallback to create named timer callback. https://reviewboard.mozilla.org/r/121336/#review123466 Who is going to be using this code? Right now it looks like there's only one user of NewTimerCallback, so if you're going to update that to use this you can also get rid of the old one that doesn't implement nsINamed. Dropping review flag for now, please explain more. ::: gfx/layers/apz/util/APZThreadUtils.h:139 (Diff revision 3) > + return NS_OK; > + } > + > + NS_IMETHOD SetName(const char * aName) override > + { > + return NS_ERROR_NOT_IMPLEMENTED; Why not implement this?

Comment on attachment 8848428 [details] Bug 1348221 - Implement NewNamedTimerCallback to create named timer callback. https://reviewboard.mozilla.org/r/121336/#review123466 Yes, there is only one place using `NewTimerCallback`. And it will be replaced with `NewNamedTimerCallback` in Bug 1342863. I'm just not sure I should get rid of the old one. If you think it's ok to remove the old one, I'll remove it. > Why not implement this? Oh, I found I missed `[noscript] void setName(in string aName);` in nsINamed.idl. I'll implement it in next version.

(In reply to Tommy Kuo [:KuoE0] from comment #6) > Yes, there is only one place using `NewTimerCallback`. And it will be > replaced with `NewNamedTimerCallback` in Bug 1342863. I'm just not sure I > should get rid of the old one. If you think it's ok to remove the old one, > I'll remove it. Yeah, I think it would be good to remove the old one, since it will be unused after you switch the caller to using the new one. Users who don't want to specify a name can just use an empty string or null or something. I'm ok with you removing the old one in a followup bug since you'll probably need to land this before you land bug 1342863, but you can only do the removal afterwards. Alternatively, you could just do a single patch which removes the old one, adds the new one, and updates the call site. That would be fine as well. > > Why not implement this? > > Oh, I found I missed `[noscript] void setName(in string aName);` in > nsINamed.idl. I'll implement it in next version. Ok, thanks.

What do think if I move this code to nsTimerImpl.h? I think nsTimerImpl.h is where the code belonging to.

(In reply to Tommy Kuo [:KuoE0] from comment #9) > What do think if I move this code to nsTimerImpl.h? I think nsTimerImpl.h is > where the code belonging to. When the code was originally added in bug 1200063 there was some discussion about this, and we filed bug 1205480 to move it into xpcom. I'd be totally happy if you moved it, but please do the work in bug 1205480. Thanks!

Comment on attachment 8848428 [details] Bug 1348221 - Implement NewNamedTimerCallback to create named timer callback. https://reviewboard.mozilla.org/r/121336/#review123920 r+ with comments addressed ::: gfx/layers/apz/util/APZThreadUtils.h:83 (Diff revision 4) > -class GenericTimerCallback final : public GenericTimerCallbackBase > +class GenericNamedTimerCallback final : public GenericNamedTimerCallbackBase > { > public: > - explicit GenericTimerCallback(const Function& aFunction) : mFunction(aFunction) {} > + explicit GenericNamedTimerCallback(const Function& aFunction, > + const char* aName) > + : mFunction(aFunction), mName(aName) {} nit: Please format this like so: : mFunction(aFunction) , mName(aName) { } ::: gfx/layers/apz/util/APZThreadUtils.h:116 (Diff revision 4) > // terse inline usage: > -// timer->InitWithCallback(NewTimerCallback([](){ ... }), delay); > +// timer->InitWithCallback(NewTimerCallback([](){ ... }, name), delay); > template <typename Function> > -GenericTimerCallback<Function>* NewTimerCallback(const Function& aFunction) > +GenericNamedTimerCallback<Function>* > + NewNamedTimerCallback(const Function& aFunction, > + const char* aName) This patch cannot land by itself, since this changed function signature will result in compilation failure. Please either add a default argument to this aName field (i.e. "const char* aName = nullptr"), or change the callsite to pass in something, like a nullptr or empty string. Then in your other bug you can update that call site to pass in a real string.

I think maybe I should deprecate the old function after bug 1342863 landed to make the functionality using the old function work and compile successfully. I'll upload another version with implementing the new function and without deprecating the old one.

I'll deprecate the old one in Bug 1348900 later.

Ok, that works too.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/34b87d7ffe4e Implement NewNamedTimerCallback to create named timer callback. r=kats