Closed Bug 1146235 Opened 5 years ago Closed 5 years ago

add support for an [Alias] extended attribute on IDL operations

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
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?
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8581444 - Flags: review?(peterv)
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.
(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.
> 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.
(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?
> 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?
(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.
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.
Disallow [Alias] on global interface operations and on static/unforgeable/identifierless operations.
Attachment #8581444 - Attachment is obsolete: true
Attachment #8581444 - Flags: review?(peterv)
Attachment #8582221 - Flags: review?(peterv)
Forgot to disallow ChromeOnly/Pref/etc. too.
Attachment #8582221 - Attachment is obsolete: true
Attachment #8582221 - Flags: review?(peterv)
Attachment #8582224 - Flags: review?(peterv)
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.
Attachment #8582226 - Flags: review?(peterv)
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.
Attachment #8582224 - Flags: review?(peterv) → review+
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?
Attachment #8582226 - Flags: review?(peterv) → review+
Cameron, if you get a chance to update the docs at https://developer.mozilla.org/en/Mozilla/WebIDL_bindings that would be much appreciated!
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.