LGTM with a few comments:
http://codereview.chromium.org/6223/diff/1/10 File src/array.js (right): http://codereview.chromium.org/6223/diff/1/10#newcode932 Line 932: InstallFunctions($Array.prototype, DONT_ENUM, new $Array( I guess an array literal doesn't work here? You could also get rid of the 'new' keyword... http://codereview.chromium.org/6223/diff/1/7 File src/date-delay.js (right): http://codereview.chromium.org/6223/diff/1/7#newcode645 Line 645: }; I would rather remove the semicolons than add new ones, but maybe we should do that in a separate pass? http://codereview.chromium.org/6223/diff/1/7#newcode965 Line 965: InstallFunctions($Date, DONT_ENUM, new $Array( See other comment. http://codereview.chromium.org/6223/diff/1/7#newcode976 Line 976: InstallFunctions($Date.prototype, DONT_ENUM, new $Array( Already mentioned in other comment. http://codereview.chromium.org/6223/diff/1/13 File src/math.js (right): http://codereview.chromium.org/6223/diff/1/13#newcode143 Line 143: InstallFunctions($Math, DONT_ENUM, new $Array( Yup. Again. http://codereview.chromium.org/6223/diff/1/9 File src/messages.js (right): http://codereview.chromium.org/6223/diff/1/9#newcode654 Line 654: DefineError('Error', function Error(msg) { }); Do you need to pass in the name here too? Maybe you could just get that from the f function? I don't know. Does that (msg) part buy us anything? We replace the code of the function anyway, right? http://codereview.chromium.org/6223/diff/1/6 File src/regexp-delay.js (right): http://codereview.chromium.org/6223/diff/1/6#newcode292 Line 292: InstallFunctions($RegExp.prototype, DONT_ENUM, new $Array( Already commented on elsewhere. http://codereview.chromium.org/6223/diff/1/6#newcode342 Line 342: function NoOpSetter(ignored) {}; No need for the ; - let's clean that up soon. http://codereview.chromium.org/6223/diff/1/6#newcode363 Line 363: // A local scope to hide the loop index i. Is this local scope necessary anymore? It would be great to get rid of it if not. http://codereview.chromium.org/6223/diff/1/12 File src/string.js (right): http://codereview.chromium.org/6223/diff/1/12#newcode1 Line 1: // Copyright 2006-2008 the V8 project authors. All rights reserved. This file could actually use a slight rewrite to make it use the InstallFunctions helper. I'm sure that would generate less code too. http://codereview.chromium.org/6223/diff/1/11 File src/uri.js (right): http://codereview.chromium.org/6223/diff/1/11#newcode360 Line 360: InstallFunctions(global, DONT_ENUM, new $Array( Yes, yes. http://codereview.chromium.org/6223/diff/1/4 File src/v8natives.js (right): http://codereview.chromium.org/6223/diff/1/4#newcode78 Line 78: return $floor(string); Maybe add $floor to the requirements section like the other // const $xxx comments? http://codereview.chromium.org/6223 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
