[webkit-reviews] review denied: [Bug 31173] Implement WebGLUniformLocation and change API to use it : [Attachment 44487] The WebGLUniformLocation change from before plus some style fixes.

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Peterson Trethewey
<petersont at google.com>'s request for review:
Bug 31173: Implement WebGLUniformLocation and change API to use it
https://bugs.webkit.org/show_bug.cgi?id=31173

Attachment 44487: The WebGLUniformLocation change from before plus some style
fixes.
https://bugs.webkit.org/attachment.cgi?id=44487&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 51872)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,69 @@
> +2009-12-08  Peterson Trethewey  <petersont at 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.