[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Review Request: KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty
From: "Rolf Eike Beer" <kde () opensource ! sf-tec ! de>
Date: 2012-06-01 21:06:50
Message-ID: 20120601210650.4375.49767 () vidsolbach ! de
[Download RAW message or body]
> On April 29, 2012, 6:26 p.m., Maks Orlovich wrote:
> > kjs/object.cpp, line 433
> > <http://git.reviewboard.kde.org/r/104515/diff/4/?file=58277#file58277line433>
> >
> > more * inconsistency
>
> Bernd Buschinski wrote:
> I don't get the problem here, could you please explain?
GetterSetterImp *gs -> GetterSetterImp* gs
- Rolf Eike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/#review13085
-----------------------------------------------------------
On June 1, 2012, 12:34 p.m., Bernd Buschinski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104515/
> -----------------------------------------------------------
>
> (Updated June 1, 2012, 12:34 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
> 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
>
>
[Attachment #3 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/104515/">http://git.reviewboard.kde.org/r/104515/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 29th, 2012, 6:26 p.m., <b>Maks Orlovich</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: \
collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: \
4px 8px; text-align: left;"> <a \
href="http://git.reviewboard.kde.org/r/104515/diff/4/?file=58277#file58277line433" style="color: black; \
font-weight: bold; text-decoration: underline;">kjs/object.cpp</a> <span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">bool \
JSObject::defineOwnProperty(ExecState* exec, const Identifier& propertyName, PropertyDescriptor& \
desc, bool shouldThrow)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font \
size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; \
margin: 0; "></pre></td> <th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px \
solid #C0C0C0;" align="right"><font size="2">433</font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">GetterSetterImp</span> \
<span class="o">*</span><span class="n">gs</span> <span class="o">=</span> <span class="k">new</span> \
<span class="n">GetterSetterImp</span><span class="p">();</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;">more * inconsistency</pre> </blockquote>
<p>On April 30th, 2012, 7:11 p.m., <b>Bernd Buschinski</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;">I don't get the problem here, could you please explain?</pre> \
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; \
white-space: -o-pre-wrap; word-wrap: break-word;">GetterSetterImp *gs -> GetterSetterImp* gs</pre> <br \
/>
<p>- Rolf Eike</p>
<br />
<p>On June 1st, 2012, 12:34 p.m., Bernd Buschinski wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: \
left top; background-repeat: repeat-x; border: 1px black solid;"> <tr>
<td>
<div>Review request for kdelibs.</div>
<div>By Bernd Buschinski.</div>
<p style="color: grey;"><i>Updated June 1, 2012, 12:34 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid \
#b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">ecmascript & daily surfing
used valgrind on many array testcases to check for possible memleaks</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kjs/CMakeLists.txt <span style="color: grey">(1188064)</span></li>
<li>kjs/CommonIdentifiers.h <span style="color: grey">(8ee40e8)</span></li>
<li>kjs/array_instance.h <span style="color: grey">(3f2b630)</span></li>
<li>kjs/array_instance.cpp <span style="color: grey">(fe9b8b4)</span></li>
<li>kjs/function.h <span style="color: grey">(3757fe8)</span></li>
<li>kjs/function.cpp <span style="color: grey">(5f39ae6)</span></li>
<li>kjs/object.h <span style="color: grey">(047c242)</span></li>
<li>kjs/object.cpp <span style="color: grey">(c19122f)</span></li>
<li>kjs/object_object.cpp <span style="color: grey">(986f03f)</span></li>
<li>kjs/operations.h <span style="color: grey">(f8a28c8)</span></li>
<li>kjs/operations.cpp <span style="color: grey">(d4c0066)</span></li>
<li>kjs/propertydescriptor.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>kjs/propertydescriptor.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104515/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic