Review Request: KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

Fri Apr 13 00:01:53 BST 2012

> On April 10, 2012, 2:54 a.m., Maks Orlovich wrote:
> > kjs/CMakeLists.txt, line 193
> > <http://git.reviewboard.kde.org/r/104515/diff/2/?file=56228#file56228line193>
> >
> >     where is this file?
> >

good question, sorry, I will include it in the next update

> On April 10, 2012, 2:54 a.m., Maks Orlovich wrote:
> > kjs/array_instance.cpp, line 55
> > <http://git.reviewboard.kde.org/r/104515/diff/2/?file=56231#file56231line55>
> >
> >     This is useless; it does what the default one would do (and why not copy constructor then?)

ok, dropped

> On April 10, 2012, 2:54 a.m., Maks Orlovich wrote:
> > kjs/object.cpp, line 168
> > <http://git.reviewboard.kde.org/r/104515/diff/2/?file=56235#file56235line168>
> >
> >     I don't understand all that you're doing here --- you invoke getOwnPropertySlot (which is virtual and may be overridden in subclasses, then getPropertyAttributes and then getDirect. And then get. What's the interface to the subclass here? Just how many look ups is it doing, etc.?
> >

uhm yes, thats a left over from trying to fight without virtual getDirect, will reduce it to getDirect and getPropertyAttributes.

> On April 10, 2012, 2:54 a.m., Maks Orlovich wrote:
> > kjs/object.cpp, line 444
> > <http://git.reviewboard.kde.org/r/104515/diff/2/?file=56235#file56235line444>
> >
> >     There is an explicit isUndefined check, but what about the ways in which toObject can fail?

in propertydescriptor is checked, or rather it is checked via implementsCall.
If it does not, we will not even got to this point.

> On April 10, 2012, 2:54 a.m., Maks Orlovich wrote:
> > kjs/operations.cpp, line 196
> > <http://git.reviewboard.kde.org/r/104515/diff/2/?file=56238#file56238line196>
> >
> >     Please point out how this isn't the same as strictEquals (due to the spec having two almost identical, but slightly different in handling of FP ops).
> >

Yes, its only different in the number check. The +0 != 0, and both NaN check.
in C++ NAN == NAN -> false (which is strictEquals)
for sameValue NAN == NAN -> true

same for +0 and 0. There are some testcases which rely on this

> On April 10, 2012, 2:54 a.m., Maks Orlovich wrote:
> > kjs/array_instance.cpp, line 42
> > <http://git.reviewboard.kde.org/r/104515/diff/2/?file=56231#file56231line42>
> >
> >     Could the array stuff perhaps be split out?

Splitted out? to where? I would like to keep the array implementation in the .cpp 
so that it can be changed without any ABI/API changes :)

> On April 10, 2012, 2:54 a.m., Maks Orlovich wrote:
> > kjs/object.h, line 458
> > <http://git.reviewboard.kde.org/r/104515/diff/2/?file=56234#file56234line458>
> >
> >     This is why I don't like the making of these virtual: getDirectLocation goes with these, so if these are virtual so should it be, but getDirectLocation is used by the Window object, so it's absolutely and utterly performance-critical.
> >     
> >     What I don't understand is: is there actually generic/polymorphic code that needs to call both the default and array's version?
> >

First of, Array::defineOwnProperty calls Object::defineOwnProperty for every array index, Array::defineOwnProperty does not store any data for array indexes.

And the last step of Object::defineOwnProperty is like everything is allowed.
Most Important is it manipulates the getter/setter directly.

And if Array::defineOwnProperty calls Object::defineOwnProperty, now (in the last step) we need to direct get it.
This might be an array index, so getDirect is needed.
After we changed it, we need to store it directly (with its new attributes), putDirect is needed.

Also without virtual putDirect we can't store anything correctly if the prototype has this property as getter/setter

In general putDirect is needed for every getter/setter put. As we can not update getter/setter using put.
And we also need to update the attributes, put checks that, but we just want to put them without checks in Object::defineOwnProperty

Back to getDirectLocation, luckily I don't need it. (guess that doesn't satisfy you)

- Bernd

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/#review12282
-----------------------------------------------------------

On April 9, 2012, 8:23 p.m., Bernd Buschinski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104515/
> -----------------------------------------------------------
> 
> (Updated April 9, 2012, 8:23 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> -------
> 
> KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty
> 
> This is a pretty big patch, to get Object.defineProperty perfect for ecmascript (for all tests that only use implemented stuff, all test that use Object.create for example will fail, as its not implemented)
> 
> PropertyDescriptor:
> Necessary for collectiong data, this introduce new CommonIdentifiers.h, this might requiere to rebuild khtml against new kjs, otherwise it might cause weird crashes (at least for me)
> 
> 
> object.h:
> Beside from adding new getPropertyDescriptor & getOwnPropertyDescriptor & defineOwnProperty, the important changes are making
> getPropertyAttributes, put/get/remove-Direct virtual.
> Why do I need that?
> Because put checks if the prototype already has property XYZ and uses it. Now imagine an array that got a setter-only property via a prototype. DefineProperty would try to use put, which uses the prototype property and it would fail. So all custom-data classes like Array need to implement/use put/get/remove-Direct.
> 
> 
> object.cpp:
> currently put on a setter-only property would always throw an exception, this is only correct for strict-mode, as we currently do not check for strict-mode it would make more sense to change it to default not throwing an exception.
> 
> 
> array.cpp/h:
> The old Array implementation did not store attributes for array indexes, I rewrote it to also store the attributes.
> + Bonus: also fix getOwnPropertyNames, as we now store attributes.
> + use new attributes, reject put/delete/enum if set
> 
> function.cpp (Arguments)
> changed the default attributes how parameter are stored, according to ECMA 10.6.11.b
> 
> 
> Rest is "just" the defineProperty implementation
> 
> 
> Diffs
> -----
> 
>   kjs/CMakeLists.txt 1188064 
>   kjs/CommonIdentifiers.h 8ee40e8 
>   kjs/array_instance.h 3f2b630 
>   kjs/array_instance.cpp fe9b8b4 
>   kjs/function.h 3757fe8 
>   kjs/function.cpp 5f39ae6 
>   kjs/object.h 047c242 
>   kjs/object.cpp c19122f 
>   kjs/object_object.cpp 986f03f 
>   kjs/operations.h f8a28c8 
>   kjs/operations.cpp d4c0066 
> 
> Diff: http://git.reviewboard.kde.org/r/104515/diff/
> 
> 
> Testing
> -------
> 
> ecmascript & daily surfing
> 
> used valgrind on many array testcases to check for possible memleaks
> 
> 
> Thanks,
> 
> Bernd Buschinski
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20120412/a9db2941/attachment.htm>