Use the LookupIterator for SetElement and friends

Use the LookupIterator for SetElement and friends

BUG=v8:4137
LOG=n

Committed: https://crrev.com/52f44a83533af11d2dae10ce19b80510b4b3c19a
Cr-Commit-Position: refs/heads/master@{#28946}

[Toon Verwaest, 2015-06-09 16:46:27]

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

[Toon Verwaest, 2015-06-09 16:46:27]

ptal

[Igor Sheludko, 2015-06-11 13:44:26]

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

https://codereview.chromium.org/1172683003/diff/120001/src/objects.cc#newcode315
src/objects.cc:315: if (structure->IsAccessorInfo()) {
IsExecutableAccessorInfo()?

https://codereview.chromium.org/1172683003/diff/120001/src/objects.cc#newcode...
src/objects.cc:3194: if (found) return result;
Missing break; ?

https://codereview.chromium.org/1172683003/diff/120001/src/objects.cc#newcode...
src/objects.cc:3365: if (receiver->IsJSArray()) {
You dropped CheckArrayAbuse() calls. Was it done intentionally?

https://codereview.chromium.org/1172683003/diff/120001/src/objects.cc#newcode...
src/objects.cc:12811: return;
nit: Do we need return; here?

https://codereview.chromium.org/1172683003/diff/120001/src/objects.cc#newcode...
src/objects.cc:12881: Handle<JSArray>::cast(receiver)->length())) {
What about adding DCHECK(receiver->map()->is_observed()); or a comment to ease
understanding.

https://codereview.chromium.org/1172683003/diff/120001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1172683003/diff/120001/src/objects.h#newcode1892
src/objects.h:1892: static void SetSloppyArgumentsElements(Handle<JSObject>
object,
"SetSloppyArgumentsElement" to be consistent.

https://codereview.chromium.org/1172683003/diff/120001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1172683003/diff/120001/test/cctest/test-api.c...
test/cctest/test-api.cc:13269: global->ForceSet(access_property,
v8::Int32::New(isolate, 8));
I think it makes sense to add the above comment here.

[Toon Verwaest, 2015-06-11 14:26:50]

Addressed comments

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

https://codereview.chromium.org/1172683003/diff/120001/src/objects.cc#newcode315
src/objects.cc:315: if (structure->IsAccessorInfo()) {
On 2015/06/11 13:44:25, Igor Sheludko wrote:
> IsExecutableAccessorInfo()?

It's in a weird state since it's the only subclass. IsAccessorInfo calls
IsExecutableAccessorInfo... We should get rid of that distinction later.

https://codereview.chromium.org/1172683003/diff/120001/src/objects.cc#newcode...
src/objects.cc:3194: if (found) return result;
On 2015/06/11 13:44:25, Igor Sheludko wrote:
> Missing break; ?

Done.

https://codereview.chromium.org/1172683003/diff/120001/src/objects.cc#newcode...
src/objects.cc:3365: if (receiver->IsJSArray()) {
On 2015/06/11 13:44:25, Igor Sheludko wrote:
> You dropped CheckArrayAbuse() calls. Was it done intentionally?

Done.

https://codereview.chromium.org/1172683003/diff/120001/src/objects.cc#newcode...
src/objects.cc:12811: return;
On 2015/06/11 13:44:25, Igor Sheludko wrote:
> nit: Do we need return; here?

Done.

https://codereview.chromium.org/1172683003/diff/120001/src/objects.cc#newcode...
src/objects.cc:12881: Handle<JSArray>::cast(receiver)->length())) {
On 2015/06/11 13:44:25, Igor Sheludko wrote:
> What about adding DCHECK(receiver->map()->is_observed()); or a comment to ease
> understanding.

Done.

https://codereview.chromium.org/1172683003/diff/120001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1172683003/diff/120001/test/cctest/test-api.c...
test/cctest/test-api.cc:13269: global->ForceSet(access_property,
v8::Int32::New(isolate, 8));
On 2015/06/11 13:44:25, Igor Sheludko wrote:
> I think it makes sense to add the above comment here.

Done.

[Igor Sheludko, 2015-06-11 14:38:19]

lgtm with nits:

https://codereview.chromium.org/1172683003/diff/120001/src/elements.h
File src/elements.h (right):

https://codereview.chromium.org/1172683003/diff/120001/src/elements.h#newcode49
src/elements.h:49: }
Empty line is missing.

https://codereview.chromium.org/1172683003/diff/120001/src/elements.h#newcode195
src/elements.h:195: virtual Handle<Object> Set(Handle<JSObject> holder, uint32_t
key,
Please make it protected.

https://codereview.chromium.org/1172683003/diff/120001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1172683003/diff/120001/src/objects.h#newcode1650
src/objects.h:1650: enum SetPropertyMode {
It looks like this enum is not used anymore.

[Toon Verwaest, 2015-06-11 14:41:39]

Thanks!
Addressed final comments and rebased. Landing.

[Toon Verwaest, 2015-06-11 14:42:04]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2015-06-11 14:42:04]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/1172683003/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-11 14:42:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1172683003/200001

[commit-bot: I haz the power, 2015-06-11 15:07:06]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-06-11 15:07:23]

Patchset 11 (id:??) landed as
https://crrev.com/52f44a83533af11d2dae10ce19b80510b4b3c19a
Cr-Commit-Position: refs/heads/master@{#28946}

[Michael Achenbach, 2015-06-11 17:10:00]

machenbach@chromium.org changed reviewers:
+ machenbach@chromium.org, vogelheim@chromium.org

[Michael Achenbach, 2015-06-11 17:10:01]

This breaks webkit unit tests:
http://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064%20%2...

Can it be reverted?