[api] Add more parameters to Object::GetPropertyNames

[api] Add more parameters to Object::GetPropertyNames

Expose more or less the full functionality of the KeyAccumulator in the API:
- use the PropertyFilter introduced for GetOwnPropertyNames
- use KeyCollectionLimit for OWN_ONLY or INLCUDE_PROTOS
- use IndexFilter to eithe SKIP_INDICES or INCLUDE_INDICES

Rewire Object::GetOwnPropertyNames to use GetPropertyNames.

BUG=chromium:148757
Committed: https://crrev.com/63efe9e4164a68888a8ee74aae75a31fe3149cab
Cr-Commit-Position: refs/heads/master@{#36595}

[Camillo Bruni, 2016-05-23 19:07:24]

Description was changed from

==========
[api] Add more parameters to Object::GetPropertyNames

Expose more or less the full functionality of the KeyAccumulator in the API:
- use the PropertyFilter introduced for GetOwnPropertyNames
- use KeyCollectionLimit for OWN_ONLY or INLCUDE_PROTOS
- use IndexFilter to eithe SKIP_INDICES or INCLUDE_INDICES

Rewire Object::GetOwnPropertyNames to use GetPropertyNames.

BUG=chromium:148757
==========

to

==========
[api] Add more parameters to Object::GetPropertyNames

Expose more or less the full functionality of the KeyAccumulator in the API:
- use the PropertyFilter introduced for GetOwnPropertyNames
- use KeyCollectionLimit for OWN_ONLY or INLCUDE_PROTOS
- use IndexFilter to eithe SKIP_INDICES or INCLUDE_INDICES

Rewire Object::GetOwnPropertyNames to use GetPropertyNames.

BUG=chromium:148757
==========

[Camillo Bruni, 2016-05-25 09:58:31]

cbruni@chromium.org changed reviewers:
+ ishell@chromium.org, yangguo@chromium.org

[Camillo Bruni, 2016-05-25 09:58:31]

yangguo@ please have a look at debug*
ishell@ PTAL at the rest

[Igor Sheludko, 2016-05-25 14:52:57]

https://codereview.chromium.org/2002203002/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2002203002/diff/40001/include/v8.h#newcode2634
include/v8.h:2634: enum KeysCollectionLimit { OWN_ONLY, INCLUDE_PROTOS };
WDYT about KeyCollectionMode? Why not enum class?

https://codereview.chromium.org/2002203002/diff/40001/src/keys.h
File src/keys.h (right):

https://codereview.chromium.org/2002203002/diff/40001/src/keys.h#newcode34
src/keys.h:34: KeyAccumulator(Isolate* isolate, KeysCollectionLimit type,
I would rename |type| here, below and above as well.

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

https://codereview.chromium.org/2002203002/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:15214:
static_cast<v8::PropertyFilter>(v8::ONLY_ENUMERABLE | v8::SKIP_SYMBOLS);
It would be nice to check the case where we don't skip symbols. And other
filtering modes too.

[Camillo Bruni, 2016-05-27 12:58:37]

PTAL again, more work on the tests

https://codereview.chromium.org/2002203002/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2002203002/diff/40001/include/v8.h#newcode2634
include/v8.h:2634: enum KeysCollectionLimit { OWN_ONLY, INCLUDE_PROTOS };
On 2016/05/25 at 14:52:56, Igor Sheludko wrote:
> WDYT about KeyCollectionMode? Why not enum class?

right, seems like a better choice.

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

https://codereview.chromium.org/2002203002/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:15214:
static_cast<v8::PropertyFilter>(v8::ONLY_ENUMERABLE | v8::SKIP_SYMBOLS);
On 2016/05/25 at 14:52:56, Igor Sheludko wrote:
> It would be nice to check the case where we don't skip symbols. And other
filtering modes too.

thought so... just gonna make the tests horribly complicated :D

[Igor Sheludko, 2016-05-27 13:53:42]

https://codereview.chromium.org/2002203002/diff/100001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2002203002/diff/100001/include/v8.h#newcode2634
include/v8.h:2634: enum class KeyCollectionMode { OWN_ONLY, INCLUDE_PROTOS };
I think we use kBlaBlah naming convention for enum classes.

https://codereview.chromium.org/2002203002/diff/100001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2002203002/diff/100001/src/api.cc#newcode3885
src/api.cc:3885: isolate, static_cast<i::KeyCollectionMode>(mode),
We need STATIC_ASSERTs that v8::KeyCollectionMode is compatible with
i::KeyCollectionMode.

https://codereview.chromium.org/2002203002/diff/100001/src/api.cc#newcode3886
src/api.cc:3886: static_cast<i::PropertyFilter>(property_filter));
Same here.

[Camillo Bruni, 2016-05-27 16:16:40]

PTAL again

[Yang, 2016-05-30 09:25:27]

yangguo@chromium.org changed reviewers:
+ jochen@chromium.org

[Yang, 2016-05-30 09:25:28]

+Jochen

https://codereview.chromium.org/2002203002/diff/160001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2002203002/diff/160001/include/v8.h#newcode2634
include/v8.h:2634: enum class KeyCollectionMode { kOwnOnly, kIncludePrototypes
};
Do we actually have a consistent way of naming enums? we have both UPPER_CASE
and kCamelCase enum names in v8.h. Initially, I thought UPPER_CASE are enums
that we use as bitfields. But then you got StackTrace::StackTraceOptions and
PropertyAttribute.

:(

https://codereview.chromium.org/2002203002/diff/160001/include/v8.h#newcode2785
include/v8.h:2785: V8_WARN_UNUSED_RESULT MaybeLocal<Array> GetPropertyNames(
Why don't we just extend this with default args?

[Camillo Bruni, 2016-05-30 11:25:45]

ishel, PTAL again

https://codereview.chromium.org/2002203002/diff/160001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2002203002/diff/160001/include/v8.h#newcode2634
include/v8.h:2634: enum class KeyCollectionMode { kOwnOnly, kIncludePrototypes
};
On 2016/05/30 at 09:25:28, Yang wrote:
> Do we actually have a consistent way of naming enums? we have both UPPER_CASE
and kCamelCase enum names in v8.h. Initially, I thought UPPER_CASE are enums
that we use as bitfields. But then you got StackTrace::StackTraceOptions and
PropertyAttribute.
> 
> :(

yeah I was confused as well. Apparently "enum" used uppercase, while "enum
class" use the k prefix...

https://codereview.chromium.org/2002203002/diff/160001/include/v8.h#newcode2785
include/v8.h:2785: V8_WARN_UNUSED_RESULT MaybeLocal<Array> GetPropertyNames(
On 2016/05/30 at 09:25:28, Yang wrote:
> Why don't we just extend this with default args?

Just followed the rest of the api here...

[Igor Sheludko, 2016-05-30 13:35:37]

lgtm with nits:

https://codereview.chromium.org/2002203002/diff/200001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2002203002/diff/200001/include/v8.h#newcode2634
include/v8.h:2634: enum class KeyCollectionMode { kOwnOnly, kIncludePrototypes
};
This is a public API, we probably need some comments here.

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

https://codereview.chromium.org/2002203002/diff/200001/test/cctest/test-api.c...
test/cctest/test-api.cc:15091: if (names[i] == nullptr) continue;
Suggestion: we can probably check that this value is actually a symbol.

[jochen (gone - plz use gerrit), 2016-05-30 14:28:49]

i like enum classes with kCamelCase, I think we should use that going forward

[Yang, 2016-05-30 15:03:36]

On 2016/05/30 14:28:49, jochen wrote:
> i like enum classes with kCamelCase, I think we should use that going forward

lgtm

[Camillo Bruni, 2016-05-30 15:22:06]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-05-30 15:22:06]

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/2002203002/#ps220001
(title: "addressing nits")

[commit-bot: I haz the power, 2016-05-30 15:22:12]

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

[commit-bot: I haz the power, 2016-05-30 15:53:02]

Description was changed from

==========
[api] Add more parameters to Object::GetPropertyNames

Expose more or less the full functionality of the KeyAccumulator in the API:
- use the PropertyFilter introduced for GetOwnPropertyNames
- use KeyCollectionLimit for OWN_ONLY or INLCUDE_PROTOS
- use IndexFilter to eithe SKIP_INDICES or INCLUDE_INDICES

Rewire Object::GetOwnPropertyNames to use GetPropertyNames.

BUG=chromium:148757
==========

to

==========
[api] Add more parameters to Object::GetPropertyNames

Expose more or less the full functionality of the KeyAccumulator in the API:
- use the PropertyFilter introduced for GetOwnPropertyNames
- use KeyCollectionLimit for OWN_ONLY or INLCUDE_PROTOS
- use IndexFilter to eithe SKIP_INDICES or INCLUDE_INDICES

Rewire Object::GetOwnPropertyNames to use GetPropertyNames.

BUG=chromium:148757
==========

[commit-bot: I haz the power, 2016-05-30 15:53:03]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-05-30 15:54:17]

Description was changed from

==========
[api] Add more parameters to Object::GetPropertyNames

Expose more or less the full functionality of the KeyAccumulator in the API:
- use the PropertyFilter introduced for GetOwnPropertyNames
- use KeyCollectionLimit for OWN_ONLY or INLCUDE_PROTOS
- use IndexFilter to eithe SKIP_INDICES or INCLUDE_INDICES

Rewire Object::GetOwnPropertyNames to use GetPropertyNames.

BUG=chromium:148757
==========

to

==========
[api] Add more parameters to Object::GetPropertyNames

Expose more or less the full functionality of the KeyAccumulator in the API:
- use the PropertyFilter introduced for GetOwnPropertyNames
- use KeyCollectionLimit for OWN_ONLY or INLCUDE_PROTOS
- use IndexFilter to eithe SKIP_INDICES or INCLUDE_INDICES

Rewire Object::GetOwnPropertyNames to use GetPropertyNames.

BUG=chromium:148757

Committed: https://crrev.com/63efe9e4164a68888a8ee74aae75a31fe3149cab
Cr-Commit-Position: refs/heads/master@{#36595}
==========

[commit-bot: I haz the power, 2016-05-30 15:54:19]

Patchset 12 (id:??) landed as
https://crrev.com/63efe9e4164a68888a8ee74aae75a31fe3149cab
Cr-Commit-Position: refs/heads/master@{#36595}