Open Bug 683785 Opened 9 years ago Updated 2 years ago

Create timers directly instead of with do_CreateInstance


(Core :: XPCOM, defect)

Not set




(Reporter: jrmuizel, Assigned: reznord, Mentored)


(Whiteboard: [lang=c++])


(1 file, 1 obsolete file)

Attached patch Create timers directly. (obsolete) — Splinter Review
This renames nsTimerImpl to mozilla::Timer and changes do_CreateInstance(";1", &err);
into 'new mozilla::Timer'

This has a bunch of advantages:
* it's nicer to look at
* it removes the need to handle do_CreateInstance() failure. (which about 1/3 of callers weren't handling)
* will let compilers easily devirtualize the call to InitWithFuncCallback etc.
Attachment #557374 - Flags: feedback?(benjamin)
Attachment #557374 - Attachment is patch: true
Comment on attachment 557374 [details] [diff] [review]
Create timers directly.

I'd be surprised if even modern compilers were smart enough to devirtualize InitWithFuncCallback without C++0x "final" markings. I think I'm ok with the general change here, although I'm still wondering about my XXX comment in nsTimerImpl::nsTimerImpl and whether the constructor should actually be fallible.

Also this patch is missing the changes to nsTimerImpl.h/cpp, which seem like the important part ;-)
Attachment #557374 - Flags: feedback?(benjamin) → feedback+
- Fix conversion of CallCreateInstance
- Add missing files
- Take out some imglib stuff that snuck in
Attachment #557374 - Attachment is obsolete: true
Many of the uses of Timers don't need to share them, so I think I'm going to rework this so that you can just use a Timer as a value.
Thanks for looking at this, Anuj!
Assignee: nobody → anujagarwal464
Mentor: nfroyd
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [lang=c++]
Anuj, are you still planning on working on this?
Flags: needinfo?(anujagarwal464)
@Nathan, If somebody is working on this reassign it to them.
Flags: needinfo?(anujagarwal464)
Thanks, Anuj.  I'm going to clear the Assignee field so other people searching for C++ bugs to work on can see that this bug is available.
Assignee: anujagarwal464 → nobody
@Nathan What is it that remains in this task? I can look into it if nobody else is in line. Just give me a bit of a spec and I'll fix it.
(In reply to Linus Probert from comment #8)
> @Nathan What is it that remains in this task? I can look into it if nobody
> else is in line. Just give me a bit of a spec and I'll fix it.

The basics are already done in attachment 558933 [details] [diff] [review].  What remains is:

- Fixing up the patch for current mozilla-central (it's been a couple years)--the scripts in bug 579517 will probably be helpful here;
- Replacing any new additions of |do_CreateInstance(";1");| with |new Timer|;
- Testing the patch to ensure that it doesn't regress anything.

I think the idea in comment 3 of using timers as values can wait for a followup patch.
Hi Nathan,

I would like to work on this bug, can you please assign it to me?
Assignee: nobody → allamsetty.anup
Hello, I would like to work on this bug
(In reply to aliyua7 from comment #11)
> Hello, I would like to work on this bug


I am working on this bug, currently I'm busy with exams till 11th December, after that I will be working on this. 

And this is assigned to me also :)
Hi, can I be assigned to this bug? I can get to work on it immediately.
You need to log in before you can comment on or make changes to this bug.