----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104515/#review13085 -----------------------------------------------------------
Still need more big picture stuff... kjs/object.h <http://git.reviewboard.kde.org/r/104515/#comment10212> The comment "only looks at the property map" is no longer true; and in general, if you're going to make so much of stuff virtual, this needs heavy comments on what gets called where. When do I need to implement getOwnPropertySlot, when do I need getDirect? getOwnPropertyDescriptor? getPropertyAttributes? Ditto for the put family. kjs/object.h <http://git.reviewboard.kde.org/r/104515/#comment10208> Should this particular override really be virtual? kjs/object.cpp <http://git.reviewboard.kde.org/r/104515/#comment10206> * kjs/object.cpp <http://git.reviewboard.kde.org/r/104515/#comment10205> This is rather hard to follow; please try consider giving higher-level description of what the cases do. kjs/object.cpp <http://git.reviewboard.kde.org/r/104515/#comment10200> more * inconsistency kjs/object.cpp <http://git.reviewboard.kde.org/r/104515/#comment10201> When is this ever not true when you're in this branch? kjs/object.cpp <http://git.reviewboard.kde.org/r/104515/#comment10202> * kjs/object.cpp <http://git.reviewboard.kde.org/r/104515/#comment10203> Not putDirect? kjs/object_object.cpp <http://git.reviewboard.kde.org/r/104515/#comment10199> This can throw, I think. Yes, we probably have zillions of bugs where we don't check ->hadException precisely. On that note, I would discourage using toString in exception error messages, for the same reason --- you can get an error produced just trying to construct them. Instead, you'll probably want to factor out the code in printInfo() in internal.cpp into something that can make you a string, and use that. kjs/operations.cpp <http://git.reviewboard.kde.org/r/104515/#comment10198> What I meant wrt to this function that it'd be helpful to put in how it's different from strictEquals in a comment, and why both exist (e.g. operator === vs. descriptor-type stuff) kjs/propertydescriptor.h <http://git.reviewboard.kde.org/r/104515/#comment10187> Are you sure you want them copyable? At any rate, you should never have a copy constructor but no operator= kjs/propertydescriptor.h <http://git.reviewboard.kde.org/r/104515/#comment10189> Hmm, I find these names a bit weird on PropertyDescriptor methods; it feels more like it should be setFromObject or such instead. (But feel free to ignore this suggestion, as it seems like it'd be a pain to change) kjs/propertydescriptor.h <http://git.reviewboard.kde.org/r/104515/#comment10190> One thing we don't need is even more inconsistency in * placement.. More importantly, looking at the code this has very different behavior from the above. Please give it a different name and document the difference. kjs/propertydescriptor.h <http://git.reviewboard.kde.org/r/104515/#comment10188> What do these mean? kjs/propertydescriptor.h <http://git.reviewboard.kde.org/r/104515/#comment10191> What does this do? kjs/propertydescriptor.h <http://git.reviewboard.kde.org/r/104515/#comment10192> Feels weird to have both an equalTo and a private(!) equality operator. kjs/propertydescriptor.cpp <http://git.reviewboard.kde.org/r/104515/#comment10193> Doesn't make sense in a function taking a JSObject*. Either make it take a JSValue* (which will simplify callers!) or remove. kjs/propertydescriptor.cpp <http://git.reviewboard.kde.org/r/104515/#comment10194> Looks redundant. kjs/propertydescriptor.cpp <http://git.reviewboard.kde.org/r/104515/#comment10196> This may be worth splitting up; it's certainly beyond my ability to read. At very least, you could have a helper for the foo == bar || (foo && bar && sameValue(exec, m_value, other.value()) pattern, too. kjs/propertydescriptor.cpp <http://git.reviewboard.kde.org/r/104515/#comment10195> I think parentheses would be helpful here. kjs/propertydescriptor.cpp <http://git.reviewboard.kde.org/r/104515/#comment10197> Unused, I think (and different semantics from equalTo)? - Maks Orlovich On April 20, 2012, 11:55 a.m., Bernd Buschinski wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104515/ > ----------------------------------------------------------- > > (Updated April 20, 2012, 11:55 a.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 > kjs/propertydescriptor.h PRE-CREATION > kjs/propertydescriptor.cpp PRE-CREATION > > 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 > >
