Chromium Code Reviews
Help | Chromium Project | Sign in
(23)

Issue 11338048: Handle Object.observe notifications for setting Array.length (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by adamk
Modified:
1 year, 5 months ago
Reviewers:
rossberg, rafaelw
CC:
v8-dev_googlegroups.com
Base URL:
git@github.com:rafaelw/v8@master
Visibility:
Public.

Description

Handle Object.observe notifications for setting Array.length

Also handles notification of deleted properties when an array
is truncated by setting length.

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

Patch Set 1 #

Patch Set 2 : Better support for deleting callback properties #

Patch Set 3 : Fix sparse arrays #

Patch Set 4 : Merged with trunk #

Total comments: 6

Patch Set 5 : Respond to review, merged to trunk #

Total comments: 3

Patch Set 6 : Make use of unsigned wraparound in for loop #

Patch Set 7 : Add a test case for really truncating to zero #

Patch Set 8 : Clarify test todo #

Unified diffs Side-by-side diffs Delta from patch set Stats (+106 lines, -24 lines) Lint Patch
M src/accessors.cc View 1 2 3 4 5 6 3 chunks +47 lines, -2 lines 0 comments 0 errors Download
M src/objects.h View 1 2 3 4 2 chunks +6 lines, -5 lines 0 comments 0 errors Download
M src/objects.cc View 1 2 3 4 7 chunks +18 lines, -17 lines 0 comments 0 errors Download
M test/mjsunit/harmony/object-observe.js View 1 2 3 4 5 6 7 1 chunk +35 lines, -0 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 10
adamk
Just sending out so folks can take a look, this will likely be rolled up ...
1 year, 5 months ago #1
adamk
Ready for review.
1 year, 5 months ago #2
rossberg
LGTM with nits. https://chromiumcodereview.appspot.com/11338048/diff/7001/src/accessors.cc File src/accessors.cc (right): https://chromiumcodereview.appspot.com/11338048/diff/7001/src/accessors.cc#newcode109 src/accessors.cc:109: while (end_index && end_index-- > new_length) ...
1 year, 5 months ago #3
adamk
https://chromiumcodereview.appspot.com/11338048/diff/7001/src/accessors.cc File src/accessors.cc (right): https://chromiumcodereview.appspot.com/11338048/diff/7001/src/accessors.cc#newcode109 src/accessors.cc:109: while (end_index && end_index-- > new_length) { Done, but ...
1 year, 5 months ago #4
rossberg
https://chromiumcodereview.appspot.com/11338048/diff/10002/src/accessors.cc File src/accessors.cc (right): https://chromiumcodereview.appspot.com/11338048/diff/10002/src/accessors.cc#newcode108 src/accessors.cc:108: for (uint32_t len = old_length; len > new_length; --len) ...
1 year, 5 months ago #5
adamk
https://chromiumcodereview.appspot.com/11338048/diff/10002/src/accessors.cc File src/accessors.cc (right): https://chromiumcodereview.appspot.com/11338048/diff/10002/src/accessors.cc#newcode108 src/accessors.cc:108: for (uint32_t len = old_length; len > new_length; --len) ...
1 year, 5 months ago #6
rossberg
https://chromiumcodereview.appspot.com/11338048/diff/10002/src/accessors.cc File src/accessors.cc (right): https://chromiumcodereview.appspot.com/11338048/diff/10002/src/accessors.cc#newcode108 src/accessors.cc:108: for (uint32_t len = old_length; len > new_length; --len) ...
1 year, 5 months ago #7
adamk
On 2012/11/08 15:11:22, adamk wrote: > https://chromiumcodereview.appspot.com/11338048/diff/10002/src/accessors.cc > File src/accessors.cc (right): > > https://chromiumcodereview.appspot.com/11338048/diff/10002/src/accessors.cc#newcode108 > ...
1 year, 5 months ago #8
adamk
Added a real truncation test case and added TODOs for arrays with dictionary elements.
1 year, 5 months ago #9
rossberg
1 year, 5 months ago #10
LGTM again, gonna land it.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6