> On April 10, 2012, 2:38 a.m., Maks Orlovich wrote: > > kjs/object.h, line 431 > > <http://git.reviewboard.kde.org/r/104511/diff/1/?file=56158#file56158line431> > > > > It's OK to make API changes here; just make sure you fix every call > > site. Frankly, past experience shows that overloaded virtual groups like > > this are a mistake and hard to get right (there have been bugs caused with > > the getters, for example). > >
Oh good :) I would love to clean it, I was told that I should avoid API changes, but if you are ok with it, I will do it. > On April 10, 2012, 2:38 a.m., Maks Orlovich wrote: > > kjs/object_object.cpp, line 241 > > <http://git.reviewboard.kde.org/r/104511/diff/1/?file=56160#file56160line241> > > > > Faster: save return value of getObject() and check that for null. (Note > > that it matters here..) done > On April 10, 2012, 2:38 a.m., Maks Orlovich wrote: > > kjs/object_object.cpp, line 251 > > <http://git.reviewboard.kde.org/r/104511/diff/1/?file=56160#file56160line251> > > > > So this one shows the DontEnum ones? Joy. Yes that also includes the DontEnum ones. Is this just comment or is there something I overlook? (As this is an Issue that stops the "Ship it!") > On April 10, 2012, 2:38 a.m., Maks Orlovich wrote: > > kjs/object.cpp, line 496 > > <http://git.reviewboard.kde.org/r/104511/diff/1/?file=56159#file56159line496> > > > > This might actually show up on some benchmarks. I bet you can speed > > this up by making an appropriate choice of value for > > PropertyMap::IncludeDontEnumProperties -- basically making it a mask on > > attr, but that may be a bit brittle maintenance wise uhm really? for maintenance I would really say I am against it. And I doubt that it would really show up on some benchmarks... unless you are benchmarking huge objects/arrays with millions of properties > On April 10, 2012, 2:38 a.m., Maks Orlovich wrote: > > kjs/function.cpp, line 301 > > <http://git.reviewboard.kde.org/r/104511/diff/1/?file=56157#file56157line301> > > > > How should this react when the length property is altered? > > good question, I don't know, not explicitly covered as I can see. But I guess it would make sense that it shows 0 to length-1 parameters, if the they are all mapped. > On April 10, 2012, 2:38 a.m., Maks Orlovich wrote: > > kjs/function.cpp, line 308 > > <http://git.reviewboard.kde.org/r/104511/diff/1/?file=56157#file56157line308> > > > > Why convert to ident when isMapped will convert back to number again? > > Just add aversion that takes a number; similarly [] works on numbers > > already. (And perhaps fix up some of the sloppiness with signs there) Nononono!, I figured out that I have to be very careful with the number versions. The problem is the numbers have attributes too. If I overwrite it, it will only return the properties of the mapped object. In other places I need the properties for the numbers too. (defineOwnProperty for example) So I must not add a number version. - Bernd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/#review12281 ----------------------------------------------------------- On April 8, 2012, 9:14 p.m., Bernd Buschinski wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104511/ > ----------------------------------------------------------- > > (Updated April 8, 2012, 9:14 p.m.) > > > Review request for kdelibs. > > > Description > ------- > > KJS: Implement Object.prototypeOf, Object.keys, Object.getOwnPropertyNames > > NOTE: Array was left out on purpose, as currentl imeplementation does not > save attributes (next patch will fix this) > > keys&GetOwnPropertyNames requieres to change the > JSObject::getOwnPropertyNames implementation, > for future use a enum is better than than a bool, maybe there will be more > ways to include/exclude properties. > > All changes for khtml/ecma/ are to silense the -Woverloaded-virtual warnings > > > Diffs > ----- > > khtml/ecma/kjs_css.h aba44b8 > khtml/ecma/kjs_css.cpp e3e7417 > khtml/ecma/kjs_dom.h d0433c3 > khtml/ecma/kjs_dom.cpp 5fff7e3 > khtml/ecma/kjs_html.h 0f3f544c > khtml/ecma/kjs_html.cpp e3da95c > khtml/ecma/kjs_scriptable.h af5343c > khtml/ecma/kjs_scriptable.cpp 5d4ea68 > kjs/JSVariableObject.h a8f01eb > kjs/JSVariableObject.cpp b00ef76 > kjs/array_instance.h 3f2b630 > kjs/function.h 3757fe8 > kjs/function.cpp 5f39ae6 > kjs/object.h 047c242 > kjs/object.cpp c19122f > kjs/object_object.cpp 986f03f > kjs/property_map.h 6b127ff > kjs/property_map.cpp b2ff08e > kjs/string_object.h e890d52 > kjs/string_object.cpp 170e2f7 > > Diff: http://git.reviewboard.kde.org/r/104511/diff/ > > > Testing > ------- > > ecma script & daily surfing > > > Thanks, > > Bernd Buschinski > >
