Bug 32242 - implement Object.getOwnPropertyNames()
Summary: implement Object.getOwnPropertyNames()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kent Hansen
URL:
Keywords: ES5
Depends on:
Blocks:
 
Reported: 2009-12-07 13:24 PST by Patrick Mueller
Modified: 2010-04-24 20:28 PDT (History)
11 users (show)

See Also:


Attachments
Proposed patch (74.86 KB, patch)
2010-01-05 11:33 PST, Kent Hansen
ggaren: review-
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-
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 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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Mueller 2009-12-07 13:24:49 PST
For EcmaScript 5, Object.getOwnPropertyNames() is required, but not yet implemented by JSC.
Comment 1 Patrick Mueller 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 Patrick Mueller 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 Patrick Mueller 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 Kent Hansen 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 Maciej Stachowiak 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 Patrick Mueller 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 Geoffrey Garen 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 Kent Hansen 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 Patrick Mueller 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 Kent Hansen 2010-01-05 11:33:35 PST
Created attachment 45915 [details]
Proposed patch
Comment 11 WebKit Review Bot 2010-01-05 11:36:37 PST
style-queue ran check-webkit-style on attachment 45915 [details] without any errors.
Comment 12 Darin Adler 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 Geoffrey Garen 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 Geoffrey Garen 2010-01-05 13:34:34 PST
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.
Comment 15 Kent Hansen 2010-01-06 04:30:34 PST
Created attachment 45959 [details]
Revised patch (bool --> enum argument)
Comment 16 WebKit Review Bot 2010-01-06 04:35:49 PST
style-queue ran check-webkit-style on attachment 45959 [details] without any errors.
Comment 17 Simon Hausmann 2010-01-08 03:21:50 PST
Comment on attachment 45959 [details]
Revised patch (bool --> enum argument)

Let's try this patch through the commit-queue :)
Comment 18 Kent Hansen 2010-01-08 07:30:56 PST
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.
Comment 19 Geoffrey Garen 2010-01-08 09:08:38 PST
> Geoffrey, could you perhaps land this?

Sure, I'll give it a go.
Comment 20 Geoffrey Garen 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 Geoffrey Garen 2010-01-08 10:12:31 PST
Created attachment 46143 [details]
test failures

Here's the list of test failures.
Comment 22 Kent Hansen 2010-01-11 07:12:25 PST
(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.
Comment 23 Kent Hansen 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 Geoffrey Garen 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 Kent Hansen 2010-01-12 02:00:23 PST
Created attachment 46352 [details]
Revised patch (apply cleanly to trunk again)
Comment 26 WebKit Review Bot 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 Adam Barth 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 Kent Hansen 2010-01-12 03:35:38 PST
Created attachment 46359 [details]
Revised patch (apply cleanly to trunk again)

Reuploading to try to get BuildBot output.
Comment 29 WebKit Review Bot 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 Kent Hansen 2010-01-12 05:25:41 PST
(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.
Comment 31 Simon Hausmann 2010-01-12 06:38:43 PST
(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...
Comment 32 Geoffrey Garen 2010-01-12 11:48:42 PST
Comment on attachment 46359 [details]
Revised patch (apply cleanly to trunk again)

r=me
Comment 33 Adam Barth 2010-01-12 14:26:29 PST
@ossy: Looks like folks knew this would break the bot before landing.
Comment 34 Csaba Osztrogonác 2010-01-12 14:29:57 PST
I'll start a test now, and will write the Qt results in 30 minutes.
Comment 35 Csaba Osztrogonác 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 WebKit Commit Bot 2010-01-12 16:58:32 PST
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>
Comment 37 WebKit Commit Bot 2010-01-12 16:58:43 PST
All reviewed patches have been landed.  Closing bug.