Issue 3595 - Get rid of detached threads for setTimeout

The implementation is also threads friendly, i.e it does not spawn a new thread for each timer and allows a user to use another implementation.

[hub, 2017-03-27 13:42:22]

https://codereview.adblockplus.org/29395640/diff/29395659/src/DefaultTimer.h
File src/DefaultTimer.h (right):

https://codereview.adblockplus.org/29395640/diff/29395659/src/DefaultTimer.h#...
src/DefaultTimer.h:41: // pay attention 2 < 1 becaus we need the smallest time
at the top.
'nit: typo in the comment: "because"

[sergei, 2017-03-27 14:45:25]

https://codereview.adblockplus.org/29395640/diff/29395659/src/DefaultTimer.h
File src/DefaultTimer.h (right):

https://codereview.adblockplus.org/29395640/diff/29395659/src/DefaultTimer.h#...
src/DefaultTimer.h:41: // pay attention 2 < 1 becaus we need the smallest time
at the top.
On 2017/03/27 13:42:22, hub wrote:
> 'nit: typo in the comment: "because"

Done.

[hub, 2017-03-27 22:59:03]

LGTM

[Oleksandr, 2017-03-28 10:39:55]

https://codereview.adblockplus.org/29395640/diff/29395691/include/AdblockPlus...
File include/AdblockPlus/ITimer.h (right):

https://codereview.adblockplus.org/29395640/diff/29395691/include/AdblockPlus...
include/AdblockPlus/ITimer.h:19: #define ADBLOCK_PLUS_TIMER_H
The diff in this file is weird. I assume your merely add ITimer.h, not making
any changes to LogSystem.h, right?

https://codereview.adblockplus.org/29395640/diff/29395691/src/DefaultTimer.cpp
File src/DefaultTimer.cpp (right):

https://codereview.adblockplus.org/29395640/diff/29395691/src/DefaultTimer.cp...
src/DefaultTimer.cpp:43: std::lock_guard<std::mutex> lock(mutex);
If there's already a timer for let's say 1 minute the SetTimer call would just
hang for 1 minute here, right? I don't think that's acceptable.

https://codereview.adblockplus.org/29395640/diff/29395691/src/DefaultTimer.h
File src/DefaultTimer.h (right):

https://codereview.adblockplus.org/29395640/diff/29395691/src/DefaultTimer.h#...
src/DefaultTimer.h:40: bool operator()(const first_argument_type& t1, const
second_argument_type& t2) const {
I don't see why we need to use typedef'd type here. Why not just:

book operator()(const TimerUnit& t1, const TimerUnit& t2) const ?

[sergei, 2017-03-28 11:06:59]

https://codereview.adblockplus.org/29395640/diff/29395691/include/AdblockPlus...
File include/AdblockPlus/ITimer.h (right):

https://codereview.adblockplus.org/29395640/diff/29395691/include/AdblockPlus...
include/AdblockPlus/ITimer.h:19: #define ADBLOCK_PLUS_TIMER_H
On 2017/03/28 10:39:55, Oleksandr wrote:
> The diff in this file is weird. I assume your merely add ITimer.h, not making
> any changes to LogSystem.h, right?

Yes, VCS is trying to be smarter and it detects it as a copy of file with
modifications.

https://codereview.adblockplus.org/29395640/diff/29395691/src/DefaultTimer.cpp
File src/DefaultTimer.cpp (right):

https://codereview.adblockplus.org/29395640/diff/29395691/src/DefaultTimer.cp...
src/DefaultTimer.cpp:43: std::lock_guard<std::mutex> lock(mutex);
On 2017/03/28 10:39:55, Oleksandr wrote:
> If there's already a timer for let's say 1 minute the SetTimer call would just
> hang for 1 minute here, right? I don't think that's acceptable.

No, `conditionVariable.wait(lock,...` and `conditionVariable.wait_until(lock,`
in `DefaultTimer::ThreadFunc` "unlock" the mutex, so one can add timers.

https://codereview.adblockplus.org/29395640/diff/29395691/src/DefaultTimer.h
File src/DefaultTimer.h (right):

https://codereview.adblockplus.org/29395640/diff/29395691/src/DefaultTimer.h#...
src/DefaultTimer.h:40: bool operator()(const first_argument_type& t1, const
second_argument_type& t2) const {
On 2017/03/28 10:39:55, Oleksandr wrote:
> I don't see why we need to use typedef'd type here. Why not just:
> 
> book operator()(const TimerUnit& t1, const TimerUnit& t2) const ?

It seems we can, it simply was not working before.
Removed.

[Oleksandr, 2017-03-28 11:11:00]

LGTM

https://codereview.adblockplus.org/29395640/diff/29395691/src/DefaultTimer.cpp
File src/DefaultTimer.cpp (right):

https://codereview.adblockplus.org/29395640/diff/29395691/src/DefaultTimer.cp...
src/DefaultTimer.cpp:43: std::lock_guard<std::mutex> lock(mutex);
On 2017/03/28 11:06:59, sergei wrote:
> On 2017/03/28 10:39:55, Oleksandr wrote:
> > If there's already a timer for let's say 1 minute the SetTimer call would
just
> > hang for 1 minute here, right? I don't think that's acceptable.
> 
> No, `conditionVariable.wait(lock,...` and `conditionVariable.wait_until(lock,`
> in `DefaultTimer::ThreadFunc` "unlock" the mutex, so one can add timers.

Fair enough.