Implement setTimeout

Not using an event loop, but a separate thread for the timeout instead, as discussed.

[Wladimir Palant, 2013-04-08 08:36:00]

http://codereview.adblockplus.org/10085006/diff/1/src/GlobalJsObject.cpp
File src/GlobalJsObject.cpp (right):

http://codereview.adblockplus.org/10085006/diff/1/src/GlobalJsObject.cpp#newc...
src/GlobalJsObject.cpp:17: isolate,
v8::Local<v8::Function>::Cast(arguments[0]));
What will this produce if the first argument isn't a function? And what if there
isn't a sufficient number of arguments?

http://codereview.adblockplus.org/10085006/diff/1/src/GlobalJsObject.cpp#newc...
src/GlobalJsObject.cpp:37: const v8::Locker locker(isolate);
Does this really lock when this line is reached or already when the scope is
entered? I would add an additional scope after the Sleep() call.

http://codereview.adblockplus.org/10085006/diff/1/src/GlobalJsObject.cpp#newc...
src/GlobalJsObject.cpp:40: delete this;
Self-deleting object?

1) Is that a good practice?
2) Is it thread-safe? Won't it corrupt the memory allocation table if delete/new
operations run simultaneously on different threads?

http://codereview.adblockplus.org/10085006/diff/1/src/GlobalJsObject.cpp#newc...
src/GlobalJsObject.cpp:61: // clearTimeout(), we can safe that for later.
safe => save?

http://codereview.adblockplus.org/10085006/diff/1/src/JsEngine.cpp
File src/JsEngine.cpp (right):

http://codereview.adblockplus.org/10085006/diff/1/src/JsEngine.cpp#newcode112
src/JsEngine.cpp:112: std::string AdblockPlus::JsEngine::GetGlobal(const
std::string& name)
This name got me thoroughly confused. Please rename into "GetGlobalVariable" -
the global *object* is definitely not what you are looking for here.

http://codereview.adblockplus.org/10085006/diff/1/test/GlobalJsObject.cpp
File test/GlobalJsObject.cpp (right):

http://codereview.adblockplus.org/10085006/diff/1/test/GlobalJsObject.cpp#new...
test/GlobalJsObject.cpp:22: }
How about testing whether two timeouts execute in the right order? That would
also address my locking question from before.

[Felix Dahlke, 2013-04-08 13:07:48]

Uploaded a new patch set addressing all issues.

http://codereview.adblockplus.org/10085006/diff/1/src/GlobalJsObject.cpp
File src/GlobalJsObject.cpp (right):

http://codereview.adblockplus.org/10085006/diff/1/src/GlobalJsObject.cpp#newc...
src/GlobalJsObject.cpp:17: isolate,
v8::Local<v8::Function>::Cast(arguments[0]));
On 2013/04/08 08:36:00, Wladimir Palant wrote:
> What will this produce if the first argument isn't a function? And what if
there
> isn't a sufficient number of arguments?

Phew, good find. V8 isn't exactly documented but I assumed they'd throw an
exception and that's it... Instead it's actually causing a fatal error and
crashes the whole runtime.

I made sure to check argument number and types properly and added some tests.

http://codereview.adblockplus.org/10085006/diff/1/src/GlobalJsObject.cpp#newc...
src/GlobalJsObject.cpp:37: const v8::Locker locker(isolate);
On 2013/04/08 08:36:00, Wladimir Palant wrote:
> Does this really lock when this line is reached or already when the scope is
> entered? I would add an additional scope after the Sleep() call.

Yes it will, the constructor is being called after the sleep, and I'd be very
surprised if any compiler changed the order here.

http://codereview.adblockplus.org/10085006/diff/1/src/GlobalJsObject.cpp#newc...
src/GlobalJsObject.cpp:40: delete this;
On 2013/04/08 08:36:00, Wladimir Palant wrote:
> Self-deleting object?
> 
> 1) Is that a good practice?
> 2) Is it thread-safe? Won't it corrupt the memory allocation table if
delete/new
> operations run simultaneously on different threads?

1) Depends. It's not generally considered a bad practice, but I can't think of
many situations where it makes sense. I can't think of another way that doesn't
involve some additional house keeping thread, which I'd rather avoid.

