Overall, this change LGTM. I'm not as scared by the change as I thought I would be. I do feel that the outer/inner global and the ApiGlobal/JSGlobal names need to be used more consistently and that it might make sense to use names like JSGlobalObject and JSGlobalProxy (for the API one)? I don't know. I like the notion that there is a global object (JSGlobalObject) and that it may be proxied for security reasons. Inner/outer seems too weird for me (maybe internal/external would be better) and JSGlobal vs ApiGlobal rubs me the wrong way too. Sorry.
I really think we need to land this change sooner rather than later, so if you feel that updating comments and renaming classes and variables is too time consuming, I'd have no problem with you submitting the code as it is now and then we can work on figuring the rest out in a set of future changes. How many changes does this require in the binding code? We have to make sure that we can easily merge it to the release branch. http://codereview.chromium.org/7366/diff/1/5 File include/v8.h (right): http://codereview.chromium.org/7366/diff/1/5#newcode2061 Line 2061: /** Detaches the global object from its context before Move the comment (starting with Detaches) to the next line to better match the other multi-line doc comments. http://codereview.chromium.org/7366/diff/1/8 File src/api.cc (right): http://codereview.chromium.org/7366/diff/1/8#newcode1940 Line 1940: new_map->set_needs_access_check(); Do we really want a boolean default value for the set_needs_access_check method? I would much rather be more explicit and maybe turn the true/false values into an enum with proper names for the values like DONT_CHECK_ON_ACCESS and CHECK_ON_ACCESS. http://codereview.chromium.org/7366/diff/1/42 File src/codegen-ia32.cc (right): http://codereview.chromium.org/7366/diff/1/42#newcode2660 Line 2660: Why is there extra whitespace here? http://codereview.chromium.org/7366/diff/1/7 File src/macro-assembler-ia32.cc (right): http://codereview.chromium.org/7366/diff/1/7#newcode570 Line 570: push(holder_reg); We should definitely try to get rid of this push/pop either by using another scratch register or by finding a way of providing more direct access to the security token. http://codereview.chromium.org/7366/diff/1/33 File src/objects-debug.cc (right): http://codereview.chromium.org/7366/diff/1/33#newcode573 Line 573: // Make sure that this object has no properties, elements Terminate comment with a period. http://codereview.chromium.org/7366/diff/1/38 File src/objects-inl.h (right): http://codereview.chromium.org/7366/diff/1/38#newcode357 Line 357: #ifdef DEBUG How about rewriting this to something ala: bool result = <tag and type check>; ASSERT(!result || IsAccessCheckNeeded()); http://codereview.chromium.org/7366/diff/1/38#newcode371 Line 371: return IsHeapObject() && I think we should cache the instance type in a local variable here. Unrelated to your change of course. http://codereview.chromium.org/7366/diff/1/16 File src/objects.cc (right): http://codereview.chromium.org/7366/diff/1/16#newcode1255 Line 1255: Whitespace? Why? http://codereview.chromium.org/7366/diff/1/28 File src/objects.h (right): http://codereview.chromium.org/7366/diff/1/28#newcode2821 Line 2821: Do we need whitespace here? http://codereview.chromium.org/7366/diff/1/24 File src/runtime.js (right): http://codereview.chromium.org/7366/diff/1/24#newcode77 Line 77: } else if (x == null || IS_UNDEFINED(x)) { I don't think this makes sense at all because undefined is equal (==) to null. Please remove this. http://codereview.chromium.org/7366/diff/1/17 File src/top.cc (right): http://codereview.chromium.org/7366/diff/1/17#newcode586 Line 586: GetProperty(Top::builtins(), key)); Maybe this fits on one line now? http://codereview.chromium.org/7366/diff/1/4 File test/cctest/test-api.cc (right): http://codereview.chromium.org/7366/diff/1/4#newcode4831 Line 4831: // TODO (fqian): when running "cctest test-api", the initial count is 2, File a bug report and add the issue number here instead of your username - like TODO(121): When running... http://codereview.chromium.org/7366 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
