Bug 31173 – Implement WebGLUniformLocation and change API to use it

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Chris Marrin]

The WebGL spec now uses the WebGLUniformLocation object to pass the uniform location to the uniform*() and getUniform calls. It's also returned from the getUniformLocation call.

[Chris Marrin]

WebGLUniformLocation also needs to be implemented

[Peterson Trethewey]

Created attachment 44452 [details]
A patch adding the class WebGLUniformLocation

[WebKit Review Bot]

Attachment 44452 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 5120 characters of output:
 use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:274:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:275:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:354:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:420:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:582:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:583:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:584:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:584:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:585:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:586:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:587:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:588:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:589:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:590:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:591:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:592:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:620:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:621:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:622:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:622:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:624:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:626:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:626:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:627:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:628:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:628:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:703:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/html/canvas/WebGLRenderingContext.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/html/canvas/WebGLUniformLocation.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/html/canvas/WebGLUniformLocation.h:37:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/html/canvas/WebGLUniformLocation.h:39:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/html/canvas/WebGLUniformLocation.h:43:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/html/canvas/WebGLUniformLocation.h:45:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/html/canvas/WebGLUniformLocation.h:52:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/html/canvas/WebGLUniformLocation.cpp:30:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:240:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:242:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:288:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:290:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:294:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:467:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:484:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 48

[Chris Marrin]

Comment on attachment 44452 [details]
A patch adding the class WebGLUniformLocation

> Index: WebCore/html/canvas/WebGLRenderingContext.cpp
> ===================================================================
> --- WebCore/html/canvas/WebGLRenderingContext.cpp	(revision 51799)
> +++ WebCore/html/canvas/WebGLRenderingContext.cpp	(working copy)
> -long WebGLRenderingContext::getUniformLocation(WebGLProgram* program, const String& name, ExceptionCode& ec)
> +PassRefPtr<WebGLUniformLocation> WebGLRenderingContext::getUniformLocation(WebGLProgram* program, const String& name, ExceptionCode& ec)
>  {
>      if (!program || program->context() != this) {
>          ec = TYPE_MISMATCH_ERR;
>          return 0;
>      }
> -    return m_context->getUniformLocation(program, name);
> +    RefPtr<WebGLUniformLocation> location = WebGLUniformLocation::create(program, m_context->getUniformLocation(program, name));
> +    return location;
>  }

Why not just:

    return WebGLUniformLocation::create(program, m_context->getUniformLocation(program, name));

?

> Index: WebCore/html/canvas/WebGLRenderingContext.h
> ===================================================================
> --- WebCore/html/canvas/WebGLRenderingContext.h	(revision 51799)
> +++ WebCore/html/canvas/WebGLRenderingContext.h	(working copy)
> @@ -32,6 +32,7 @@
>  #include "WebGLGetInfo.h"
>  #include "WebGLIntArray.h"
>  #include "WebGLUnsignedByteArray.h"
> +#include "WebGLUniformLocation.h"

You should be able to use

    class WebGLUniformLocation;

and avoid the inclusion of the .h here

>  #include "GraphicsContext3D.h"
>  #include "PlatformString.h"
>  
> @@ -157,9 +158,9 @@ class WebKitCSSMatrix;
>  
>          WebGLGetInfo getTexParameter(unsigned long target, unsigned long pname, ExceptionCode&);
>  
> -        WebGLGetInfo getUniform(WebGLProgram* program, long location, ExceptionCode& ec);
> +        WebGLGetInfo getUniform(WebGLProgram* program, const WebGLUniformLocation* location, ExceptionCode& ec);

You can leave off "ec" and "location". The type name is sufficient.

[Eric Seidel (no email)]

Comment on attachment 44452 [details]
A patch adding the class WebGLUniformLocation

The tabs will make this patch impossible to land as-is.  I suggest running check-webkit-style locally and re-posting.

XCode has a setting for using spaces instead of tabs in the preferences under "Text Editing".

[Chris Marrin]

And Textmate will convert all tabs to spaces with one operation

[Peterson Trethewey]

Created attachment 44487 [details]
The WebGLUniformLocation change from before plus some style fixes.

[Kenneth Russell]

FWIW, these changes look good to me. Would be great if we could get these landed soon because other changes being worked on collide with them.

[Chris Marrin]

Why hasn't the style bot run on this patch yet?

[Oliver Hunt]

Comment on attachment 44487 [details]
The WebGLUniformLocation change from before plus some style fixes.

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 51872)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,69 @@
> [phone]  Peterson Trethewey  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Implement WebGLUniformLocation and change API to use it.
> +        https://bugs.webkit.org/show_bug.cgi?id=31173
> +
> +        No new tests: tests which call getUniformLocation already exist.  That function returns a
> +        WebGLUniformLocation now instead of an integer, but the way the API gets used hasn't
> +        changed.

