Bug 32242 – implement Object.getOwnPropertyNames()

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Patrick Mueller]

For EcmaScript 5, Object.getOwnPropertyNames() is required, but not yet implemented by JSC.

[Patrick Mueller]

The implementation of this function should be the nearly the same as the Object.keys() current implementation.  The difference between keys() and getOwnPropertyNames() seems to be that keys() only returns enumerable properties, and gOPN() returns all of them.

Looking at the keys() implementation in runtime/ObjectConstructor.cpp, objectConstructorKeys(), it calls an internal getOwnPropertyNames() which actually returns property names pre-filtered as enumerable.  Unfortunate name choice there (oh well).  

Looks to me like Structure::getEnumerablePropertyNames() should probably be generalized to take a boolean option on whether to include non-enumerable properties or not, then the same function can be used for both the Object.keys() and Object.getOwnPropertyNames() implementation.

[Patrick Mueller]

turning out to be yuckier than I thought.  I was hoping to just modify some of the get*PropertyNames() methods in JSObject and Structure, but to the virtual nature of these, I've had to add an optional (default-valued)  "bool enumerable" in getOwnPropertyNames() methods through JSC.  Here's the hit list so far:

JavaScriptCore/runtime/Structure.h
JavaScriptCore/runtime/JSObject.h
JavaScriptCore/runtime/JSObject.cpp
JavaScriptCore/runtime/JSArray.h
JavaScriptCore/runtime/StringObject.h
JavaScriptCore/runtime/JSByteArray.h
JavaScriptCore/runtime/JSArray.cpp
JavaScriptCore/runtime/JSVariableObject.h
JavaScriptCore/runtime/JSByteArray.cpp
JavaScriptCore/runtime/Structure.cpp
JavaScriptCore/runtime/RegExpMatchesArray.h
JavaScriptCore/runtime/JSNotAnObject.h
JavaScriptCore/runtime/StringObject.cpp
JavaScriptCore/runtime/JSNotAnObject.cpp
JavaScriptCore/runtime/JSVariableObject.cpp
JavaScriptCore/debugger/DebuggerActivation.cpp
JavaScriptCore/debugger/DebuggerActivation.h
JavaScriptCore/API/JSCallbackObjectFunctions.h
JavaScriptCore/API/JSCallbackObject.h
JavaScriptCore/JavaScriptCore.exp

Even just with these changes, I'm seeing LayoutTest failures, trying just fast/js for now.  One test case - global-constructors, is all that's failing, but NONE of the constructors are being found.  I'm guessing that some of the higher levels of JavaScript goop in WebKit will need changes also.  

Icky.

[Patrick Mueller]

doing a global search across WebKit for getOwnPropertyNames() methods yields
multiple hits.  It's unclear to me at the moment how many of these are genned,
and how many are written by hand.  I'm guessing that there are implementations
of visible objects, specifically, what fast/js/global-constructors actually
finds, which are auto-genned, and there are glue-y bits which are hand built.  

The search did get a hit on WebCore/bindings/scripts/CodeGeneratorJS.pm which
is a generator, which would presumably work for the auto-genned classes.

It's not clear to me that there is a less intrusive fix here.  The problem is
that any object, including host ones, can presumably have non-enumerable
properties, so there has to be a way to get these.  On the other hand,
retrieving non-enumerable properties is going to be something done almost none
of the time, compared with retrieving the enumerable properties, so we don't
want to, for instance, always have the most inner most calls returning
non-enumerable properties then filtering them somehow later (not sure how that
would even be done).

It might be the case that we could assume that host objects never have
non-enumerable properties, and then just provide the capability of getting the
own, non-enumerated properties to non-host objects.  Not quite sure how this
would work either, but is potentially an interesting solution (affects less
code, no performance impacts, etc).

In any case, so I'm going to stop looking into this one:

- perhaps there is a simpler way to do this that doesn't hit so many files, but I don't know what it is

- the fix requires hits to so many files that I'm hesitant to make the change myself given my lack of experience with JSC

[Kent Hansen]

Hi Patrick, great that you've been looking into this!

You pretty much arrived at the same conclusion as I did. Yes, you will have to modify dozens of files when adding a bool enumerable flag to getOwnPropertyNames(). (And yes, you will need to modify the JS bindings code generator, too.)
Yes, it feels rather intrusive. But I'm not convinced there's a better way.

