LGTM We should make sure to implement this on Windows so d8 works equally well on all supported platforms.
http://codereview.chromium.org/42641/diff/1/5 File src/d8-posix.cc (right): http://codereview.chromium.org/42641/diff/1/5#newcode147 Line 147: if (seconds * 1000000 + time_now.tv_usec - start_time->tv_usec > total_time * 1000) { Does it lint? How about naming some intermediate results? http://codereview.chromium.org/42641/diff/1/5#newcode202 Line 202: Handle<Value> Shell::System(const Arguments& args) { This method is very, very big. There seems to be phases here that you should be able to factor out? http://codereview.chromium.org/42641/diff/1/5#newcode208 Line 208: if (args.Length() > first_argument && args[first_argument]->IsInt32()) { I would like to have the optional timeout arguments after the program and arguments. How about requiring a string name, an array of arguments, and then having two optional timeouts after that: system("program", [1, 2, 3, 4], 100, 100)? http://codereview.chromium.org/42641/diff/1/5#newcode289 Line 289: if (WaitOnFD(stdout_fds[kReadFD], read_timeout, total_timeout, &start_time) || Long line. http://codereview.chromium.org/42641/diff/1/3 File src/d8-windows.cc (right): http://codereview.chromium.org/42641/diff/1/3#newcode39 Line 39: return ThrowException(String::New("system() is not yet supported on your OS")); Long line. http://codereview.chromium.org/42641 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
