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

[hub, 2017-03-20 17:42:38]

LGTM

[Oleksandr, 2017-03-20 20:00:42]

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#ne...
src/JsEngine.cpp:140: callback(params);
Nit: Maybe check if callback is not null JIC?

[sergei, 2017-03-20 20:08:13]

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: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.

[sergei, 2017-03-20 20:23:24]

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: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.

[Oleksandr, 2017-03-20 20:25:25]

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.