Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29389576: Issue 4694 - JsEngine::{SetEventCallback,RemoveEventCallback,TriggerEvent} should be thread safe. (Closed)

Created:
March 20, 2017, 1:59 p.m. by sergei
Modified:
March 21, 2017, 2:31 p.m.
Reviewers:
Eric, Oleksandr, hub
CC:
Felix Dahlke, anton
Visibility:
Public.

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -3 lines) Patch
M include/AdblockPlus/JsEngine.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/JsEngine.cpp View 1 chunk +11 lines, -3 lines 4 comments Download

Messages

Total messages: 6
sergei
March 20, 2017, 2:09 p.m. (2017-03-20 14:09:43 UTC) #1
hub
LGTM
March 20, 2017, 5:42 p.m. (2017-03-20 17:42:38 UTC) #2
Oleksandr
LGTM with a nit https://codereview.adblockplus.org/29389576/diff/29389577/src/JsEngine.cpp File src/JsEngine.cpp (right): https://codereview.adblockplus.org/29389576/diff/29389577/src/JsEngine.cpp#newcode140 src/JsEngine.cpp:140: callback(params); Nit: Maybe check if ...
March 20, 2017, 8 p.m. (2017-03-20 20:00:42 UTC) #3
sergei
https://codereview.adblockplus.org/29389576/diff/29389577/src/JsEngine.cpp File src/JsEngine.cpp (right): https://codereview.adblockplus.org/29389576/diff/29389577/src/JsEngine.cpp#newcode140 src/JsEngine.cpp:140: callback(params); On 2017/03/20 20:00:42, Oleksandr wrote: > Nit: Maybe ...
March 20, 2017, 8:08 p.m. (2017-03-20 20:08:13 UTC) #4
sergei
https://codereview.adblockplus.org/29389576/diff/29389577/src/JsEngine.cpp File src/JsEngine.cpp (right): https://codereview.adblockplus.org/29389576/diff/29389577/src/JsEngine.cpp#newcode140 src/JsEngine.cpp:140: callback(params); On 2017/03/20 20:08:13, sergei wrote: > On 2017/03/20 ...
March 20, 2017, 8:23 p.m. (2017-03-20 20:23:24 UTC) #5
Oleksandr
March 20, 2017, 8:25 p.m. (2017-03-20 20:25:25 UTC) #6
https://codereview.adblockplus.org/29389576/diff/29389577/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29389576/diff/29389577/src/JsEngine.cpp#ne...
src/JsEngine.cpp:140: callback(params);
On 2017/03/20 20:23:24, sergei wrote:
> On 2017/03/20 20:08:13, sergei wrote:
> > On 2017/03/20 20:00:42, Oleksandr wrote:
> > > Nit: Maybe check if callback is not null JIC?
> > 
> > What about such check in SetEventCallback? Maybe even removing of existing
> > callback if an empty value is passed into SetEventCallback, though I would
> like
> > to avoid such dualism, it definitely should not be a documented way while
> there
> > is RemoveEventCallback.
> 
> Actually, I think it should be in another commit.

Fair enough. It isn't that big of an issue IMO, so, again, LGTM.

Powered by Google App Engine
This is Rietveld