Cameron McCormack (:heycam) Assignee
	Description • 9 years ago

I want to be able to declare in .webidl files that a given IDL operation has one or more aliases -- properties that have the same Function object value for the IDL operation.  This is to create interfaces that follow the ES pattern of Set, where keys, values and @@iterator all have the same value.

	Cameron McCormack (:heycam) Assignee
	Comment 1 • 9 years ago

I followed the lead of SetObject::initClass in js/src/builtin/MapObject.cpp, where it uses JS_DefineProperty to stick the aliased properties on the prototype.  It does seems a bit awkward in my case to have to call JS_GetProperty to get the Function object out that I want to copy into these other properties.  Is there a better way to handle that?

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 9 years ago

There isn't really, because right now the function is currently created entirely inside the JS engine.

Some things that we should consider:

1)  What is the .name of the function?  Presumably the spec should say this.
2)  What happens with Xrays?  Do we care about preserving the object identity guarantees
    there?  Because iirc Xrays resolve stuff lazily on DOM prototypes and wouldn't hit
    the code you're adding.
3)  When should the aliases be JSPROP_ENUMERATE or not?
4)  What happens with aliases on the global?  The code seems to assume that all the
    alias action is on the proto.  I'd be fine with just failing codegen for now if
    someone tries to use them on a global, but we shouldn't just silently generate wrong
    code.

	Cameron McCormack (:heycam) Assignee
	Comment 3 • 9 years ago

(In reply to Not doing reviews right now from comment #2)
> There isn't really, because right now the function is currently created
> entirely inside the JS engine.
> 
> Some things that we should consider:
> 
> 1)  What is the .name of the function?  Presumably the spec should say this.

The .name should be the same as the thing being aliased, as it's the very same Function object.  But yes we should define .name: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22392.

> 2)  What happens with Xrays?  Do we care about preserving the object
>     identity guarantees there?  Because iirc Xrays resolve stuff lazily on DOM
>     prototypes and wouldn't hit the code you're adding.

Where is this lazy resolution done?

> 3)  When should the aliases be JSPROP_ENUMERATE or not?

Maybe we should make them take on the enumerability of the property being aliased?  Which in my case of emulating the setlike interface on document.fonts would make them enumerable.  (Although I realise now that I may want a way to make all of them non-enumerable, as that's what the setlike members are meant to be according to Web IDL.)

How does enumerability work for Symbol-named properties?

> 4)  What happens with aliases on the global?  The code seems to assume that
>     all the alias action is on the proto.  I'd be fine with just failing codegen
>     for now if someone tries to use them on a global, but we shouldn't just
>     silently generate wrong code.

Disallowing sounds good.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 9 years ago

> The .name should be the same as the thing being aliased

OK.  So the alias relationship is clearly asymmetric in IDL?

> Where is this lazy resolution done?

See the XrayResolve* stuff in BindingUtils.cpp.

> Maybe we should make them take on the enumerability of the property being aliased?

OK.  Just checking, because the patch passes "0" for the define flags, which doesn't include JSPROP_ENUMERATE.

> Although I realise now that I may want a way to make all of them non-enumerable, as
> that's what the setlike members are meant to be according to Web IDL.

Hmm.   It's a bit weird to have some webidl things enumerable but not others...  But OK.

> How does enumerability work for Symbol-named properties?

You can define properties whose name is a Symbol and whose descriptor says "enumerable: true".  Apart from what getOwnPropertyDescriptor returns the enumerable value has no effect for normal objects, because http://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinary-object-internal-methods-and-internal-slots-enumerate explicitly excludes Symbol-named properties.

	Cameron McCormack (:heycam) Assignee
	Comment 5 • 9 years ago

