Feng, I'm not done yet, but I thought I would give you my initial comments now since I'll be away next week.
My main comment is that it would be nice if we could use interceptors on the ApiGlobalObject to do all the forwarding. If the ApiGlobalObject is only accessed in window.foo style programming, interceptors might not be a performance problem and we would avoid the special casing in the objects.cc functions. I like the fact that we are now back to having a one-to-one correspondence between (inner) global objects and global contexts. -- Mads http://codereview.chromium.org/7366/diff/1/8 File src/api.cc (right): http://codereview.chromium.org/7366/diff/1/8#newcode2298 Line 2298: if (IsDeadCheck("v8::Context::RestoreSecurityToken()")) return; RestoreSecurityToken -> UseDefaultSecurityToken. http://codereview.chromium.org/7366/diff/1/8#newcode2358 Line 2358: i::Bootstrapper::DetachGlobal(context); Why is DetachGlobal on the bootstrapper? It does not appear to be used during bootstrapping. http://codereview.chromium.org/7366/diff/1/18 File src/bootstrapper.cc (right): http://codereview.chromium.org/7366/diff/1/18#newcode351 Line 351: void Bootstrapper::DetachGlobal(Handle<Context> env) { Move this and it's helper to handles.[h,cpp]? http://codereview.chromium.org/7366/diff/1/13 File src/bootstrapper.h (right): http://codereview.chromium.org/7366/diff/1/13#newcode50 Line 50: static void DetachGlobal(Handle<Context> env); Move this to handles.[h,cpp]? http://codereview.chromium.org/7366/diff/1/31 File src/factory.h (right): http://codereview.chromium.org/7366/diff/1/31#newcode275 Line 275: ApiInstanceType type = JavaScriptObject); Indentation: align with other argument. http://codereview.chromium.org/7366/diff/1/7 File src/macro-assembler-ia32.cc (left): http://codereview.chromium.org/7366/diff/1/7#oldcode486 Line 486: // Restore scratch register to be the map of the object. We This comment makes sense to me. Leave it in? http://codereview.chromium.org/7366/diff/1/7 File src/macro-assembler-ia32.cc (right): http://codereview.chromium.org/7366/diff/1/7#newcode485 Line 485: // Pass in the prototype JSGlobalObject Out-dated comment? http://codereview.chromium.org/7366/diff/1/7#newcode582 Line 582: // Read the first work and compare to global_context_map(), work -> word http://codereview.chromium.org/7366/diff/1/16 File src/objects.cc (right): http://codereview.chromium.org/7366/diff/1/16#newcode1372 Line 1372: return JSObject::cast(proto)->LocalLookup(name, result); Should this be LocalLookupRealNamedProperty? Also, can we avoid all this special casing for the ApiGlobalObject by installing interceptors on the ApiGlobalObject that forward all access to the prototype? It would be great to avoid all these extra checks. http://codereview.chromium.org/7366/diff/1/28 File src/objects.h (right): http://codereview.chromium.org/7366/diff/1/28#newcode2748 Line 2748: // An ApiGlobalObject can be reinitialized by keeping its identity. by keeping -> which will preserve? http://codereview.chromium.org/7366/diff/1/26 File src/runtime.cc (right): http://codereview.chromium.org/7366/diff/1/26#newcode3710 Line 3710: static Object* Runtime_GlobalThis(Arguments args) { Should this be called GlobalReceiver for consistency? 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'm not sure I understand these changes. Did the old code cause problems because of undetectable objects? http://codereview.chromium.org/7366/diff/1/9 File src/stub-cache-ia32.cc (right): http://codereview.chromium.org/7366/diff/1/9#newcode888 Line 888: // Generate store field code. Trashes then ame register. ame -> name http://codereview.chromium.org/7366/diff/1/17 File src/top.cc (right): http://codereview.chromium.org/7366/diff/1/17#newcode477 Line 477: if (receiver->IsApiGlobalObject()) { Create an inline helper function for this shared part of MayNamedAccess and MayIndexedAccess? http://codereview.chromium.org/7366/diff/1/22 File src/top.h (right): http://codereview.chromium.org/7366/diff/1/22#newcode233 Line 233: // a builtin object, or a js globla object. globla -> global http://codereview.chromium.org/7366 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
