Bug 32242 - implement Object.getOwnPropertyNames()
: implement Object.getOwnPropertyNames()
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: ES5
:
:
  Show dependency treegraph
 
Reported: 2009-12-07 13:24 PST by
Modified: 2010-04-24 20:28 PST (History)


Attachments
Proposed patch (74.86 KB, patch)
2010-01-05 11:33 PST, Kent Hansen
ggaren: review-
Review Patch | Details | Formatted Diff | Diff
Revised patch (bool --> enum argument) (75.32 KB, patch)
2010-01-06 04:30 PST, Kent Hansen
ggaren: review-
kent.hansen: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
test failures (21.92 KB, text/html)
2010-01-08 10:12 PST, Geoffrey Garen
no flags Details
Revised patch (apply cleanly to trunk again) (75.38 KB, patch)
2010-01-12 02:00 PST, Kent Hansen
no flags Review Patch | Details | Formatted Diff | Diff
Revised patch (apply cleanly to trunk again) (75.38 KB, patch)
2010-01-12 03:35 PST, Kent Hansen
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-12-07 13:24:49 PST
For EcmaScript 5, Object.getOwnPropertyNames() is required, but not yet implemented by JSC.
------- Comment #1 From 2009-12-08 14:12:16 PST -------
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.
------- Comment #2 From 2009-12-15 10:21:35 PST -------
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.
------- Comment #3 From 2009-12-15 11:48:05 PST -------
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
------- Comment #4 From 2009-12-30 04:55:45 PST -------
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).
------- Comment #5 From 2009-12-30 16:32:48 PST -------
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.
------- Comment #6 From 2010-01-04 13:30:39 PST -------
(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?
------- Comment #7 From 2010-01-04 13:49:44 PST -------
> 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.
------- Comment #8 From 2010-01-05 01:05:22 PST -------
(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.
------- Comment #9 From 2010-01-05 07:05:47 PST -------
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.
------- Comment #10 From 2010-01-05 11:33:35 PST -------
Created an attachment (id=45915) [details]
Proposed patch
------- Comment #11 From 2010-01-05 11:36:37 PST -------
style-queue ran check-webkit-style on attachment 45915 [details] without any errors.
------- Comment #12 From 2010-01-05 13:01:16 PST -------
(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.
------- Comment #13 From 2010-01-05 13:29:15 PST -------
(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".
------- Comment #14 From 2010-01-05 13:34:34 PST -------
(From update of attachment 45915 [details])
I'll say r- based on the enum vs bool style issue, but the rest of the patch looks good.
------- Comment #15 From 2010-01-06 04:30:34 PST -------
Created an attachment (id=45959) [details]
Revised patch (bool --> enum argument)
------- Comment #16 From 2010-01-06 04:35:49 PST -------
style-queue ran check-webkit-style on attachment 45959 [details] without any errors.
------- Comment #17 From 2010-01-08 03:21:50 PST -------
(From update of attachment 45959 [details])
Let's try this patch through the commit-queue :)
------- Comment #18 From 2010-01-08 07:30:56 PST -------
(From update of attachment 45959 [details])
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.
------- Comment #19 From 2010-01-08 09:08:38 PST -------
> Geoffrey, could you perhaps land this?

Sure, I'll give it a go.
------- Comment #20 From 2010-01-08 10:11:52 PST -------
(In reply to comment #19)
Hmmm. I applied the patch locally and got a bunch of test failures.
------- Comment #21 From 2010-01-08 10:12:31 PST -------
Created an attachment (id=46143) [details]
test failures

Here's the list of test failures.
------- Comment #22 From 2010-01-11 07:12:25 PST -------
(In reply to comment #21)
> Created an attachment (id=46143) [details] [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.
------- Comment #23 From 2010-01-11 11:36:23 PST -------
(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?
------- Comment #24 From 2010-01-11 14:16:36 PST -------
> 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.
------- Comment #25 From 2010-01-12 02:00:23 PST -------
Created an attachment (id=46352) [details]
Revised patch (apply cleanly to trunk again)
------- Comment #26 From 2010-01-12 03:02:17 PST -------
Attachment 46352 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/182432
------- Comment #27 From 2010-01-12 03:26:34 PST -------
> 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.  :(
------- Comment #28 From 2010-01-12 03:35:38 PST -------
Created an attachment (id=46359) [details]
Revised patch (apply cleanly to trunk again)

Reuploading to try to get BuildBot output.
------- Comment #29 From 2010-01-12 04:58:53 PST -------
Attachment 46359 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/183456
------- Comment #30 From 2010-01-12 05:25:41 PST -------
(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.
------- Comment #31 From 2010-01-12 06:38:43 PST -------
(In reply to comment #30)
> (In reply to comment #29)
> > Attachment 46359 [details] [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...
------- Comment #32 From 2010-01-12 11:48:42 PST -------
(From update of attachment 46359 [details])
r=me
------- Comment #33 From 2010-01-12 14:26:29 PST -------
@ossy: Looks like folks knew this would break the bot before landing.
------- Comment #34 From 2010-01-12 14:29:57 PST -------
I'll start a test now, and will write the Qt results in 30 minutes.
------- Comment #35 From 2010-01-12 14:56:47 PST -------
(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)
------- Comment #36 From 2010-01-12 16:58:32 PST -------
(From update of attachment 46359 [details])
Clearing flags on attachment: 46359

Committed r53170: <http://trac.webkit.org/changeset/53170>
------- Comment #37 From 2010-01-12 16:58:43 PST -------
All reviewed patches have been landed.  Closing bug.