Bug 31173 - Implement WebGLUniformLocation and change API to use it
Summary: Implement WebGLUniformLocation and change API to use it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Peterson Trethewey
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-05 10:37 PST by Chris Marrin
Modified: 2009-12-08 18:50 PST (History)
6 users (show)

See Also:


Attachments
A patch adding the class WebGLUniformLocation (66.45 KB, patch)
2009-12-07 17:40 PST, Peterson Trethewey
eric: review-
Details | Formatted Diff | Diff
The WebGLUniformLocation change from before plus some style fixes. (69.71 KB, patch)
2009-12-08 14:20 PST, Peterson Trethewey
oliver: review-
Details | Formatted Diff | Diff
Fixed some indenting and added tests. (82.26 KB, patch)
2009-12-08 17:30 PST, Peterson Trethewey
oliver: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Fixed conflicts in WebCore.xcodeproj/project.pbjproj. (83.19 KB, patch)
2009-12-08 18:23 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Removed svn:eol-style properties accidentally introduced for new files in last patch. (82.19 KB, patch)
2009-12-08 18:27 PST, Kenneth Russell
oliver: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2009-11-05 10:37:33 PST
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.
Comment 1 Chris Marrin 2009-11-05 14:20:33 PST
WebGLUniformLocation also needs to be implemented
Comment 2 Peterson Trethewey 2009-12-07 17:40:38 PST
Created attachment 44452 [details]
A patch adding the class WebGLUniformLocation
Comment 3 WebKit Review Bot 2009-12-07 17:45:12 PST
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
Comment 4 Chris Marrin 2009-12-08 07:20:30 PST
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.
Comment 5 Eric Seidel (no email) 2009-12-08 11:18:02 PST
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".
Comment 6 Chris Marrin 2009-12-08 11:23:02 PST
And Textmate will convert all tabs to spaces with one operation
Comment 7 Peterson Trethewey 2009-12-08 14:20:30 PST
Created attachment 44487 [details]
The WebGLUniformLocation change from before plus some style fixes.
Comment 8 Kenneth Russell 2009-12-08 15:25:27 PST
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.
Comment 9 Chris Marrin 2009-12-08 15:32:38 PST
Why hasn't the style bot run on this patch yet?
Comment 10 Oliver Hunt 2009-12-08 15:50:47 PST
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 @@
> +2009-12-08  Peterson Trethewey  <petersont@google.com>
> +
> +        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.
Comment 11 Peterson Trethewey 2009-12-08 17:30:06 PST
Created attachment 44499 [details]
Fixed some indenting and added tests.
Comment 12 Oliver Hunt 2009-12-08 17:47:18 PST
Comment on attachment 44499 [details]
Fixed some indenting and added tests.

r=me
Comment 13 WebKit Commit Bot 2009-12-08 17:50:02 PST
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
Comment 14 Kenneth Russell 2009-12-08 18:23:02 PST
Created attachment 44505 [details]
Fixed conflicts in WebCore.xcodeproj/project.pbjproj.
Comment 15 Kenneth Russell 2009-12-08 18:27:13 PST
Created attachment 44506 [details]
Removed svn:eol-style properties accidentally introduced for new files in last patch.
Comment 16 Oliver Hunt 2009-12-08 18:27:56 PST
Comment on attachment 44506 [details]
Removed svn:eol-style properties accidentally introduced for new files in last patch.

r=me
Comment 17 WebKit Review Bot 2009-12-08 18:28:21 PST
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
Comment 18 Oliver Hunt 2009-12-08 18:29:44 PST
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
Comment 19 Kenneth Russell 2009-12-08 18:32:41 PST
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?
Comment 20 Oliver Hunt 2009-12-08 18:34:44 PST
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)
Comment 21 WebKit Commit Bot 2009-12-08 18:38:15 PST
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>
Comment 22 WebKit Commit Bot 2009-12-08 18:38:45 PST
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
Comment 23 Kenneth Russell 2009-12-08 18:50:26 PST
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.