Make the runtime entry for setting/changing accessors "atomic".

Make the runtime entry for setting/changing accessors "atomic".

Previously, there were 1 or 2 calls to the runtime when accessors were changed
or set. This doesn't really work well with property attributes, leading to some
hacks and complicates things even further when trying to share maps in presence
of accessors. Therefore, the runtime entry now takes the full triple (getter,
setter, attributes), where the getter and/or the setter can be null in case they
shouldn't be changed.

For now, we do basically the same on the native side as we did before on the
JavaScript side, but this will change in future CLs, the current CL is already
large enough.

Note that object literals with a getter and a setter for the same property still
do 2 calls, but this is a little bit more tricky to fix and will be handled in a
separate CL.

Committed: https://code.google.com/p/v8/source/detail?r=10956

[Michael Starzinger, 2012-03-07 12:49:14]

LGTM (if all comments addressed).

https://chromiumcodereview.appspot.com/9616016/diff/4001/src/runtime.cc
File src/runtime.cc (left):

https://chromiumcodereview.appspot.com/9616016/diff/4001/src/runtime.cc#oldco...
src/runtime.cc:4324: 
Please leave this empty line in (or at least keep layout in sync with
Runtime_DefineOrRedefineDataProperty). I cleaned it up a month ago.

https://chromiumcodereview.appspot.com/9616016/diff/4001/src/runtime.cc
File src/runtime.cc (right):

https://chromiumcodereview.appspot.com/9616016/diff/4001/src/runtime.cc#newco...
src/runtime.cc:4318: static bool IsValidAccessor(Object* obj) {
Since this is only used as part of an assertion, I would prefer to have the
condition inline instead of in a function.

https://chromiumcodereview.appspot.com/9616016/diff/4001/src/runtime.cc#newco...
src/runtime.cc:4334: CONVERT_ARG_HANDLE_CHECKED(String, name, 1);
Is there a particular reason you handlified the name (and not the getter and
setter)?

https://chromiumcodereview.appspot.com/9616016/diff/4001/src/runtime.cc#newco...
src/runtime.cc:4349: getter =
FixedArray::cast(array->elements())->get(GETTER_INDEX);
I assume that this goes away once the above TODO is addressed? Then I am fine
with this intermediate step.

[Sven Panne, 2012-03-07 13:23:32]

http://codereview.chromium.org/9616016/diff/4001/src/runtime.cc
File src/runtime.cc (left):

http://codereview.chromium.org/9616016/diff/4001/src/runtime.cc#oldcode4324
src/runtime.cc:4324: 
On 2012/03/07 12:49:14, Michael Starzinger wrote:
> Please leave this empty line in (or at least keep layout in sync with
> Runtime_DefineOrRedefineDataProperty). I cleaned it up a month ago.

Done.

http://codereview.chromium.org/9616016/diff/4001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/9616016/diff/4001/src/runtime.cc#newcode4318
src/runtime.cc:4318: static bool IsValidAccessor(Object* obj) {
On 2012/03/07 12:49:14, Michael Starzinger wrote:
> Since this is only used as part of an assertion, I would prefer to have the
> condition inline instead of in a function.

As discussed offline, it's OK to keep this funtion, because RUNTIME_ASSERT uses
its argument even in release mode.

http://codereview.chromium.org/9616016/diff/4001/src/runtime.cc#newcode4334
src/runtime.cc:4334: CONVERT_ARG_HANDLE_CHECKED(String, name, 1);
On 2012/03/07 12:49:14, Michael Starzinger wrote:
> Is there a particular reason you handlified the name (and not the getter and
> setter)?

The Object class was missing an IsObject predicate, which I added, so we can
consistently use the CONVERT_ARG_HANDLE_CHECKED macro now.

http://codereview.chromium.org/9616016/diff/4001/src/runtime.cc#newcode4349
src/runtime.cc:4349: getter =
FixedArray::cast(array->elements())->get(GETTER_INDEX);
On 2012/03/07 12:49:14, Michael Starzinger wrote:
> I assume that this goes away once the above TODO is addressed? Then I am fine
> with this intermediate step.

Yes, this will vanish, it is just a temporary C++ re-write of the previous
corresponding JavaScript code.