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.

Sign up for GitHub

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

Jump to bottom

Use Web IDL's new-ish interface mixins concept #2566

Merged
merged 1 commit into from
Jan 4, 2018
Merged

Use Web IDL's new-ish interface mixins concept #2566

merged 1 commit into from
Jan 4, 2018

Conversation

romandev
Copy link
Contributor

@romandev romandev commented Dec 21, 2017 •
edited by kainino0x
Loading

WebIDL recently introduced dedicated syntax for mixins [1]. This
replaces the existing [NoInterfaceObject] and "implements" syntax with
"interface mixin" and "includes" in the appropriate places.

This fixes #2538, fixes #2539 issues.

[1] whatwg/webidl@45e8173

@romandev
Use Web IDL's new-ish interface mixins concept
b9ea994 
WebIDL recently introduced dedicated syntax for mixins [1]. This
replaces the existing [NoInterfaceObject] and "implements" syntax with
"interface mixin" and "includes" in the appropriate places.

This fixes KhronosGroup#2538, KhronosGroup#2539 issues.

[1] whatwg/webidl@45e8173
kenrussell
kenrussell approved these changes Dec 21, 2017
View reviewed changes
Copy link
Member

@kenrussell kenrussell left a comment

Choose a reason for hiding this comment

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

Looks good to me. Confirmed that the .idl files are exactly what the Makefiles generate from the HTML.

@jdashg in particular any comments?

@RafaelCintron
Copy link
Member

All of the WebGL extension interfaces also use NoInterfaceObject. I presume those would need to be changed as part of this pull request or a subsequent one

@kainino0x
Copy link
Contributor

It used to be that [NoInterfaceObject] interface had to be used for mixins (basically, parent interfaces). The interface mixin+includes syntax replaces that.

But the extensions aren't mixins - they're legacy uses of [NoInterfaceObject]. It's not clear to me why this use of [NoInterfaceObject] is deprecated, though. I don't think we can really remove it.

@kenrussell
Copy link
Member

The intent was to prevent polluting the global namespace with interfaces for each of the WebGL extensions, since the actual type of those extension objects is unimportant. We shouldn't change that decision going forward, at least in my opinion.

This change seems fine to move the IDL closer toward the current Web IDL specification. Merging now since there were no objections to this specific change.

@kenrussell kenrussell merged commit 2df151b into KhronosGroup:master Jan 4, 2018
tobie
tobie reviewed Jan 4, 2018
View reviewed changes
specs/latest/2.0/index.html
@@ -968,12 +967,12 @@ <h3><a name="WEBGLRENDERINGCONTEXT">The WebGL context</a></h3>
[WebGLHandlesContextLoss] GLboolean isVertexArray(WebGLVertexArrayObject? vertexArray);
void bindVertexArray(WebGLVertexArrayObject? array);
};
WebGL2RenderingContextBase implements WebGLRenderingContextBase;
WebGL2RenderingContextBase includes WebGLRenderingContextBase;
Copy link

@tobie tobie Jan 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So mixins cannot include other mixins. Instead you need to write:

interface WebGL2RenderingContext {};
WebGL2RenderingContext includes WebGLRenderingContextBase;
WebGL2RenderingContext includes WebGL2RenderingContextBase;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're right. I'll make a follow-up PR.

This was referenced Jan 4, 2018
Merged
Closed
@tobie
Copy link

tobie commented Jan 4, 2018

@kainino0x wrote:

But the extensions aren't mixins - they're legacy uses of [NoInterfaceObject]. It's not clear to me why this use of [NoInterfaceObject] is deprecated, though. I don't think we can really remove it.

Added a section to WebIDL that clarifies what legacy constructs really mean, as this is something that comes up regularly and which I also found confusing when I started working on WebIDL. I hope it helps.

romandev pushed a commit to romandev/WebGL that referenced this pull request Jan 4, 2018
Fix misuse of mixin syntax
0e02fae 
Introduced `interface mixin` syntax in KhronosGroup#2566 but it's incorrect.
A `interface mixin` SHOULD NOT includes another mixin.

This fixes KhronosGroup#2566.
romandev added a commit to romandev/WebGL that referenced this pull request Jan 4, 2018
@romandev
Fix misuse of mixin syntax
c074014 
Introduced `interface mixin` syntax in KhronosGroup#2566 but it's incorrect.
A `interface mixin` SHOULD NOT includes another mixin.

This fixes KhronosGroup#2566.
@romandev romandev mentioned this pull request Jan 4, 2018
Merged
@romandev romandev deleted the mixin branch January 4, 2018 20:03
@romandev romandev restored the mixin branch January 4, 2018 20:03
romandev added a commit to romandev/WebGL that referenced this pull request Jan 4, 2018
@romandev
Fix misuse of mixin syntax
acb6351 
Introduced `interface mixin` syntax in KhronosGroup#2566 but it's incorrect.
A `interface mixin` SHOULD NOT includes another mixin.

This fixes KhronosGroup#2566.
kenrussell pushed a commit that referenced this pull request Jan 4, 2018
@romandev @kenrussell
Fix misuse of mixin syntax (#2575)
a6f9909 
Introduced `interface mixin` syntax in #2566 but it's incorrect.
A `interface mixin` SHOULD NOT includes another mixin.

This fixes #2566.
Richard-Yunchao pushed a commit to Richard-Yunchao/WebGL that referenced this pull request Jan 12, 2018
@romandev @Richard-Yunchao
Use Web IDL's new-ish interface mixins concept (KhronosGroup#2566)
fc19b4d 
WebIDL recently introduced dedicated syntax for mixins [1]. This
replaces the existing [NoInterfaceObject] and "implements" syntax with
"interface mixin" and "includes" in the appropriate places.

This fixes KhronosGroup#2538, KhronosGroup#2539 issues.

[1] whatwg/webidl@45e8173
Richard-Yunchao pushed a commit to Richard-Yunchao/WebGL that referenced this pull request Jan 12, 2018
@romandev @Richard-Yunchao
Fix misuse of mixin syntax (KhronosGroup#2575)
964ef93 
Introduced `interface mixin` syntax in KhronosGroup#2566 but it's incorrect.
A `interface mixin` SHOULD NOT includes another mixin.

This fixes KhronosGroup#2566.
@bzbarsky bzbarsky mentioned this pull request Jul 29, 2019
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobie tobie tobie left review comments

@kainino0x kainino0x kainino0x approved these changes

@kenrussell kenrussell kenrussell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@romandev @RafaelCintron @kainino0x @kenrussell @tobie