[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: "Maks Orlovich" <maksim () kde ! org>
Date: 2012-04-29 18:26:16
Message-ID: 20120429182616.29537.14031 () vidsolbach ! de
[Download RAW message or body]
-----------------------------------------------------------
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
>
>
[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 />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Still need more big \
picture stuff...</pre> <br />
<div>
<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=58276#file58276line452" \
style="color: black; font-weight: bold; text-decoration: underline;">kjs/object.h</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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
">namespace KJS {</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">447</font></th> <td bgcolor="#ffffff" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// to \
look up in the prototype, it might already exist there)</span></pre></td> <th \
bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">452</font></th> <td bgcolor="#ffffff" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="c1">// to look up in the prototype, it might already exist \
there)</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
</div>
<br />
<div>
<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=58276#file58276line466" \
style="color: black; font-weight: bold; text-decoration: underline;">kjs/object.h</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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
">namespace KJS {</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">461</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="kt">void</span> <span class="n">putDirect</span><span class="p">(</span><span \
class="k">const</span> <span class="n">Identifier</span> <span \
class="o">&</span><span class="n">propertyName</span><span class="p">,</span> \
<span class="kt">int</span> <span class="n">value</span><span class="p">,</span> \
<span class="kt">int</span> <span class="n">attr</span> <span class="o">=</span> \
<span class="mi">0</span><span class="p">);</span></pre></td> <th bgcolor="#e9eaa8" \
style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">466</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="hl"> \
</span><span class="k"><span class="hl">virtual</span></span> <span \
class="kt">void</span> <span class="n">putDirect</span><span class="p">(</span><span \
class="k">const</span> <span class="n">Identifier</span> <span \
class="o">&</span><span class="n">propertyName</span><span class="p">,</span> \
<span class="kt">int</span> <span class="n">value</span><span class="p">,</span> \
<span class="kt">int</span> <span class="n">attr</span> <span class="o">=</span> \
<span class="mi">0</span><span class="p">);</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Should this \
particular override really be virtual?</pre> </div>
<br />
<div>
<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#file58277line130" \
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::getPropertyDescriptor(ExecState* exec, const Identifier& \
propertyName, PropertyDescriptor& desc)</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">130</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">JSValue</span> <span class="o">*</span><span class="n">prototype</span> \
<span class="o">=</span> <span class="n">object</span><span \
class="o">-></span><span class="n">prototype</span><span \
class="p">();</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">*</pre> \
</div> <br />
<div>
<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#file58277line424" \
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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void \
JSObject::defineSetter(ExecState*, const Identifier& propertyName, JSObject* \
setterFunc)</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">424</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="kt">bool</span> <span class="n">JSObject</span><span class="o">::</span><span \
class="n">defineOwnProperty</span><span class="p">(</span><span \
class="n">ExecState</span><span class="o">*</span> <span class="n">exec</span><span \
class="p">,</span> <span class="k">const</span> <span \
class="n">Identifier</span><span class="o">&</span> <span \
class="n">propertyName</span><span class="p">,</span> <span \
class="n">PropertyDescriptor</span><span class="o">&</span> <span \
class="n">desc</span><span class="p">,</span> <span class="kt">bool</span> <span \
class="n">shouldThrow</span><span class="p">)</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is \
rather hard to follow; please try consider giving higher-level description of what \
the cases do.</pre> </div>
<br />
<div>
<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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void \
JSObject::defineSetter(ExecState*, const Identifier& propertyName, JSObject* \
setterFunc)</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="margin-left: 2em; 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> </div>
<br />
<div>
<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#file58277line436" \
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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void \
JSObject::defineSetter(ExecState*, const Identifier& propertyName, JSObject* \
setterFunc)</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">436</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> \
<span class="n">newAttr</span> <span class="o">|=</span> <span \
class="n">GetterSetter</span><span class="p">;</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">When is \
this ever not true when you're in this branch?</pre> </div>
<br />
<div>
<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#file58277line481" \
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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void \
JSObject::defineSetter(ExecState*, const Identifier& propertyName, JSObject* \
setterFunc)</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">481</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="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">*</pre> \
</div> <br />
<div>
<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#file58277line501" \
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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void \
JSObject::defineSetter(ExecState*, const Identifier& propertyName, JSObject* \
setterFunc)</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">501</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> \
<span class="n">put</span><span class="p">(</span><span class="n">exec</span><span \
class="p">,</span> <span class="n">propertyName</span><span class="p">,</span> <span \
class="n">desc</span><span class="p">.</span><span class="n">value</span><span \
class="p">()</span> <span class="o">?</span> <span class="n">desc</span><span \
class="p">.</span><span class="n">value</span><span class="p">()</span> <span \
class="o">:</span> <span class="n">jsUndefined</span><span class="p">(),</span> <span \
class="n">newAttr</span><span class="p">);</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Not \
putDirect?</pre> </div>
<br />
<div>
<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=58278#file58278line288" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kjs/object_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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
">JSValue *ObjectObjectFuncImp::callAsFunction(ExecState* exec, JSObject*, const \
List& args)</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">288</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">UString</span> <span class="n">name</span> <span class="o">=</span> <span \
class="n">args</span><span class="p">[</span><span class="mi">1</span><span \
class="p">]</span><span class="o">-></span><span class="n">toString</span><span \
class="p">(</span><span class="n">exec</span><span class="p">);</span></pre></td> \
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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. \
</pre> </div>
<br />
<div>
<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=58280#file58280line195" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kjs/operations.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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">bool \
strictEqual(ExecState *exec, JSValue *v1, JSValue *v2)</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">195</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="c1">//EMCA 9.12</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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) </pre>
</div>
<br />
<div>
<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=58281#file58281line36" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kjs/propertydescriptor.h</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; ">public:</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">36</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">PropertyDescriptor</span><span class="p">(</span><span \
class="n">PropertyDescriptor</span><span class="o">&</span> <span \
class="n">other</span><span class="p">);</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Are you \
sure you want them copyable? At any rate, you should never have a copy constructor \
but no operator=</pre> </div>
<br />
<div>
<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=58281#file58281line42" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kjs/propertydescriptor.h</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; ">public:</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">42</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">bool</span> <span class="n">setPropertyDescriptor</span><span \
class="p">(</span><span class="n">ExecState</span><span class="o">*</span> <span \
class="n">exec</span><span class="p">,</span> <span class="n">JSObject</span><span \
class="o">*</span> <span class="n">obj</span><span class="p">);</span></pre></td> \
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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)</pre> </div>
<br />
<div>
<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=58281#file58281line43" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kjs/propertydescriptor.h</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; ">public:</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">43</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">bool</span> <span class="n">setPropertyDescriptor</span><span \
class="p">(</span><span class="n">ExecState</span><span class="o">*</span> <span \
class="n">exec</span><span class="p">,</span> <span class="n">JSValue</span> <span \
class="o">*</span><span class="n">value</span><span class="p">,</span> <span \
class="kt">unsigned</span> <span class="kt">int</span> <span \
class="n">attributes</span><span class="p">);</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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. </pre> </div>
<br />
<div>
<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=58281#file58281line49" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kjs/propertydescriptor.h</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; ">public:</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">49</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">bool</span> <span class="n">enumerableSet</span><span class="p">()</span> \
<span class="k">const</span><span class="p">;</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">What do \
these mean?</pre> </div>
<br />
<div>
<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=58281#file58281line70" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kjs/propertydescriptor.h</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; ">public:</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">70</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="kt">unsigned</span> <span class="kt">int</span> <span \
class="n">attributesWithOverride</span><span class="p">(</span><span \
class="n">PropertyDescriptor</span><span class="o">&</span> <span \
class="n">other</span><span class="p">)</span> <span class="k">const</span><span \
class="p">;</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">What does \
this do?</pre> </div>
<br />
<div>
<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=58281#file58281line73" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kjs/propertydescriptor.h</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; ">public:</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">73</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">bool</span> <span class="n">operator</span><span class="o">==</span><span \
class="p">(</span><span class="n">PropertyDescriptor</span><span \
class="o">&</span> <span class="n">other</span><span class="p">)</span> <span \
class="k">const</span><span class="p">;</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Feels weird \
to have both an equalTo and a private(!) equality operator.</pre> </div>
<br />
<div>
<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=58282#file58282line89" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kjs/propertydescriptor.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 PropertyDescriptor::setPropertyDescriptor(ExecState *exec, JSObject* \
obj)</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">89</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">if</span> <span class="p">(</span><span class="o">!</span><span \
class="n">obj</span><span class="o">-></span><span class="n">isObject</span><span \
class="p">())</span> <span class="p">{</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Doesn't \
make sense in a function taking a JSObject*. Either make it take a JSValue* (which \
will simplify callers!) or remove.</pre> </div>
<br />
<div>
<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=58282#file58282line114" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kjs/propertydescriptor.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 PropertyDescriptor::setPropertyDescriptor(ExecState *exec, JSObject* \
obj)</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">114</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> \
<span class="n">getter</span> <span class="o">=</span> <span \
class="n">jsUndefined</span><span class="p">();</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Looks \
redundant.</pre> </div>
<br />
<div>
<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=58282#file58282line263" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kjs/propertydescriptor.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 PropertyDescriptor::setPropertyDescriptor(ExecState *exec, JSObject* \
obj)</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">263</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> \
<span class="n">attributes</span><span class="p">()</span> <span class="o">==</span> \
<span class="n">other</span><span class="p">.</span><span \
class="n">attributes</span><span class="p">());</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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. </pre>
</div>
<br />
<div>
<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=58282#file58282line272" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kjs/propertydescriptor.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 PropertyDescriptor::setPropertyDescriptor(ExecState *exec, JSObject* \
obj)</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">272</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">if</span> <span class="p">(</span><span class="n">sharedSeen</span> <span \
class="o">&</span> <span class="n">WritableSet</span> <span \
class="o">&&</span> <span class="n">mismatch</span> <span \
class="o">&</span> <span class="n">ReadOnly</span><span \
class="p">)</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think \
parentheses would be helpful here.</pre> </div>
<br />
<div>
<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=58282#file58282line282" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kjs/propertydescriptor.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 PropertyDescriptor::setPropertyDescriptor(ExecState *exec, JSObject* \
obj)</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">282</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="kt">bool</span> <span class="n">PropertyDescriptor</span><span \
class="o">::</span><span class="k">operator</span><span class="o">==</span><span \
class="p">(</span><span class="n">PropertyDescriptor</span><span \
class="o">&</span> <span class="n">other</span><span class="p">)</span> <span \
class="k">const</span></pre></td> </tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Unused, I \
think (and different semantics from equalTo)?</pre> </div>
<br />
<p>- Maks</p>
<br />
<p>On April 20th, 2012, 11:55 a.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 April 20, 2012, 11:55 a.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