Re: Review Request: KJS: Implement Object.prototypeOf, Object.keys, Object.getOwnPropertyNames
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/#review13422 --- This review has been submitted with commit 654fa56348058ab8155a1ff87002771d87d63768 by Bernd Buschinski to branch KDE/4.8. - Commit Hook On April 30, 2012, 6:59 p.m., Bernd Buschinski wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/ --- (Updated April 30, 2012, 6:59 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) keysGetOwnPropertyNames 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/debugger/localvariabledock.cpp 289c910 khtml/ecma/kjs_css.h aba44b8 khtml/ecma/kjs_css.cpp e3e7417 khtml/ecma/kjs_data.cpp df6fa85 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/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/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
Re: Review Request: KJS: Implement Object.prototypeOf, Object.keys, Object.getOwnPropertyNames
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/#review13093 --- Ship it! Why not move the bit flags to property_map.h then? - Maks Orlovich On April 26, 2012, 4:34 p.m., Bernd Buschinski wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/ --- (Updated April 26, 2012, 4:34 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) keysGetOwnPropertyNames 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 - kjs/array_instance.cpp fe9b8b4 kjs/function.h 3757fe8 kjs/array_instance.h 3f2b630 kjs/JSVariableObject.cpp b00ef76 khtml/ecma/kjs_scriptable.h af5343c khtml/ecma/kjs_scriptable.cpp 5d4ea68 kjs/JSVariableObject.h a8f01eb khtml/ecma/debugger/localvariabledock.cpp 289c910 khtml/ecma/kjs_css.h aba44b8 khtml/ecma/kjs_css.cpp e3e7417 khtml/ecma/kjs_data.cpp df6fa85 khtml/ecma/kjs_dom.h d0433c3 khtml/ecma/kjs_dom.cpp 5fff7e3 khtml/ecma/kjs_html.h 0f3f544c khtml/ecma/kjs_html.cpp e3da95c 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
Re: Review Request: KJS: Implement Object.prototypeOf, Object.keys, Object.getOwnPropertyNames
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/ --- (Updated April 26, 2012, 4:34 p.m.) Review request for kdelibs. Changes --- - droped default argument - fixed typo that make compile in debug fail checkEnumerable is problematic to make it a inline function. As this would requiere to inlcude object.h in property_map.h, but object.h already includes property_map.h. 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) keysGetOwnPropertyNames 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 (updated) - kjs/array_instance.cpp fe9b8b4 kjs/function.h 3757fe8 kjs/array_instance.h 3f2b630 kjs/JSVariableObject.cpp b00ef76 khtml/ecma/kjs_scriptable.h af5343c khtml/ecma/kjs_scriptable.cpp 5d4ea68 kjs/JSVariableObject.h a8f01eb khtml/ecma/debugger/localvariabledock.cpp 289c910 khtml/ecma/kjs_css.h aba44b8 khtml/ecma/kjs_css.cpp e3e7417 khtml/ecma/kjs_data.cpp df6fa85 khtml/ecma/kjs_dom.h d0433c3 khtml/ecma/kjs_dom.cpp 5fff7e3 khtml/ecma/kjs_html.h 0f3f544c khtml/ecma/kjs_html.cpp e3da95c 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
Re: Review Request: KJS: Implement Object.prototypeOf, Object.keys, Object.getOwnPropertyNames
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/#review12764 --- Ship it! Please fix the inlining thing. I am also not sure about the repetition of the default value for 3rd argument for getOwnPropertyNames all over the place. May be just drop the default argument entirely? That would have the bonus of making sure that the call is it right everywhere. (For example for the call inside the debugger it might actually be handy to include DontEnum properties as well..) kjs/property_map.cpp http://git.reviewboard.kde.org/r/104511/#comment9977 This needs to be inline in the header file. - Maks Orlovich On April 20, 2012, 11:48 a.m., Bernd Buschinski wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/ --- (Updated April 20, 2012, 11:48 a.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) keysGetOwnPropertyNames 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/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/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
Re: Review Request: KJS: Implement Object.prototypeOf, Object.keys, Object.getOwnPropertyNames
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/ --- (Updated April 20, 2012, 11:48 a.m.) Review request for kdelibs. Changes --- Fix return type of Object.keys Object.getOwnPropertyNames to be a proper Array 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) keysGetOwnPropertyNames 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 (updated) - 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/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/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
Re: Review Request: KJS: Implement Object.prototypeOf, Object.keys, Object.getOwnPropertyNames
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/ --- (Updated April 19, 2012, 8:35 p.m.) Review request for kdelibs. Changes --- Updated Array getOwnPropertyNames to 3 parameter version, so it will work again alone after this patch (the following patch will change it again anyway, but its good to keep it working after every commit) 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) keysGetOwnPropertyNames 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 (updated) - 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/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/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
Re: Review Request: KJS: Implement Object.prototypeOf, Object.keys, Object.getOwnPropertyNames
On April 15, 2012, 2:55 p.m., Maks Orlovich wrote: kjs/function.cpp, line 302 http://git.reviewboard.kde.org/r/104511/diff/1/?file=56157#file56157line302 re: if length changed --- I think this should be using -size from indexToNameMap rather than the length property. Why? Because setting length on an arguments object of a 2-argument function doesn't make [0] and [1] DontEnum. ok, fixed, introduced new public getter size() to IndexToNameMap On April 15, 2012, 2:55 p.m., Maks Orlovich wrote: kjs/object_object.cpp, line 262 http://git.reviewboard.kde.org/r/104511/diff/1/?file=56160#file56160line262 This is actually my big objection --- there is too much duplication between the two, when it could be one conditional. Can it be you looked the old r1? On April 15, 2012, 2:55 p.m., Maks Orlovich wrote: kjs/property_map.cpp, line 680 http://git.reviewboard.kde.org/r/104511/diff/1/?file=56162#file56162line680 I wonder if a nice inline helper for this (!(e-attributes DontEnum) || mode == IncludeDontEnumProperties) is possible? It's just a hair too ugly for the number of places it's in. introduced static bool checkEnumerable(unsigned int attr, PropertyMode mode); which gcc inlines with -O1 and higher, so no extra call, beside from -O0 - Bernd --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/#review12471 --- On April 13, 2012, 9:38 a.m., Bernd Buschinski wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/ --- (Updated April 13, 2012, 9:38 a.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) keysGetOwnPropertyNames 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
Re: Review Request: KJS: Implement Object.prototypeOf, Object.keys, Object.getOwnPropertyNames
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/ --- (Updated April 16, 2012, 5:51 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) keysGetOwnPropertyNames 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 (updated) - 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
Re: Review Request: KJS: Implement Object.prototypeOf, Object.keys, Object.getOwnPropertyNames
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) Bernd Buschinski wrote: 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. Huh? There is no difference between numbers and idents when it comes to being property names. Any place where we use numbers for that internally is just an optimization; and at any rate indexToNameMap certainly operates on numbers as inputs. - Maks --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/#review12281 --- On April 13, 2012, 9:38 a.m., Bernd Buschinski wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/ --- (Updated April 13, 2012, 9:38 a.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) keysGetOwnPropertyNames 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
Re: Review Request: KJS: Implement Object.prototypeOf, Object.keys, Object.getOwnPropertyNames
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/#review12471 --- khtml/ecma/kjs_css.h http://git.reviewboard.kde.org/r/104511/#comment9787 kjs/function.cpp http://git.reviewboard.kde.org/r/104511/#comment9788 re: if length changed --- I think this should be using -size from indexToNameMap rather than the length property. Why? Because setting length on an arguments object of a 2-argument function doesn't make [0] and [1] DontEnum. kjs/object.h http://git.reviewboard.kde.org/r/104511/#comment9792 KJS is not a public API, so changing things is certainly possible.. kjs/object_object.cpp http://git.reviewboard.kde.org/r/104511/#comment9789 This is actually my big objection --- there is too much duplication between the two, when it could be one conditional. kjs/property_map.h http://git.reviewboard.kde.org/r/104511/#comment9790 Again, why KDE5? You can change it now. As an another option, in cases like these, I think it actually helps to make getEnumerablePropertyNames inline, as it makes the relation between the two clear. kjs/property_map.cpp http://git.reviewboard.kde.org/r/104511/#comment9791 I wonder if a nice inline helper for this (!(e-attributes DontEnum) || mode == IncludeDontEnumProperties) is possible? It's just a hair too ugly for the number of places it's in. - Maks Orlovich On April 13, 2012, 9:38 a.m., Bernd Buschinski wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/ --- (Updated April 13, 2012, 9:38 a.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) keysGetOwnPropertyNames 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
Re: Review Request: KJS: Implement Object.prototypeOf, Object.keys, Object.getOwnPropertyNames
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) keysGetOwnPropertyNames 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
Re: Review Request: KJS: Implement Object.prototypeOf, Object.keys, Object.getOwnPropertyNames
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104511/#review12281 --- kjs/function.cpp http://git.reviewboard.kde.org/r/104511/#comment9642 How should this react when the length property is altered? kjs/function.cpp http://git.reviewboard.kde.org/r/104511/#comment9643 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) kjs/object.h http://git.reviewboard.kde.org/r/104511/#comment9644 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). kjs/object.cpp http://git.reviewboard.kde.org/r/104511/#comment9645 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 kjs/object_object.cpp http://git.reviewboard.kde.org/r/104511/#comment9647 For these comments, could you include the spec version? Reason: we have a whole bunch of the around already... From something older than the 3rd edition. (Dunno if from 2nd or 1st..) kjs/object_object.cpp http://git.reviewboard.kde.org/r/104511/#comment9648 Faster: save return value of getObject() and check that for null. (Note that it matters here..) kjs/object_object.cpp http://git.reviewboard.kde.org/r/104511/#comment9649 So this one shows the DontEnum ones? Joy. kjs/object_object.cpp http://git.reviewboard.kde.org/r/104511/#comment9650 I think you could merge this implementation with the above; the only difference is what gets passed to getOwnPropertyNames as last argument, isn't it? This is way too much to duplicate.. - Maks Orlovich 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) keysGetOwnPropertyNames 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
