Follow-up to bug 1007334 (which renames the trap that JS code can use, but leaves the ProxyHandler C++ method alone) and bug 645416 (symbols).

Ugh. So in the spec "keys" sometimes means all property names (as in [[OwnPropertyKeys]] and sometimes means enumerable ones (as in Object.keys())? :(

And Reflect.ownKeys() matches the former, for extra confusion. Allen, is it too late in terms of compat to make "keys" have a consistent meaning at least for Reflect.ownKeys and Object.keys?

(In reply to Boris Zbarsky [:bz] from comment #1) > Ugh. So in the spec "keys" sometimes means all property names (as in > [[OwnPropertyKeys]] and sometimes means enumerable ones (as in > Object.keys())? :( Yes. The new term for "string or symbol used as the name of a property" is "property key" and it's used everywhere in ES6. Note that there's also {Array,Map,Set}.prototype.keys, which I don't find confusing. This is confusing because both concepts are about properties. This confuses everyone. The problem is that "property key" really is the right name for this. > And Reflect.ownKeys() matches the former, for extra confusion. That's not fair. Everything new matches "property key". The only thing that doesn't match, as far as I can tell, is Object.keys ...and a bunch of internal cruft we visited upon ourselves. We should rename internally away from the "Object.keys" precedent.

I think the issue is that Object.keys is the sort of "key" that ES consumers are most familiar with... I guess I'm ok with moving away from that naming wholesale internally, though; let's just not get stuck halfway. What's the new name for "enumerable keys"?

(In reply to Boris Zbarsky [:bz] from comment #4) > I think the issue is that Object.keys is the sort of "key" that ES consumers > are most familiar with... True. :( > I guess I'm ok with moving away from that naming wholesale internally, > though; let's just not get stuck halfway. What's the new name for > "enumerable keys"? There's nothing official. I say stuff like "for-in keys" if it's supposed to behave like for-in (in particular, walking the proto chain); "Object.keys keys" if it's like Object.keys.

Jason summarized it nicely. It's legacy (well only since ES5) Object.keys that doesn't follow the naming pattern. If we came up with a different term rather than "key" for the concept of "the value (a string or symbol) that is used to identify and access object properties" we could do a mass renaming in the ES6 spec, including the Reflect function names. But, so far I haven't seen any good candidates. (BTW, for compatibility with ES5 APIs, we've already make "name" string-valued property names.) Finally, the new name for "enumerable keys" is "enumerable keys".

(cleared needinfo)

> There's nothing official. I guess the question is what the proxy handler "keys" trap should be renamed to.

(In reply to Boris Zbarsky [:bz] from comment #8) > I guess the question is what the proxy handler "keys" trap should be renamed > to. In the current ES6 draft the handler name corresponding to the [[OwnPropertyKeys]] internal method is "ownKeys". There currently isn't any ongoing discussion in TC39-land about changing that name.

So to be clear, I'm talking about our internal proxy API, not the spec one. [[OwnPropertyKeys]] is not the same thing as the "keys" internal proxy handler trap in SpiderMonkey. [[OwnPropertyKeys]] corresponds to the getOwnPropertyNames internal proxy handler trap. "keys" is currently a derived trap which returns only the enumerable subset of what getOwnPropertyNames returns. Presumably this is all based on an old version of the spec's proxy setup or something.

(In reply to Boris Zbarsky [:bz] from comment #10) ok, here is the current ES6 mapping of ES6 internal methods to proxy handler methods [[GetPrototypeOf]] getPrototypeOf [[SetPrototypeOf]] setPrototypeOf [[IsExtensible]] isExtensible [[PreventExtensions]] preventExtensions [[GetOwnProperty]] getOwnPropertyDescriptor [[HasProperty]] has [[Get]] get [[Set]] set [[Delete]] deleteProperty [[DefineOwnProperty]] defineProperty [[Enumerate]] enumerate [[OwnPropertyKeys]] ownKeys [[Call]] apply [[Construct]] construct

Also renamed in this patch: ENUMERATE_IF_DEFINED -> ADD_KEYS_IF_DEFINED XrayEnumerateAttributesOrMethods -> XrayAttributeOrMethodKeys XrayEnumerateNativeProperties -> XrayOwnNativePropertyKeys XrayEnumerateProperties -> XrayOwnPropertyKeys WrapperOwner::getPropertyNames -> getPropertyKeys These make sense because JSITER_* flags are involved; the functions in question are not for finding enumerable properties only.

Comment on attachment 8500504 [details] [diff] [review] part 1 - Rename BaseProxyHandler::getOwnPropertyNames -> ownPropertyKeys to match the ES6 [[OwnPropertyKeys]] internal method r?bz for the dom parts

Comment on attachment 8500504 [details] [diff] [review] part 1 - Rename BaseProxyHandler::getOwnPropertyNames -> ownPropertyKeys to match the ES6 [[OwnPropertyKeys]] internal method Review of attachment 8500504 [details] [diff] [review]: ----------------------------------------------------------------- r=me with symbols related material moved to a separate patch ::: dom/bindings/DOMJSProxyHandler.cpp @@ +301,2 @@ > { > + return ownPropNames(cx, proxy, JSITER_OWNONLY | JSITER_HIDDEN | JSITER_SYMBOLS, props); Is this a seperate fix from a previously missed case? Adding JSITER_SYMBOLS seems unrelated to the rename. ::: js/ipc/WrapperOwner.cpp @@ +775,5 @@ > ObjectId objId = idOf(proxy); > > ReturnStatus status; > InfallibleTArray<nsString> names; > + if (!CallGetPropertyNames(objId, flags, &status, &names)) // FIXME: what about symbols? Again, this comment is justified, but maybe lost in this patch. ::: js/src/proxy/ScriptedDirectProxyHandler.cpp @@ -557,5 @@ > // strict-mode throwing. At present, the engine is not prepared to do that. See bug 826587. > return true; > } > > -// This is secretly [[OwnPropertyKeys]]. SM uses the old wiki name, internally. glad to see this go!

Comment on attachment 8500508 [details] [diff] [review] part 2 - Rename js::GetPropertyNames -> GetPropertyKeys Review of attachment 8500508 [details] [diff] [review]: ----------------------------------------------------------------- APPROVED.

Comment on attachment 8500504 [details] [diff] [review] part 1 - Rename BaseProxyHandler::getOwnPropertyNames -> ownPropertyKeys to match the ES6 [[OwnPropertyKeys]] internal method Hmm. The call to ownPropNames now no longer matches the documentation for that method. One or the other should be fixed. Preferably the callsite. Maybe I shouldn't have used jsiter flags at all for ownPropNames, since it's really just serving as an "include non-enumerable props?" boolean. Further, the comment above ownPropNames() still talks about getOwnPropertyNames(), no? _That_ definitely needs to be fixed. r=me with the above fixed. I really hope we have a plan for renaming the keys() trap....

(In reply to Eric Faust [:efaust] from comment #16) > > + return ownPropNames(cx, proxy, JSITER_OWNONLY | JSITER_HIDDEN | JSITER_SYMBOLS, props); > > Is this a seperate fix from a previously missed case? Adding JSITER_SYMBOLS > seems unrelated to the rename. Sure was. Factored out to another patch which I'll post for review separately. > > + if (!CallGetPropertyNames(objId, flags, &status, &names)) // FIXME: what about symbols? > > Again, this comment is justified, but maybe lost in this patch. Yeah - part 3 in this stack removes it, so there's no point introducing a FIXME there at all...

(In reply to Boris Zbarsky [:bz] from comment #18) > Comment on attachment 8500504 [details] [diff] [review] > part 1 - Rename BaseProxyHandler::getOwnPropertyNames -> ownPropertyKeys to > match the ES6 [[OwnPropertyKeys]] internal method > > Hmm. The call to ownPropNames now no longer matches the documentation for > that method. One or the other should be fixed. Preferably the callsite. I have a patch that does exactly that, which I'm going to have to land before bug 918828. This one's just for renaming; the idea is to avoid changing behavior at all in this patch. > Maybe I shouldn't have used jsiter flags at all for ownPropNames, since it's > really just serving as an "include non-enumerable props?" boolean. Well, the patch I mentioned above works by using the flags, changing the call site to pass JSPROP_SYMBOLS too. > Further, the comment above ownPropNames() still talks about > getOwnPropertyNames(), no? _That_ definitely needs to be fixed. Fixed. > I really hope we have a plan for renaming the keys() trap.... Take your pick: * Unify ownPropertyKeys(), keys() and enumerate() into a single getPropertyKeys(cx, flags, &props) method, at the expense of not aligning well with ES6 - so that the implementation of scripted proxies would be complicated somewhat. * Kill keys() and reimplement code that uses it in terms of ownPropertyKeys() and getOwnPropertyDescriptor(), with the understanding that Object.keys() performance would tank. * Just rename keys() to something verbose, like getOwnEnumerablePropertyNames().

> Take your pick: I'll take option 3, please.

https://hg.mozilla.org/integration/mozilla-inbound/rev/90eec7edf8c0 https://hg.mozilla.org/integration/mozilla-inbound/rev/d646fdb79bf6 https://hg.mozilla.org/integration/mozilla-inbound/rev/38a7ffcc3507

https://hg.mozilla.org/mozilla-central/rev/90eec7edf8c0 https://hg.mozilla.org/mozilla-central/rev/d646fdb79bf6 https://hg.mozilla.org/mozilla-central/rev/38a7ffcc3507