TM: implement iterate trap for proxies

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
This is a late but important addition to the spec.
(Assignee)

Updated

9 years ago
Blocks: 546590
Depends on: 568051
(Assignee)

Comment 1

9 years ago
Created attachment 447689 [details] [diff] [review]
patch
Assignee: general → gal
(Assignee)

Comment 2

9 years ago
This patch also includes an important fix. We didn't delegate to the builtin derived trap default implementations for scripted handlers.
(Assignee)

Comment 3

9 years ago
Created attachment 447690 [details] [diff] [review]
patch
Attachment #447689 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #447690 - Flags: review?(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
So,

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

where Enumerating means the snapshot including unshadowed enumerable proto props.

/be
Recurrent spacey final return nit, this time in MakeNativeIterator:

    *nip = ni;

    return true;

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

/be
(Assignee)

Comment 7

9 years ago
Created attachment 447791 [details] [diff] [review]
patch
Attachment #447690 - Attachment is obsolete: true
Attachment #447690 - Flags: review?(brendan)
(Assignee)

Updated

9 years ago
Attachment #447791 - Flags: review?(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
Attachment #447791 - Flags: review?(brendan) → review+
(Assignee)

Comment 9

9 years ago
http://hg.mozilla.org/tracemonkey/rev/6ca8580eb84f
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 10

9 years ago
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

9 years ago
http://hg.mozilla.org/mozilla-central/rev/6ca8580eb84f
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Depends on: 589103
You need to log in before you can comment on or make changes to this bug.