All layout tests passed fine. yours, anton.
On Fri, May 21, 2010 at 2:20 PM, <[email protected]> wrote: > Thanks a lot for comments, Mads. > > > http://codereview.chromium.org/2123012/diff/1/6 > File src/objects.cc (right): > > http://codereview.chromium.org/2123012/diff/1/6#newcode2774 > src/objects.cc:2774: || Top::MayNamedAccess(this, name, > v8::ACCESS_SET)); > On 2010/05/21 07:49:57, Mads Ager wrote: >> >> Do the types match here? I think you check that HAS access is allowed > > and here >> >> you assert that SET access is allowed. Should you be checking SET > > access below? > > You're right, thanks a lot for spotting this. And see below as well. > > http://codereview.chromium.org/2123012/diff/1/6#newcode2879 > src/objects.cc:2879: !Top::MayNamedAccess(this, name, v8::ACCESS_HAS)) { > On 2010/05/21 07:49:57, Mads Ager wrote: >> >> Should you check for SET instead of HAS here? > > I think you're right. I copied this check from the code above (see ln. > 2855-2856) in the patched code. > > For now I changed checks to SET for both DefineAccessors. Seems to work > fine with v8's (I cannot run layout tests on my linux box any more, > going to investigate), but there might be some implications to security > which I miss. > > http://codereview.chromium.org/2123012/diff/1/6#newcode2935 > src/objects.cc:2935: if (result.IsProperty() && (result.IsReadOnly() || > result.IsDontDelete())) { > On 2010/05/21 07:49:57, Mads Ager wrote: >> >> Why do we check for DontDelete when setting an accessor property? I > > guess it is >> >> up to your interpretation of the action of setting an accessor but to > > me that >> >> does not seem like a deletion. If this is the behavior that we want, > > we should >> >> make sure to document it. > >> Also, I don't understand the comment below. What are the two > > DefineAccessors you >> >> are talking about? __defineGetter__ and __defineSetter__ defined > > accessors? >> >> Example? > > I started with the simple IsReadOnly. However I had to add IsDontDelete > for the following scenario (covered in the first unit test): > > Object.defineProperty(obj, 'foo', { ... configurable: false }) > > Now I think that overriding 'foo' should fail. That seems consistent > with ES5 (see 8.6.1, [[Configurable]] If false, attempts to delete the > property, change the property to be an accessor property, or change its > attributes (other then [[Value]]) will fail. > > The principal question for me is if we'd like to treat API's accessors > more like JS accessor or JS funky properties. My feeling is we > currently sitting in between, so I'd be glad to hear what you think the > right way is. > > And !configurable is encoded as DontDelete with our current > PropertyAttribute model. > > I am talking about JSObject::DefineAccessor(String* name, bool > is_getter, JSFunction* fun, PropertyAttributes attributes) which used by > both Object.defineProperty and __defineGetter__. > > Alas, Object.defineProperty and __defineGetter__ apparently do somewhat > different checks. > > http://codereview.chromium.org/2123012/diff/1/6#newcode2936 > src/objects.cc:2936: // That's somewhat unfortunate that two > DefineAccessors > On 2010/05/21 07:49:57, Mads Ager wrote: >> >> That's -> It is > > Done. > > http://codereview.chromium.org/2123012/diff/1/6#newcode2937 > src/objects.cc:2937: // diff here, but another one performs DON_DELETE > check > On 2010/05/21 07:49:57, Mads Ager wrote: >> >> another one -> the other one > > Done. > > http://codereview.chromium.org/2123012/diff/1/6#newcode5966 > src/objects.cc:5966: Handle<Object> number = > Factory::NewNumberFromUint(index); > On 2010/05/21 07:49:57, Mads Ager wrote: >> >> This code does not seem GC safe. |self| and |holder_handle| are raw > > pointers, >> >> but the number allocation could cause a heap allocation and thereby a > > garbage >> >> collection. > > Stupid me, thanks a lot. > > http://codereview.chromium.org/2123012/diff/1/6#newcode5994 > src/objects.cc:5994: return 0; > On 2010/05/21 07:49:57, Mads Ager wrote: >> >> NULL or maybe just undefined? > > Thanks. Here and in original GetPropertyWithCallback. > > http://codereview.chromium.org/2123012/show > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