2) It's not thread safe, but we can ensure thread safety by making sure that we
don't delete the object from the outside. And since we don't even store a
reference to it, that's safe right now.

http://codereview.adblockplus.org/10085006/diff/1/src/GlobalJsObject.cpp#newc...
src/GlobalJsObject.cpp:61: // clearTimeout(), we can safe that for later.
On 2013/04/08 08:36:00, Wladimir Palant wrote:
> safe => save?

Yes, of course.

http://codereview.adblockplus.org/10085006/diff/1/src/JsEngine.cpp
File src/JsEngine.cpp (right):

http://codereview.adblockplus.org/10085006/diff/1/src/JsEngine.cpp#newcode112
src/JsEngine.cpp:112: std::string AdblockPlus::JsEngine::GetGlobal(const
std::string& name)
On 2013/04/08 08:36:00, Wladimir Palant wrote:
> This name got me thoroughly confused. Please rename into "GetGlobalVariable" -
> the global *object* is definitely not what you are looking for here.

Sure. Called it "GetVariable" since I think that "Global" is kinda obvious
there.

http://codereview.adblockplus.org/10085006/diff/1/test/GlobalJsObject.cpp
File test/GlobalJsObject.cpp (right):

http://codereview.adblockplus.org/10085006/diff/1/test/GlobalJsObject.cpp#new...
test/GlobalJsObject.cpp:22: }
On 2013/04/08 08:36:00, Wladimir Palant wrote:
> How about testing whether two timeouts execute in the right order? That would
> also address my locking question from before.

Sure, added a test.

[Wladimir Palant, 2013-04-08 14:03:47]

LGTM, feel free to address the nits however.

http://codereview.adblockplus.org/10085006/diff/1/src/GlobalJsObject.cpp
File src/GlobalJsObject.cpp (right):

http://codereview.adblockplus.org/10085006/diff/1/src/GlobalJsObject.cpp#newc...
src/GlobalJsObject.cpp:40: delete this;
I checked, delete is actually thread-safe - so no issue here. The deleting part
might be made a bit nicer with smart pointers but probably not worth it.

http://codereview.adblockplus.org/10085006/diff/6001/src/GlobalJsObject.cpp
File src/GlobalJsObject.cpp (right):

http://codereview.adblockplus.org/10085006/diff/6001/src/GlobalJsObject.cpp#n...
src/GlobalJsObject.cpp:22: throw std::runtime_error("No valid function passed to
setTimeout");
Nit: I think the canonical message is: "First argument to setTimeout must be a
function".

http://codereview.adblockplus.org/10085006/diff/6001/src/GlobalJsObject.cpp#n...
src/GlobalJsObject.cpp:26: throw std::runtime_error("No valid delay passed to
setTimeout");
Nit: I think the canonical message is: "Second argument to setTimeout must be a
number".

[Felix Dahlke, 2013-04-08 15:10:15]

On 2013/04/08 14:03:47, Wladimir Palant wrote:
> LGTM, feel free to address the nits however.
> 
> http://codereview.adblockplus.org/10085006/diff/1/src/GlobalJsObject.cpp
> File src/GlobalJsObject.cpp (right):
> 
>
http://codereview.adblockplus.org/10085006/diff/1/src/GlobalJsObject.cpp#newc...
> src/GlobalJsObject.cpp:40: delete this;
> I checked, delete is actually thread-safe - so no issue here. The deleting
part
> might be made a bit nicer with smart pointers but probably not worth it.
> 
> http://codereview.adblockplus.org/10085006/diff/6001/src/GlobalJsObject.cpp
> File src/GlobalJsObject.cpp (right):
> 
>
http://codereview.adblockplus.org/10085006/diff/6001/src/GlobalJsObject.cpp#n...
> src/GlobalJsObject.cpp:22: throw std::runtime_error("No valid function passed
to
> setTimeout");
> Nit: I think the canonical message is: "First argument to setTimeout must be a
> function".
> 
>
http://codereview.adblockplus.org/10085006/diff/6001/src/GlobalJsObject.cpp#n...
> src/GlobalJsObject.cpp:26: throw std::runtime_error("No valid delay passed to
> setTimeout");
> Nit: I think the canonical message is: "Second argument to setTimeout must be
a
> number".

Fixed the nits.