Restructure Object::SetProperty and related functions.

Restructure Object::SetProperty and related functions.

This is in preparation of implementing Reflect.set.

R=rossberg
BUG=

Committed: https://crrev.com/d91e9e4f1dec000311d2bb8fae132fdc57faa95a
Cr-Commit-Position: refs/heads/master@{#31501}

[neis, 2015-10-12 14:21:22]

neis@chromium.org changed reviewers:
+ verwaest@chromium.org

[neis, 2015-10-14 13:31:13]

PTAL

[rossberg, 2015-10-14 15:59:06]

Looks good to me modulo comments, but Toon should have a look as well.

https://codereview.chromium.org/1394983005/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1394983005/diff/40001/src/objects.cc#newcode3701
src/objects.cc:3701: return result.IsJust() ? value : MaybeHandle<Object>();
This pattern occurs several times, maybe introduce a helper for it to streamline
it and avoid all the auxiliary declarations?

https://codereview.chromium.org/1394983005/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1394983005/diff/40001/src/objects.h#newcode2022
src/objects.h:2022: // Beware: This may return Nothing even if there is no
pending exception.
In what cases?

[Toon Verwaest, 2015-10-20 15:25:06]

https://codereview.chromium.org/1394983005/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1394983005/diff/40001/src/objects.cc#newcode905
src/objects.cc:905: if (call_fun == nullptr) return Just(true);
Shouldn't this case be unreachable (at least in the long run?) Should we have
ExecutableAccessorPairs with missing setter that are "writable"? If they aren't
writable, shouldn't we have bailed out already earlier?

There should probably be a TODO here to figure that out and fix.

https://codereview.chromium.org/1394983005/diff/40001/src/objects.cc#newcode1070
src/objects.cc:1070: return Nothing<bool>();
Nice. Wouldn't it be helpful to extract this to a helper function?

https://codereview.chromium.org/1394983005/diff/40001/src/objects.cc#newcode4444
src/objects.cc:4444: /*
I wouldn't add this commented out code. Open an issue and add it there if you
want to keep track of it.

[neis, 2015-10-21 14:42:25]

PTAL

https://codereview.chromium.org/1394983005/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1394983005/diff/40001/src/objects.cc#newcode905
src/objects.cc:905: if (call_fun == nullptr) return Just(true);
On 2015/10/20 15:25:06, Toon Verwaest wrote:
> Shouldn't this case be unreachable (at least in the long run?) Should we have
> ExecutableAccessorPairs with missing setter that are "writable"? If they
aren't
> writable, shouldn't we have bailed out already earlier?
> 
> There should probably be a TODO here to figure that out and fix.

Done.

https://codereview.chromium.org/1394983005/diff/40001/src/objects.cc#newcode1070
src/objects.cc:1070: return Nothing<bool>();
On 2015/10/20 15:25:06, Toon Verwaest wrote:
> Nice. Wouldn't it be helpful to extract this to a helper function?

Yes, and this should probably be used in many other places as well.  I'll do
that in a separate CL.

https://codereview.chromium.org/1394983005/diff/40001/src/objects.cc#newcode4444
src/objects.cc:4444: /*
On 2015/10/20 15:25:06, Toon Verwaest wrote:
> I wouldn't add this commented out code. Open an issue and add it there if you
> want to keep track of it.

Done.

https://codereview.chromium.org/1394983005/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1394983005/diff/40001/src/objects.h#newcode2022
src/objects.h:2022: // Beware: This may return Nothing even if there is no
pending exception.
On 2015/10/14 15:59:06, rossberg wrote:
> In what cases?

I actually changed the code now such that it behaves more regularly.

[rossberg, 2015-10-21 15:18:39]

LGTM

[Toon Verwaest, 2015-10-22 11:32:32]

lgtm

[neis, 2015-10-22 14:09:01]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-22 14:09:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394983005/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394983005/120001

[commit-bot: I haz the power, 2015-10-22 14:52:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-22 14:52:47]

Dry run: This issue passed the CQ dry run.

[neis, 2015-10-23 09:18:50]

The CQ bit was checked by neis@chromium.org

[neis, 2015-10-23 09:18:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org, verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1394983005/#ps120001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-10-23 09:19:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394983005/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394983005/120001

[commit-bot: I haz the power, 2015-10-23 09:39:44]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-23 09:41:34]

Patchset 7 (id:??) landed as
https://crrev.com/d91e9e4f1dec000311d2bb8fae132fdc57faa95a
Cr-Commit-Position: refs/heads/master@{#31501}

[Jakob Kummerow, 2015-10-27 15:51:51]

jkummerow@chromium.org changed reviewers:
+ jkummerow@chromium.org

[Jakob Kummerow, 2015-10-27 15:51:52]

https://codereview.chromium.org/1394983005/diff/120001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1394983005/diff/120001/src/objects.cc#newcode...
src/objects.cc:1061: UNREACHABLE();
We're hitting this in the wild (crbug.com/548194).