Implement NewNamedTimerCallback to create named timer callback

RESOLVED FIXED in Firefox 55

Status

()

Core
Panning and Zooming
P3
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: kuoe0, Assigned: kuoe0)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

7 months ago
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
(Assignee)

Updated

7 months ago
Assignee: nobody → kuoe0
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Blocks: 1342863
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?)
Priority: -- → P3
Whiteboard: [gfx-noted]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8848428 - Flags: review?(bugmail)

Comment 5

7 months ago
mozreview-review
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?
Attachment #8848428 - Flags: review?(bugmail)
(Assignee)

Comment 6

7 months ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
(Assignee)

Comment 9

7 months ago
What do think if I move this code to nsTimerImpl.h? I think nsTimerImpl.h is where the code belonging to.
Flags: needinfo?(bugmail)
(Assignee)

Updated

7 months ago
Blocks: 1348738
(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!
Flags: needinfo?(bugmail)

Comment 11

7 months ago
mozreview-review
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.
Attachment #8848428 - Flags: review?(bugmail) → review+
(Assignee)

Comment 12

7 months ago
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.
Comment hidden (mozreview-request)
(Assignee)

Comment 14

7 months ago
I'll deprecate the old one in Bug 1348900 later.
Comment hidden (mozreview-request)
Ok, that works too.
Blocks: 1348900
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 17

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

Comment 18

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34b87d7ffe4e
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

7 months ago
No longer blocks: 1348738
You need to log in before you can comment on or make changes to this bug.