568413 - TM: implement iterate trap for proxies

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

This is a late but important addition to the spec.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

This patch also includes an important fix. We didn't delegate to the builtin derived trap default implementations for scripted handlers.

Comment 3

[Andreas Gal :gal]

Attachment: patch

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 447690 [details] [diff] [review]
patch

brace on own line:

JSScriptedProxyHandler::family() {

Should the singletons returned by family() be const?

Make static bool ValueToBool also be inline.

Common ugly and verbose ((JSProxyHandler *) JSVAL_TO_PRIVATE(handler)) via static inline helper...

Why doesn't MakeNativeIterator return ni directly instead of via out param?

NewNativeIterator is confusingly named in light of MakeNativeIterator and InitNativeIterator -- the latter two seem like constructor or create functions, which are typically named "NewXXX" where "XXX" is the typename. But NewNativeIterator makes an object too, and returns it via a jsval* out param -- how about NewNativeIteratorValue? But please also clean up the Make and Init misnomers.

Nit: NewNativeIterator does not need a blank line before its final return.

FundamentalTrap, in an error report, has

JS_GetStringBytes(JSVAL_TO_STRING(ID_TO_VALUE(atom)))

which should be just js_AtomToPrintableString(cx, atom) instead.

Nit: at least this line, maybe others:

    virtual bool getPropertyDescriptor(JSContext *cx, JSObject *proxy, jsid id, JSPropertyDescriptor *desc);

go over 100 columns.

/be

Comment 5

[Brendan Eich [:brendan]]

So,

s/NewNativeIterator(/NewNativeIteratorValue(/g
s/InitNativeIterator/NewEnumeratingNativeIterator/g
s/MakeNativeIterator/NewNativeIterator/g

where Enumerating means the snapshot including unshadowed enumerable proto props.

/be

Comment 6

[Brendan Eich [:brendan]]

Recurrent spacey final return nit, this time in MakeNativeIterator:

    *nip = ni;

    return true;

No need for such icy distance between such lines. ;-)

/be

Comment 7

[Andreas Gal :gal]

Attachment: patch

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 447791 [details] [diff] [review]
patch

+    virtual JS_FRIEND_API(const void) *family();

The * goes in the macro actual parameter!

r=me with that fixed, and anything like it.

/be

Comment 9

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/6ca8580eb84f

Comment 10

[Andreas Gal :gal]

Bah. I stripped off a pair of () to make it < 100 lines and a burning tree is what I got for it.

http://hg.mozilla.org/tracemonkey/rev/e51e5ce934d4

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/6ca8580eb84f