Bug 100777 – Provide return types for custom WebGLRenderingContext methods

• Component: New Bugs
• Status: RESOLVED FIXED

[anton muhin]

Provide return types for custom WebGLRenderingContext methods

[anton muhin]

Created attachment 171493 [details]
Patch

[Early Warning System Bot]

Comment on attachment 171493 [details]
Patch

Attachment 171493 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14626678

[Early Warning System Bot]

Comment on attachment 171493 [details]
Patch

Attachment 171493 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14557069

[anton muhin]

Created attachment 171498 [details]
Patch

[Kenneth Russell]

Did you run the WebGL conformance suite at https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/webgl-conformance-tests.html to ensure nothing was broken?

Clearly the autogenerated code changes somewhat, as otherwise the Qt and Qt-WK2 bots wouldn't have been broken with the first version of the patch...

[Dean Jackson]

(In reply to comment #5)
> Did you run the WebGL conformance suite at https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/webgl-conformance-tests.html to ensure nothing was broken?

Maybe we should have reviewbot add a warning for any WebGL code changes until we've incorporated the Khronos tests into WebKit? Something like what Ken says above.

[anton muhin]

(In reply to comment #5)
> Did you run the WebGL conformance suite at https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/webgl-conformance-tests.html to ensure nothing was broken?
> 
> Clearly the autogenerated code changes somewhat, as otherwise the Qt and Qt-WK2 bots wouldn't have been broken with the first version of the patch...

No, Ken, thanks a lot for pointing out the suite, I'll run it.

Regarding no code changes: I should have been more explicit---no changes in v8 bindings.  Overall, if I run the suite under Chromium with this patch in, will it be enough?

What is the best approach to test that patch against remaining platforms?

[Pavel Podivilov]

Comment on attachment 171498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171498&action=review

> Source/WebCore/html/canvas/WebGLRenderingContext.idl:533
>      // object getExtension(in DOMString name);

Please remove the comment.

> Source/WebCore/html/canvas/WebGLRenderingContext.idl:554
>      // DOMString[] getSupportedExtensions()

ditto

[Dean Jackson]

(In reply to comment #7)

> What is the best approach to test that patch against remaining platforms?

There isn't any EWS, so you'd have to try on the platforms manually if you can. The latest Khronos test suite has a nice tool to run the tests and gather a report.

I guess if you don't have access to the platforms, you'll just have to land confidently :)

[Adam Barth]

Comment on attachment 171498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171498&action=review

> Source/WebCore/html/canvas/WebGLRenderingContext.idl:555
> +    [StrictTypeChecking, Custom] DOMString[] getSupportedExtensions();

Do you really mean DOMString?  I would have expected this to return sequence<DOMString>

[Adam Barth]

Comment on attachment 171498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171498&action=review

>> Source/WebCore/html/canvas/WebGLRenderingContext.idl:555
>> +    [StrictTypeChecking, Custom] DOMString[] getSupportedExtensions();
> 
> Do you really mean DOMString?  I would have expected this to return sequence<DOMString>

Let me try again:

Do you really mean DOMString[]?  I would have expected this to return sequence<DOMString>

[Kenneth Russell]

(In reply to comment #7)
> (In reply to comment #5)
> > Did you run the WebGL conformance suite at https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/webgl-conformance-tests.html to ensure nothing was broken?
> > 
> > Clearly the autogenerated code changes somewhat, as otherwise the Qt and Qt-WK2 bots wouldn't have been broken with the first version of the patch...
> 
> No, Ken, thanks a lot for pointing out the suite, I'll run it.
> 
> Regarding no code changes: I should have been more explicit---no changes in v8 bindings.  Overall, if I run the suite under Chromium with this patch in, will it be enough?

Ideally you'd run it on Mac OS with Safari as well to test the JSC bindings. However, the EWS bots run some subset of the WebGL tests so you will probably be OK.

> What is the best approach to test that patch against remaining platforms?

I generally test WebGL patches on Mac OS with both Chromium and Safari, and rely on the EWS to catch build failures, etc. affecting other ports.

[Kenneth Russell]

(In reply to comment #11)
> (From update of attachment 171498 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171498&action=review
> 
> >> Source/WebCore/html/canvas/WebGLRenderingContext.idl:555
> >> +    [StrictTypeChecking, Custom] DOMString[] getSupportedExtensions();
> > 
> > Do you really mean DOMString?  I would have expected this to return sequence<DOMString>
> 
> Let me try again:
> 
> Do you really mean DOMString[]?  I would have expected this to return sequence<DOMString>

Right, Adam's correct; see http://www.khronos.org/registry/webgl/specs/latest/ .

[anton muhin]

Created attachment 173930 [details]
Patch

[anton muhin]

Sorry for late responses.

Two test of the suite fail:

conformance/textures/texture-mips.html (17 of 19 passed)
failed: texture that is only using the smallest 2 mips should draw with green
failed: texture that is only using smallest mips should draw with cyan

but those tests fail w/o the patch.

Regarding comments and sequence.  I tried to keep the simplest IDL close to the latest spec, hence keeping comments if current decl differs from the spec and not using sequence<DOMString> which might be not supported by other platforms like object with which I started.

I am uploading new patch with sequence<DOMString> to see if it passes EWS.


(In reply to comment #13)
> (In reply to comment #11)
> > (From update of attachment 171498 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=171498&action=review
> > 
> > >> Source/WebCore/html/canvas/WebGLRenderingContext.idl:555
> > >> +    [StrictTypeChecking, Custom] DOMString[] getSupportedExtensions();
> > > 
> > > Do you really mean DOMString?  I would have expected this to return sequence<DOMString>
> > 
> > Let me try again:
> > 
> > Do you really mean DOMString[]?  I would have expected this to return sequence<DOMString>
> 
> Right, Adam's correct; see http://www.khronos.org/registry/webgl/specs/latest/ .

[Kenneth Russell]

Comment on attachment 173930 [details]
Patch

Nice, thanks. r=me. Please wait for this to pass the EWS before cq+'ing.

[anton muhin]

Everything looks green, cq'ing while the repo is quite.

Many thanks to all for review and comments.

(In reply to comment #16)
> (From update of attachment 173930 [details])
> Nice, thanks. r=me. Please wait for this to pass the EWS before cq+'ing.

[WebKit Review Bot]

Comment on attachment 173930 [details]
Patch

Clearing flags on attachment: 173930

Committed r134580: <http://trac.webkit.org/changeset/134580>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.