(In reply to Not doing reviews right now from comment #4)
> OK.  So the alias relationship is clearly asymmetric in IDL?

Yes.

> > Where is this lazy resolution done?
> 
> See the XrayResolve* stuff in BindingUtils.cpp.

So if I understand correctly, the current patch will cause (using document.fonts as the example) keys and @@iterator properties not to be visible when looking at a content page's document.fonts object from chrome, is that right?  keys is pretty useless, but being able to iterate over the document.fonts object naturally with for-of might be something chrome code wants to do.

Without knowing this code very well, I guess this might involve either (a) adding something to JSFunctionSpec to indicate an alias, handling these aliases in JS_DefineFunctions and also in various XrayResolve* functions; or (b) adding something to NativeProperties to list the aliases, leaving the current CreateInterfaceObjects "get the property and set the aliases" code from this patch, and handling stuff in the XrayResolve* functions again.  And I guess I need to ensure the alias names get jsids, too.

Plus having the XrayResolve* functions ensure that we only get one copy of the function might be tricky.

Do you think this is all worth it?

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 9 years ago

> the current patch will cause (using document.fonts as the example) keys and @@iterator
> properties not to be visible when looking at a content page's document.fonts object from
> chrome, is that right?

If I read the patch correctly, yes.

I think the options you list are the two most obviously viable ones, yes.  (b) might not be so bad if we don't worry about aliasing for Xrays and just create separate function objects...

> Do you think this is all worth it?

You mean aliases in general?

	Cameron McCormack (:heycam) Assignee
	Comment 7 • 9 years ago

(In reply to Not doing reviews right now from comment #6)
> > Do you think this is all worth it?
> 
> You mean aliases in general?

Exposing aliases on xray wrappers.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 9 years ago

Seems to me like we want to expose @@iterator on various iterables one way or another.  Past that, I wouldn't worry about it too much, probably, as a first cut.

	Cameron McCormack (:heycam) Assignee
	Comment 9 • 9 years ago

Disallow [Alias] on global interface operations and on static/unforgeable/identifierless operations.

	Cameron McCormack (:heycam) Assignee
	Comment 10 • 9 years ago

Forgot to disallow ChromeOnly/Pref/etc. too.

	Cameron McCormack (:heycam) Assignee
	Comment 11 • 9 years ago

This exposes the @@iterator alias on XrayWrappers, albeit with its own Function object.

I think if we wanted to extend this to handle normal named aliases, we could have a (dynamically filled in) array of jsid mappings on the NativeFunctions object.

	Peter Van der Beken [:peterv]
	Comment 12 • 9 years ago

Comment on attachment 8582224 [details] [diff] [review]
Add support for an [Alias] extended attribute on IDL operations. (v2.1)

Review of attachment 8582224 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +2775,5 @@
> +            assert needInterfacePrototypeObject
> +
> +            def defineAlias(alias):
> +                if alias == "@@iterator":
> +                    getSymbol = CGGeneric("JS::Rooted<jsid> iteratorId(aCx, SYMBOL_TO_JSID(JS::GetWellKnownSymbol(aCx, JS::SymbolCode::iterator)));")

Can you rewrap this a bit?

@@ +2794,5 @@
> +                    if (!${defineFn}(aCx, proto, ${prop}, aliasedVal, JSPROP_ENUMERATE)) {
> +                      return;
> +                    }
> +                    """,
> +                    defineFn=defineFn, prop=prop))

We usually have one param per line for these.

::: dom/bindings/parser/WebIDL.py
@@ +1095,5 @@
> +                    for m in self.members:
> +                        if m.identifier.name == alias:
> +                            raise WebIDLError("[Alias=%s] has same name as "
> +                                              "interface member" % alias,
> +                                              [member.location, m.location])

What if we define 2 aliases with the same name on 2 different members? I suppose you need to check m's aliases too here.

	Peter Van der Beken [:peterv]
	Comment 13 • 9 years ago

Comment on attachment 8582226 [details] [diff] [review]
Expose @@iterator aliases on XrayWrappers.

Review of attachment 8582226 [details] [diff] [review]:
-----------------------------------------------------------------

Can you file a bug on adding complete alias support to Xrays?

	Cameron McCormack (:heycam) Assignee
	Comment 14 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d135a7b7864f
https://hg.mozilla.org/integration/mozilla-inbound/rev/665c141c2479

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 9 years ago

Cameron, if you get a chance to update the docs at https://developer.mozilla.org/en/Mozilla/WebIDL_bindings that would be much appreciated!

	Cameron McCormack (:heycam) Assignee
	Comment 16 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c203564a53ee

	Cameron McCormack (:heycam) Assignee
	Comment 17 • 9 years ago

Added docs: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#.5BAlias.5D

	Phil Ringnalda (:philor)
	Comment 18 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/d135a7b7864f
https://hg.mozilla.org/mozilla-central/rev/665c141c2479
https://hg.mozilla.org/mozilla-central/rev/c203564a53ee