Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(975)

Issue 29395640: Issue 3595 - Get rid of detached threads for setTimeout (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 7 months ago by sergei
Modified:
2 years, 7 months ago
Reviewers:
Eric, hub, Oleksandr
CC:
Felix Dahlke
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 2

Patch Set 2 : address comment #

Patch Set 3 : only rebase #

Total comments: 7

Patch Set 4 : address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+220 lines, -75 lines) Patch
A + include/AdblockPlus/ITimer.h View 1 chunk +16 lines, -20 lines 0 comments Download
M include/AdblockPlus/JsEngine.h View 6 chunks +26 lines, -16 lines 0 comments Download
M libadblockplus.gyp View 1 chunk +3 lines, -0 lines 0 comments Download
A src/DefaultTimer.h View 1 2 3 1 chunk +61 lines, -0 lines 0 comments Download
A src/DefaultTimer.cpp View 1 chunk +83 lines, -0 lines 0 comments Download
M src/GlobalJsObject.cpp View 1 chunk +1 line, -23 lines 0 comments Download
M src/JsEngine.cpp View 3 chunks +29 lines, -15 lines 0 comments Download
M test/BaseJsTest.cpp View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 7
sergei
2 years, 7 months ago (2017-03-27 10:26:43 UTC) #1
hub
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#newcode41 src/DefaultTimer.h:41: // pay attention 2 < 1 becaus we need ...
2 years, 7 months ago (2017-03-27 13:42:22 UTC) #2
sergei
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#newcode41 src/DefaultTimer.h:41: // pay attention 2 < 1 becaus we need ...
2 years, 7 months ago (2017-03-27 14:45:25 UTC) #3
hub
LGTM
2 years, 7 months ago (2017-03-27 22:59:03 UTC) #4
Oleksandr
https://codereview.adblockplus.org/29395640/diff/29395691/include/AdblockPlus/ITimer.h File include/AdblockPlus/ITimer.h (right): https://codereview.adblockplus.org/29395640/diff/29395691/include/AdblockPlus/ITimer.h#newcode19 include/AdblockPlus/ITimer.h:19: #define ADBLOCK_PLUS_TIMER_H The diff in this file is weird. ...
2 years, 7 months ago (2017-03-28 10:39:55 UTC) #5
sergei
https://codereview.adblockplus.org/29395640/diff/29395691/include/AdblockPlus/ITimer.h File include/AdblockPlus/ITimer.h (right): https://codereview.adblockplus.org/29395640/diff/29395691/include/AdblockPlus/ITimer.h#newcode19 include/AdblockPlus/ITimer.h:19: #define ADBLOCK_PLUS_TIMER_H On 2017/03/28 10:39:55, Oleksandr wrote: > The ...
2 years, 7 months ago (2017-03-28 11:06:59 UTC) #6
Oleksandr
2 years, 7 months ago (2017-03-28 11:11:00 UTC) #7
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5