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

Add references to WebGL 2.0 ("webgl2") context. #3971

Merged
merged 5 commits into from
Aug 30, 2018

Conversation

kenrussell
Copy link
Member

@kenrussell kenrussell commented Aug 28, 2018

@kenrussell
Copy link
Member Author

@annevk @domenic please review.

Sorry for taking so long to finally do this.

A slight shortcut was taken in the updating of the tables for getContext for Canvas elements and OffscreenCanvas objects. Rather than add both a new row and column, the webgl and webgl2 contexts were combined. This seemed appropriate because the WebGL reference actually points to the shared location of the 1.0 and 2.0 specs.

Also added @jdashg as WebGL spec editor in the reference.

@kenrussell
Copy link
Member Author

Note also: built the spec locally and verified by hand that these edits look good.

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.

This looks great, thanks for writing it up. Here's a bunch of nits (some up for debate).

Also, is this already tested adequately?

source Outdated
@@ -3867,10 +3867,11 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute

<dd>

<p>The following interface is defined in the WebGL specification: <ref spec=WEBGL></p>
<p>The following interfaces are defined in the WebGL specification: <ref spec=WEBGL></p>
Copy link
Member

Choose a reason for hiding this comment

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

You switched to plural at some point below and since the reference we have is for both, let's make this "WebGL specifications".

@@ -59471,6 +59473,12 @@ callback <dfn>BlobCallback</dfn> = void (<span>Blob</span>? blob);</code></pre>
<td>
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to add rowspan="2" here than duplicate its paragraph below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually – since these two were combined in a single row, the rowspan="2" annotation wasn't needed.

source Outdated
@@ -59471,6 +59473,12 @@ callback <dfn>BlobCallback</dfn> = void (<span>Blob</span>? blob);</code></pre>
<td>
<p>Follow the behavior defined in the WebGL specification. <ref spec=WEBGL></p>
Copy link
Member

Choose a reason for hiding this comment

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

specifications*

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

source Outdated
spec=WEBGL></p>
data-x="canvas-context-bitmaprenderer">bitmaprenderer</code>" contexts below. There are also
specifications that define "<code data-x="canvas-context-webgl">webgl</code>" and "<code
data-x="canvas-context-webgl2">webgl2</code>" contexts. <ref spec=WEBGL></p>
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this "The WebGL specifications define the ..."

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

source Outdated
@@ -59545,7 +59554,8 @@ callback <dfn>BlobCallback</dfn> = void (<span>Blob</span>? blob);</code></pre>
<th><span data-x="concept-canvas-none">none</span>
<th><span data-x="concept-canvas-2d">2d</span>
<th><span data-x="concept-canvas-bitmaprenderer">bitmaprenderer</span>
<th><span data-x="concept-canvas-webgl">webgl</span>
<th><span data-x="concept-canvas-webgl">webgl</span> or <span
data-x="concept-canvas-webgl2">webgl2</span>
Copy link
Member

Choose a reason for hiding this comment

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

The d of data-x needs to be aligned with the < of <th> I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

source Outdated
@@ -65189,7 +65206,8 @@ interface <dfn>OffscreenCanvas</dfn> : <span>EventTarget</span> {
<td>
<th><span data-x="offscreencanvas-context-none">none</span>
<th><span data-x="offscreencanvas-context-2d">2d</span>
<th><span data-x="offscreencanvas-context-webgl">webgl</span>
<th><span data-x="offscreencanvas-context-webgl">webgl</span> or <span
data-x="offscreencanvas-context-webgl2">webgl2</span>
Copy link
Member

Choose a reason for hiding this comment

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

Alignment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

source Outdated
@@ -65211,14 +65229,17 @@ interface <dfn>OffscreenCanvas</dfn> : <span>EventTarget</span> {
<td>
Throw an <span>"<code>InvalidStateError</code>"</span> <code>DOMException</code>.
<tr>
<th>"<dfn><code data-x="offscreen-context-type-webgl">webgl</code></dfn>"
<th>"<dfn><code data-x="offscreen-context-type-webgl">webgl</code></dfn>" or "<dfn><code
data-x="offscreen-context-type-webgl2">webgl2</code></dfn>"
Copy link
Member

Choose a reason for hiding this comment

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

Alignment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

source Outdated
@@ -65211,14 +65229,17 @@ interface <dfn>OffscreenCanvas</dfn> : <span>EventTarget</span> {
<td>
Throw an <span>"<code>InvalidStateError</code>"</span> <code>DOMException</code>.
<tr>
<th>"<dfn><code data-x="offscreen-context-type-webgl">webgl</code></dfn>"
<th>"<dfn><code data-x="offscreen-context-type-webgl">webgl</code></dfn>" or "<dfn><code
data-x="offscreen-context-type-webgl2">webgl2</code></dfn>"
<td>
Follow the instructions given in the WebGL specification's <i>Context Creation</i> section
Copy link
Member

Choose a reason for hiding this comment

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

specifications'*
sections*?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

source Outdated
data-x="offscreencanvas-context-webgl">webgl</span>, and return the
<code>WebGLRenderingContext</code> object. <ref spec=WEBGL>
to obtain either a <code>WebGLRenderingContext</code>, <code>WebGL2RenderingContext</code>
or null; if the returned value is null, then return null; otherwise, set this
Copy link
Member

Choose a reason for hiding this comment

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

Oxford comma

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

source Outdated
contexts is defined in the WebGL specification. <ref spec=WEBGL></p>
<p>The resizing behavior for "<code data-x="offscreen-context-type-webgl">webgl</code>" and "<code
data-x="offscreen-context-type-webgl2">webgl2</code>" contexts is defined in the WebGL
specification. <ref spec=WEBGL></p>
Copy link
Member

Choose a reason for hiding this comment

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

specifications*

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@annevk
Copy link
Member

annevk commented Aug 29, 2018

Also, your name isn't in the Acknowledgments section, please add it?

(And you'll need to make your membership of the google GitHub organization public.)

@kenrussell
Copy link
Member Author

Addressed all review feedback. Added myself to acknowledgments section. Made my Google organization membership public on Github. Please re-review.

source Outdated
<p>Follow the behavior defined in the WebGL specification. <ref spec=WEBGL></p>

<tr>
<tr rowspan="2">
Copy link
Member

Choose a reason for hiding this comment

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

rowspan needs to be on the td, not the tr

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed. Since I merged these rows together it wasn't needed anyway.

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.

LGTM with my own nits applied (please double check). The only question remaining here is tests and I suppose implementer bugs, but I think this has been implemented to some extent already?

kenrussell added a commit to kenrussell/WebGL that referenced this pull request Aug 30, 2018
This test covers the following HTML spec pull request:
whatwg/html#3971
@kenrussell
Copy link
Member Author

Fixes for nits look fine.

Adding a specific test for webgl2 vs. webgl context types in KhronosGroup/WebGL#2701 . Aside from that, https://github.com/KhronosGroup/WebGL already contains a thorough conformance suite for both WebGL 1.0 and 2.0.

@annevk annevk merged commit 3a20936 into whatwg:master Aug 30, 2018
@kenrussell kenrussell deleted the add-webgl2-context branch August 30, 2018 21:45
kenrussell added a commit to KhronosGroup/WebGL that referenced this pull request Aug 30, 2018
This test covers the following HTML spec pull request:
whatwg/html#3971
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants