Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop using extended attributes for constructors #700

Merged
merged 1 commit into from Aug 27, 2019
Merged

Conversation

Ms2ger
Copy link
Member

@Ms2ger Ms2ger commented Mar 29, 2019

Fixes #636.


Preview | Diff

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pushing this! I think we should wait with merging this until we have support on wpt/bikeshed, implementers are on board, and we have some number of specification PRs for this (I can help a bit next week). That seems preferable to slowly changing these things as then it might end up taking a long time for a full transition with lots of (costly) hiccups along the way.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
@marcoscaceres
Copy link
Member

@annevk is right. I can also help update legacy ReSpec specs using the old constructor.

@saschanaz and I can take care of things on WebIDL2.js, which WPT relies on.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good; a few minor things to resolve.

In regard to ecosystem impact, what I'd really like to do is have a checklist of all specs to update. How long it takes us to update is less crucial to me, but having a corpus of all [Constructor]-using IDL and a list of specs we need to send PRs to would be a good idea.

@foolip or his team may know the latest on the "canonical IDL corpus" story?

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Member

@foolip @domenic, quick list from grepping wpt's interfaces directory:
https://gist.github.com/marcoscaceres/98241f92092132cba703e9be89500061

Gives us some idea... it looks like we would need to update ~75 specs.

@marcoscaceres
Copy link
Member

Here is the list of matching files/specs: https://gist.github.com/marcoscaceres/c4d507839522d6bc96df23d460526c6a

@foolip
Copy link
Member

foolip commented Apr 4, 2019

https://github.com/tidoust/reffy-reports/tree/master/whatwg/idl is the source of the IDL files in WPT and what I've been treating as "all the web's IDL" for some time now.

I just finished fixing specs missed in the [NoInterfaceObject] interface+implements to interface mixin+includes transition, so making sure all changes have propagated to reffy-reports before using [Constructor] becomes invalid and is dropped from webidl2.js would be much appreciated.

@saschanaz
Copy link
Member

saschanaz commented Jul 14, 2019

What is this currently waiting for? Should issues be filed for browser implementations?

@bzbarsky
Copy link
Collaborator

Well, for starters it's not clear whether implementors are in fact on board...

@domenic
Copy link
Member

domenic commented Jul 14, 2019

Could you speak to that for Gecko? Chromium is on board.

@bzbarsky
Copy link
Collaborator

bzbarsky commented Jul 30, 2019

I am still sorting out the Gecko position here.

In terms of the actual change, after this you won't be able to use "constructor" as an argument name, dictionary member name, attribute name, or method name, unless you write it as _constructor. For attribute and method names this seems OK. For argument names, I'd suspect we want to add it to the ArgumentNameKeyword production, right? For dictionary members, do we want to allow it? It'd require a new production for dictionary member names, but that seems fine to me... and it would be potentially useful if people want to extract things like ES class info from objects via dictionaries.

I did just check and there are no existing IDLs in Gecko that use "constructor" in any of those situations, by the way.

@bzbarsky
Copy link
Collaborator

@domenic @Ms2ger it looks like the proposal disallows constructors on partial interfaces in prose, but it seems like we could do it on the syntax level now that there is an explicit syntactic construct. Could we do that? That check would be a lot more likely to actually get implemented in tools that way, I suspect, since just updating the grammar would do it.

@saschanaz
Copy link
Member

saschanaz commented Jul 31, 2019

A quick search on reffy-reports says the only place that uses the name constructor is CustomElementRegistry.

@bzbarsky
Copy link
Collaborator

OK, apart from the grammar issues mentioned above, I think Gecko is on board.

@bzbarsky
Copy link
Collaborator

