Hi Thanks, Fixed most of your comments and renamed ApiGlobalObject to JSGlobalProxy.
Filed 3 bugs for TODO's, and reverted two unrelated changes. 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 On 2008/10/21 10:50:02, Kasper Lund wrote: > Move the comment (starting with Detaches) to the next line to better match the > other multi-line doc comments. Done. 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(); Names must have confused you. FunctionTemplate::needs_access_check(bool) takes a boolean argument, and Map::needs_access_check() does not take arguments. I agree that naming is confusing, so I am going to rename Map::needs_access_check() to Map::is_access_check_needed(). Unfortunately, FunctionTemplate uses a macro DECL_BOOLEAN_ACCESSORS which can only take a bool value, and there are several other accessors declared in the same way, so I think a bool argument is fine after renaming Map::needs_access_check(). On 2008/10/21 10:50:02, Kasper Lund wrote: > 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/7 File src/macro-assembler-ia32.cc (right): http://codereview.chromium.org/7366/diff/1/7#newcode570 Line 570: push(holder_reg); I agree. Another approach is to profile how often this case is hit in real word (cross-frame accesses in the same domain). The most common case, in-frame accesses won't hit this case. I am going to add a TODO there. On 2008/10/21 10:50:02, Kasper Lund wrote: > 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/7#newcode570 Line 570: push(holder_reg); I filed a bug and added a TODO(119) On 2008/10/21 10:50:02, Kasper Lund wrote: > 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/38 File src/objects-inl.h (right): http://codereview.chromium.org/7366/diff/1/38#newcode357 Line 357: #ifdef DEBUG On 2008/10/21 10:50:02, Kasper Lund wrote: > How about rewriting this to something ala: > bool result = <tag and type check>; > ASSERT(!result || IsAccessCheckNeeded()); Done. http://codereview.chromium.org/7366/diff/1/38#newcode371 Line 371: return IsHeapObject() && On 2008/10/21 10:50:02, Kasper Lund wrote: > I think we should cache the instance type in a local variable here. Unrelated to > your change of course. Done. http://codereview.chromium.org/7366/diff/1/16 File src/objects.cc (right): http://codereview.chromium.org/7366/diff/1/16#newcode1255 Line 1255: On 2008/10/21 10:50:02, Kasper Lund wrote: > Whitespace? Why? Done. http://codereview.chromium.org/7366/diff/1/28 File src/objects.h (right): http://codereview.chromium.org/7366/diff/1/28#newcode2821 Line 2821: On 2008/10/21 10:50:02, Kasper Lund wrote: > Do we need whitespace here? Done. 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'd better not include this change in this change list. I am going to revert it and re-do it in a separate CB. This is related to issue 102. On 2008/10/21 10:50:02, Kasper Lund wrote: > 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)); On 2008/10/21 10:50:02, Kasper Lund wrote: > Maybe this fits on one line now? Done. 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, On 2008/10/21 10:50:02, Kasper Lund wrote: > File a bug report and add the issue number here instead of your username - like > TODO(121): When running... Done. http://codereview.chromium.org/7366 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
