Overall, this looks good to me. I'm puzzled by the checking going on after one callback though. We have many more callbacks in the code. Why is the checking only needed for one of them?
I think it would be good if Søren could have a look at this change as well since he did the latest changes in this area. http://codereview.chromium.org/12901/diff/1/5 File src/builtins.cc (right): http://codereview.chromium.org/12901/diff/1/5#newcode389 Line 389: Top::ReportPendingMessages(); I'm not sure I understand why we need to check here? There are a lot of places in the code where we invoke a c++ callback function. If we need to check here, don't we need to check in the other places as well? http://codereview.chromium.org/12901/diff/1/6 File src/execution.cc (right): http://codereview.chromium.org/12901/diff/1/6#newcode102 Line 102: Top::setup_external_caught(); Why is setup_external_caught needed in the case that there was no exception? http://codereview.chromium.org/12901/diff/1/3 File src/top.cc (right): http://codereview.chromium.org/12901/diff/1/3#newcode859 Line 859: = thread_local_.pending_exception_; We normally have the '=' on the previous line. http://codereview.chromium.org/12901/diff/1/2 File test/cctest/test-api.cc (right): http://codereview.chromium.org/12901/diff/1/2#newcode3000 Line 3000: Script::Compile(v8_str("(function() { try { throw ''; } finally { return; } })()"))->Run(); These two lines seem too long? Break them by using the C++ concatenation of adjacent strings? http://codereview.chromium.org/12901 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