I don't think it's sufficient to only provide enumeration for non-native objects. getOwnPropertyNames() needs to work for Array, String and arguments objects, for example.

A less intrusive approach might be to introduce a dedicated virtual function for getting non-enumerable property names; e.g. getOwnNonEnumerablePropertyNames(). You'd still need to actually reimplement that function in most of the classes you list in comment #2, in order to fully support Object.getOwnPropertyNames() for all types of object.

Finally, I agree that Structure::getEnumerablePropertyNames() should be generalized to take a flag. Or rather, it should simply call a helper function, e.g. Structure::getPropertyNames(EnumerablePropertiesOnly).

[Maciej Stachowiak]

I suspect that no matter what you do, you'll have to touch every place that implements getOwnPropertyNames. Thus, you may as well change the internal getOwnPropertyNames method to take the extra boolean argument.

[Patrick Mueller]

(In reply to comment #5)
> I suspect that no matter what you do, you'll have to touch every place that
> implements getOwnPropertyNames. Thus, you may as well change the internal
> getOwnPropertyNames method to take the extra boolean argument.

Since so many places are being touched, seems worthwhile to ask what the shape of the final change should be.  For the purposes of this bug, all that is currently envisioned is adding a bool indicating whether non-enumerable properties should be returned.  Perhaps an int should be used instead, for future flag capabilities?

[Geoffrey Garen]

> Since so many places are being touched, seems worthwhile to ask what the shape
> of the final change should be.  For the purposes of this bug, all that is
> currently envisioned is adding a bool indicating whether non-enumerable
> properties should be returned.  Perhaps an int should be used instead, for
> future flag capabilities?

I think a bool is better, for clarity.

We don't have binary compatibility constraints within WebKit. If we ever want to add more flags in the future, we can add another parameter, or change the bool to an int or an enum.

[Kent Hansen]

(In reply to comment #7)
> I think a bool is better, for clarity.

Except getOwnProperties(..., true) isn't that readable.
This was brought up in the "unwritten rules of webkit style" thread on webkit-dev@. One argument I remember was "if you're going to be passing bool constants (i.e. not variables) as arguments to functions, prefer an enum".

The only use-case I have so far for calling getOwnPropertyNames() with the non-default value is in the implementation of Object.getOwnPropertyNames, so I'm not sure if that justifies using an enum.

The general form would be one of:

void getOwnPropertyNames(ExecState*, PropertyNameArray&, bool includeNonEnumerable = false);

or:

enum EnumerationMode {
    EnumerateNormal,
    EnumerateAll
};

void getOwnPropertyNames(ExecState*, PropertyNameArray&, EnumerationMode mode = EnumerateNormal);

(Not sure about the best name for the enum)
I tend to prefer the bool for the reasons you gave.

[Patrick Mueller]

One of the use cases I thought of for "additional function" for a flags variable would be an unordered enumeration.  Perhaps a bad example, but it's a pet peeve of mine (hate that the JS engines have to keep properties in insertion order), but if someone needed access to property values, didn't care about the insertion order, they could avoid a sort, perhaps.

In a case like that, the enum wouldn't fit well, because you'd actually want that indication (don't sort) as well as it as the other options like (return only enumerable / return all).

I'm in over my head here, and this smells a bit like over-engineering, but thought I'd give the dead horse a final kick.

[Kent Hansen]

Created attachment 45915 [details]
Proposed patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45915 [details] without any errors.

[Darin Adler]

(In reply to comment #8)
> Except getOwnProperties(..., true) isn't that readable.
> This was brought up in the "unwritten rules of webkit style" thread on
> webkit-dev@. One argument I remember was "if you're going to be passing bool
> constants (i.e. not variables) as arguments to functions, prefer an enum".

This is definitely a rule of WebKit style, albeit one that's not yet in the style guide. Having a ", true" or ", false" in a function call is not nearly as good as a named argument. The enum style is something we do all the time instead of bools in new code.

[Geoffrey Garen]

(In reply to comment #8)
> > I think a bool is better, for clarity.
> 
> Except getOwnProperties(..., true) isn't that readable.
> This was brought up in the "unwritten rules of webkit style" thread on
> webkit-dev@. One argument I remember was "if you're going to be passing bool
> constants (i.e. not variables) as arguments to functions, prefer an enum".

I just meant to say that a type that conveys your meaning is better than a type
that conveys your meaning and reserves extra behavior for mysterious future
features.

I agree that, according to webkit's style guidelines, an enum is preferable in
this case.

> enum EnumerationMode {
>     EnumerateNormal,
>     EnumerateAll
> };

Rather than "Normal" and "All", which are a little vague, I would use
"ExcludeDontEnumProperties" and IncludeDontEnumProperties".

[Geoffrey Garen]

Comment on attachment 45915 [details]
Proposed patch

I'll say r- based on the enum vs bool style issue, but the rest of the patch looks good.

[Kent Hansen]

Created attachment 45959 [details]
Revised patch (bool --> enum argument)

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45959 [details] without any errors.

[Simon Hausmann]

Comment on attachment 45959 [details]
Revised patch (bool --> enum argument)

Let's try this patch through the commit-queue :)

[Kent Hansen]

Comment on attachment 45959 [details]
Revised patch (bool --> enum argument)

The patch no longer applies for me after r52948 (a line was added in Structure.h, or it was because of the unintended whitespace insertion on the line before "private").
Geoffrey, could you perhaps land this? Note that I _have not_ tested the build on Windows nor Mac; I hope the symbol exports will be fine.
Otherwise, I'll rebase and submit a new patch on Monday.

[Geoffrey Garen]

> Geoffrey, could you perhaps land this?

Sure, I'll give it a go.

[Geoffrey Garen]

(In reply to comment #19)
Hmmm. I applied the patch locally and got a bunch of test failures.

[Geoffrey Garen]

Created attachment 46143 [details]
test failures

Here's the list of test failures.

[Kent Hansen]

(In reply to comment #21)
> Created an attachment (id=46143) [details]
> test failures
> 
> Here's the list of test failures.

Thanks. Working on getting a Safari build up and running so I can debug these myself.

[Kent Hansen]

(In reply to comment #20)
> (In reply to comment #19)
> Hmmm. I applied the patch locally and got a bunch of test failures.

Strange, I've applied the patch on top of r53079 and I'm not seeing any of those failures (Safari build this time, not Qt port).
Did you do a clean build?

[Geoffrey Garen]

> Strange, I've applied the patch on top of r53079 and I'm not seeing any of
> those failures (Safari build this time, not Qt port).
> Did you do a clean build?

Interesting.

I didn't type "make clean", but I did do a build with no other changes in my tree. Maybe I made a mistake.

Why don't you post your updated patch, and we can ask the commit queue to land it.

[Kent Hansen]

Created attachment 46352 [details]
Revised patch (apply cleanly to trunk again)

[WebKit Review Bot]

Attachment 46352 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/182432

[Adam Barth]

> Build output: http://webkit-commit-queue.appspot.com/results/182432

Sorry that the build output isn't available.  The EWS was misbehaving for a bit because of a typo on my part.  :(

[Kent Hansen]

Created attachment 46359 [details]
Revised patch (apply cleanly to trunk again)

Reuploading to try to get BuildBot output.

[WebKit Review Bot]

Attachment 46359 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/183456

[Kent Hansen]

(In reply to comment #29)
> Attachment 46359 [details] did not build on qt:
> Build output: http://webkit-commit-queue.appspot.com/results/183456

Build output is still blank.
If it's doing an incremental build, then that might the reason for it failing; it does for me locally at least (during linking). I have to do a clean build.

[Simon Hausmann]

(In reply to comment #30)
> (In reply to comment #29)
> > Attachment 46359 [details] [details] did not build on qt:
> > Build output: http://webkit-commit-queue.appspot.com/results/183456
> 
> Build output is still blank.
> If it's doing an incremental build, then that might the reason for it failing;
> it does for me locally at least (during linking). I have to do a clean build.

Ah yes, a limitation of the qt build system used for webkit ;(


That means when landing the patch we also have to nudge the Qt build bot...

[Geoffrey Garen]

Comment on attachment 46359 [details]
Revised patch (apply cleanly to trunk again)

r=me

[Adam Barth]

@ossy: Looks like folks knew this would break the bot before landing.

[Csaba Osztrogonác]

I'll start a test now, and will write the Qt results in 30 minutes.

[Csaba Osztrogonác]

(In reply to comment #34)
> I'll start a test now, and will write the Qt results in 30 minutes.
Here the patch works correctly without any layout test regression. (Qt-Linux)

[WebKit Commit Bot]

Comment on attachment 46359 [details]
Revised patch (apply cleanly to trunk again)

Clearing flags on attachment: 46359

Committed r53170: <http://trac.webkit.org/changeset/53170>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.