[runtime] Support Symbols in KeyAccumulator

[runtime] Support Symbols in KeyAccumulator

BUG=

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

[Camillo Bruni, 2015-11-03 12:47:41]

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

[commit-bot: I haz the power, 2015-11-03 12:47:49]

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

[commit-bot: I haz the power, 2015-11-03 13:21:32]

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

[commit-bot: I haz the power, 2015-11-03 13:21:33]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel/builds/11427)

[Camillo Bruni, 2015-11-03 14:46:17]

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

[commit-bot: I haz the power, 2015-11-03 14:46:22]

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

[Camillo Bruni, 2015-11-03 14:49:54]

cbruni@chromium.org changed reviewers:
+ jkummerow@chromium.org, neis@chromium.org

[Camillo Bruni, 2015-11-03 14:49:55]

PTAL

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

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

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

Dry run: This issue passed the CQ dry run.

[Jakob Kummerow, 2015-11-03 15:33:37]

LGTM, but a bunch of comments are outdated.

https://codereview.chromium.org/1425403002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1425403002/diff/20001/src/objects.cc#newcode7565
src/objects.cc:7565: // |protoLengths_| and the available element keys in the
corresponding bucket
nit: outdated comment

https://codereview.chromium.org/1425403002/diff/20001/src/objects.cc#newcode7618
src/objects.cc:7618: int KeyAccumulator::GetLength() { return length_; }
nit: a simple getter like this should be called just "length()" and be defined
inline in the header file (unless you have a reason to deviate from that rule of
thumb).

https://codereview.chromium.org/1425403002/diff/20001/src/objects.cc#newcode7767
src/objects.cc:7767: // Sort and remove duplicated from the current elements
level and adjust.
nit: s/.//, also s/duplicated/duplicate entries/?

https://codereview.chromium.org/1425403002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1425403002/diff/20001/src/objects.h#newcode10760
src/objects.h:10760: // collected in |properties_|, a single OrderedHashSet. To
separate the keys per
nit: outdated comment

https://codereview.chromium.org/1425403002/diff/20001/src/objects.h#newcode10785
src/objects.h:10785: // Jump to the next level, pushing the current
|levelLength_| to
nit: s/levelLength_/level_lengths_/.

I count six more variable names in comments below that need similar updates.

https://codereview.chromium.org/1425403002/diff/20001/src/objects.h#newcode10793
src/objects.h:10793: 
nit: one empty line is enough

[Jakob Kummerow, 2015-11-03 15:40:00]

one more comment.

https://codereview.chromium.org/1425403002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1425403002/diff/20001/src/objects.h#newcode10774
src/objects.h:10774: bool AddKey(uint32_t key);
nit: most of these methods can probably be private, right?

[Camillo Bruni, 2015-11-03 16:03:51]

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

[commit-bot: I haz the power, 2015-11-03 16:04:01]

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

[commit-bot: I haz the power, 2015-11-03 16:44:09]

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

[commit-bot: I haz the power, 2015-11-03 16:44:09]

Dry run: This issue passed the CQ dry run.

[neis, 2015-11-04 12:49:27]

https://codereview.chromium.org/1425403002/diff/40001/src/key-accumulator.h
File src/key-accumulator.h (right):

https://codereview.chromium.org/1425403002/diff/40001/src/key-accumulator.h#n...
src/key-accumulator.h:27: //
Can you update the comment to say something about symbols?

https://codereview.chromium.org/1425403002/diff/40001/src/key-accumulator.h#n...
src/key-accumulator.h:70: // insertion order.
Some variable names in the comments are wrong.

https://codereview.chromium.org/1425403002/diff/40001/src/key-accumulator.h#n...
src/key-accumulator.h:75: // (elements + properties) in the current level.
These comments need to be updated too.

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

https://codereview.chromium.org/1425403002/diff/40001/src/objects.cc#newcode1...
src/objects.cc:14818: if (key->FilterKey(filter)) continue;
This check of attributes and FilterKey occurs twice more in the code below. 
Maybe move it into a small function.

[Camillo Bruni, 2015-11-04 13:09:30]

https://codereview.chromium.org/1425403002/diff/40001/src/key-accumulator.h
File src/key-accumulator.h (right):

https://codereview.chromium.org/1425403002/diff/40001/src/key-accumulator.h#n...
src/key-accumulator.h:27: //
On 2015/11/04 at 12:49:27, neis wrote:
> Can you update the comment to say something about symbols?

yes, missed that.

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

https://codereview.chromium.org/1425403002/diff/40001/src/objects.cc#newcode1...
src/objects.cc:14818: if (key->FilterKey(filter)) continue;
On 2015/11/04 at 12:49:27, neis wrote:
> This check of attributes and FilterKey occurs twice more in the code below. 
Maybe move it into a small function.

you're right, I'll put in on my Friday cleanup list :)

[Camillo Bruni, 2015-11-04 13:18:02]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2015-11-04 13:18:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1425403002/#ps80001
(title: "updating + clarifying")

[commit-bot: I haz the power, 2015-11-04 13:18:09]

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

[commit-bot: I haz the power, 2015-11-04 14:05:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-04 14:05:47]

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