Though I guess constructor is explicitly a reserved identifier (such that you can't use _constructor either, even without these changes).

In that case, the only grammar concern I have for now is the partial interface situation.

@Ms2ger
Copy link
Member Author

Ms2ger commented Aug 6, 2019

Made the change for partial interfaces. I think that resolves everything besides the comment about "constructor method". I'll change that to "operation" tomorrow, but the PR should be ready for re-review now.

Who works on this for WebKit now? @rniwa?

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
Copy link
Collaborator

@bzbarsky bzbarsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me. Would still like feedback from WebKit.

@Ms2ger
Copy link
Member Author

Ms2ger commented Aug 8, 2019

Looks reasonable to me. Would still like feedback from WebKit.

@cdumez @youennf

@Ms2ger
Copy link
Member Author

Ms2ger commented Aug 13, 2019

I propose we merge if Apple doesn't comment by next Monday, the 19th.

@youennf
Copy link

youennf commented Aug 13, 2019

I see some potential future benefits but this seems quite a big syntactic change.
Is there any concrete plan for specs to use this new ability?

In terms of WebKit implementation, our constructor-specific keywords (canthrow, callwith, custom) have counterparts for operations, this seems fine and I quite like it in fact.

I wonder what the plan is for NamedConstructor.
WebKit implementation would need to keep these constructor-specific keywords around for NamedConstructor if it is not migrated as well.

@Ms2ger
Copy link
Member Author

Ms2ger commented Aug 13, 2019

I'm expecting to file spec bugs based on a combination of scraping browser source code and adding errors in idlharness.js.

I think changing NamedConstructor as well would make sense, but don't have a timeline for that yet.

@domenic
Copy link
Member

domenic commented Aug 13, 2019

I'm not so sure about NamedConstructor, since that adds a factory function to the global, and doesn't add a member of the class (or effect the class in any way). Keeping it as-is seems better to me... Or if anything, making it a separate top-level declaration.

@Ms2ger Ms2ger merged commit 91ca6eb into master Aug 27, 2019
@Ms2ger Ms2ger deleted the constructor branch August 27, 2019 15:27
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 13, 2019
…ser. r=edgar

The grammar changes parallel those in whatwg/webidl#700

We don't prevent having both a constructor operation and [Constructor] or
[ChromeConstructor], because those extended attributes are about to get
removed, once they are no longer used in our IDL.

Differential Revision: https://phabricator.services.mozilla.com/D45387

--HG--
extra : moz-landing-system : lando
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Sep 13, 2019
…ser. r=edgar

The grammar changes parallel those in whatwg/webidl#700

We don't prevent having both a constructor operation and [Constructor] or
[ChromeConstructor], because those extended attributes are about to get
removed, once they are no longer used in our IDL.

Differential Revision: https://phabricator.services.mozilla.com/D45387
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 4, 2019
…ser. r=edgar

The grammar changes parallel those in whatwg/webidl#700

We don't prevent having both a constructor operation and [Constructor] or
[ChromeConstructor], because those extended attributes are about to get
removed, once they are no longer used in our IDL.

Differential Revision: https://phabricator.services.mozilla.com/D45387

UltraBlame original commit: 937448e0b594ed4108b7e376305dcdd877325354
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 4, 2019
…ser. r=edgar

The grammar changes parallel those in whatwg/webidl#700

We don't prevent having both a constructor operation and [Constructor] or
[ChromeConstructor], because those extended attributes are about to get
removed, once they are no longer used in our IDL.

Differential Revision: https://phabricator.services.mozilla.com/D45387

UltraBlame original commit: 937448e0b594ed4108b7e376305dcdd877325354
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 4, 2019
…ser. r=edgar

The grammar changes parallel those in whatwg/webidl#700

We don't prevent having both a constructor operation and [Constructor] or
[ChromeConstructor], because those extended attributes are about to get
removed, once they are no longer used in our IDL.

Differential Revision: https://phabricator.services.mozilla.com/D45387

UltraBlame original commit: 937448e0b594ed4108b7e376305dcdd877325354
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Stop using extended attributes for constructors
8 participants