There should be new tests, at least the following:
  Using an integer instead of a UniformLocation
  Using a UniformLocation from another context
  Using a UniformLocation with a field of a struct
  Using a UniformLocation with a field of a matrix
  Using a UniformLocation after changing the active shader on a context
  Using a UniformLocation with a separately compiled copy of the same shader

etc, etc

You want your test cases to cover as many scenarios as possible, not necessarily just the correct uses :D

> Index: WebCore/html/canvas/WebGLUniformLocation.h
> ===================================================================
> --- WebCore/html/canvas/WebGLUniformLocation.h	(revision 0)
> +++ WebCore/html/canvas/WebGLUniformLocation.h	(revision 0)
> @@ -0,0 +1,58 @@
> +
> +class WebGLUniformLocation : public RefCounted<WebGLUniformLocation> {
> +    public:
Incorrect indenting
> +    virtual ~WebGLUniformLocation() { }
> +        
> +        static PassRefPtr<WebGLUniformLocation> create(WebGLProgram* program, long location);
Incorrect indenting

> +    
> +    WebGLProgram* program() const { return m_program.get(); }
> +    
> +    long location() const { return m_location; }
> +    
<start incorrect indent>
> +    protected:
> +        WebGLUniformLocation(WebGLProgram* program, long location);
> +    
> +    private:
> +        RefPtr<WebGLProgram> m_program;
</end incorrect indent> :D
> +    long m_location;
> +};
> +    

Okay, with the exception of the minor style problems the code appears to be fine,
my only real problem is the absence of test cases which is why I am r-'ing this patch.

[Peterson Trethewey]

Created attachment 44499 [details]
Fixed some indenting and added tests.

[Oliver Hunt]

Comment on attachment 44499 [details]
Fixed some indenting and added tests.

r=me

[WebKit Commit Bot]

Comment on attachment 44499 [details]
Fixed some indenting and added tests.

Rejecting patch 44499 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Oliver Hunt', '--force']" exit_code: 1
Last 500 characters of output:
ormLocation.cpp
patching file WebCore/html/canvas/WebGLUniformLocation.h
patching file WebCore/html/canvas/WebGLUniformLocation.idl
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/canvas/webgl/uniform-location-expected.txt
patching file LayoutTests/fast/canvas/webgl/uniform-location.html
patching file LayoutTests/fast/canvas/webgl/resources/structUniformShader.vert
patching file LayoutTests/fast/canvas/webgl/script-tests/uniform-location.js

[Kenneth Russell]

Created attachment 44505 [details]
Fixed conflicts in WebCore.xcodeproj/project.pbjproj.

[Kenneth Russell]

Created attachment 44506 [details]
Removed svn:eol-style properties accidentally introduced for new files in last patch.

[Oliver Hunt]

Comment on attachment 44506 [details]
Removed svn:eol-style properties accidentally introduced for new files in last patch.

r=me

[WebKit Review Bot]

Attachment 44506 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:501:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:510:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:523:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:532:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:554:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:560:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:573:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:579:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:605:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:609:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:622:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:626:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 12

[Oliver Hunt]

Comment on attachment 44506 [details]
Removed svn:eol-style properties accidentally introduced for new files in last patch.

sigh, apparently the style bot disagrees -- can yet another updated version be made? sorry

[Kenneth Russell]

What happened with the patch 44505 which was submitted to the commit queue? Sorry I didn't notice the svn:eol-style properties on that.

The bot is complaining about preexisting case statements with incorrect indentation. Do you want me to fix the indentation, which will add more changes to the patch, or should we fix them in another cleanup bug?

[Oliver Hunt]

Comment on attachment 44506 [details]
Removed svn:eol-style properties accidentally introduced for new files in last patch.

The style bot is apparently complaining about pre-existing formatting issues, we can fix later.

r=me (based on prior review)

[WebKit Commit Bot]

Comment on attachment 44505 [details]
Fixed conflicts in WebCore.xcodeproj/project.pbjproj.

Clearing flags on attachment: 44505

Committed r51884: <http://trac.webkit.org/changeset/51884>

[WebKit Commit Bot]

Comment on attachment 44506 [details]
Removed svn:eol-style properties accidentally introduced for new files in last patch.

Rejecting patch 44506 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Oliver Hunt', '--force']" exit_code: 1
Last 500 characters of output:
ormLocation.cpp
patching file WebCore/html/canvas/WebGLUniformLocation.h
patching file WebCore/html/canvas/WebGLUniformLocation.idl
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/canvas/webgl/uniform-location-expected.txt
patching file LayoutTests/fast/canvas/webgl/uniform-location.html
patching file LayoutTests/fast/canvas/webgl/resources/structUniformShader.vert
patching file LayoutTests/fast/canvas/webgl/script-tests/uniform-location.js

[Kenneth Russell]

Comment on attachment 44506 [details]
Removed svn:eol-style properties accidentally introduced for new files in last patch.

Attachment 44505 [details] was committed and it turns out the svn:eol-style properties it contained weren't applied to the new files. Marking this patch obsolete.