Bug 41095 - Bring set/get state functions to GLES2 conformance
Summary: Bring set/get state functions to GLES2 conformance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on:
Blocks: 29230
  Show dependency treegraph
 
Reported: 2010-06-23 12:52 PDT by Zhenyao Mo
Modified: 2010-06-25 21:24 PDT (History)
5 users (show)

See Also:


Attachments
patch (117.39 KB, patch)
2010-06-23 12:56 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: responding to kbr's review (117.24 KB, patch)
2010-06-24 10:13 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: minor fix (116.91 KB, patch)
2010-06-24 13:48 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhenyao Mo 2010-06-23 12:52:58 PDT
functions as list in WebGL spec 5.13.3
Comment 1 Zhenyao Mo 2010-06-23 12:56:18 PDT
Created attachment 59555 [details]
patch
Comment 2 Kenneth Russell 2010-06-23 17:12:44 PDT
Comment on attachment 59555 [details]
patch

WebCore/html/canvas/WebGLRenderingContext.cpp:311
 +          if (!validateBlendMode(mode))
While this name matches the argument name, I think it would be more clear if it were named "validateBlendEquation".


WebCore/html/canvas/WebGLRenderingContext.cpp:1664
 +          }
What about validating "mode"?


WebCore/html/canvas/WebGLRenderingContext.cpp:3516
 +  bool WebGLRenderingContext::validateBlendMode(unsigned long mode)
See above regarding naming.


LayoutTests/fast/canvas/webgl/gl-enum-tests.html:17
 +  description("This test ensures WebGL various functions fail when passed non OpenGL ES 2.0 enums.");
"ensures various WebGL functions"


LayoutTests/fast/canvas/webgl/resources/desktop-gl-constants.js:2642
 +  Added: svn:executable
This shouldn't have the executable bit set.
Comment 3 Zhenyao Mo 2010-06-23 17:40:14 PDT
(In reply to comment #2)
> (From update of attachment 59555 [details])
> WebCore/html/canvas/WebGLRenderingContext.cpp:311
>  +          if (!validateBlendMode(mode))
> While this name matches the argument name, I think it would be more clear if it were named "validateBlendEquation".
> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:1664
>  +          }
> What about validating "mode"?

The "mode" enum list for GENERATE_MIPMAP_HINT is the same for GL and GLES, so we don't need to do it twice.

Will fix the rest.

> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:3516
>  +  bool WebGLRenderingContext::validateBlendMode(unsigned long mode)
> See above regarding naming.
> 
> 
> LayoutTests/fast/canvas/webgl/gl-enum-tests.html:17
>  +  description("This test ensures WebGL various functions fail when passed non OpenGL ES 2.0 enums.");
> "ensures various WebGL functions"
> 
> 
> LayoutTests/fast/canvas/webgl/resources/desktop-gl-constants.js:2642
>  +  Added: svn:executable
> This shouldn't have the executable bit set.
Comment 4 Zhenyao Mo 2010-06-24 10:13:04 PDT
Created attachment 59667 [details]
revised patch: responding to kbr's review
Comment 5 Kenneth Russell 2010-06-24 11:56:16 PDT
Comment on attachment 59667 [details]
revised patch: responding to kbr's review

Couple more comments; sorry for not realizing these before.

> Property changes on: LayoutTests/fast/canvas/webgl/gl-enable-enum-test.html
> ___________________________________________________________________
> Added: svn:executable
>    + *
> 
> Property changes on: LayoutTests/fast/canvas/webgl/gl-enum-tests.html
> ___________________________________________________________________
> Added: svn:executable
>    + *

These files shouldn't have the executable bit set either.

WebCore/html/canvas/WebGLRenderingContext.h:454
 +          // Helper function to validate GL a capability.
validate GL a capability -> validate a GL capability
Comment 6 Zhenyao Mo 2010-06-24 13:48:42 PDT
Created attachment 59694 [details]
revised patch: minor fix
Comment 7 Kenneth Russell 2010-06-24 14:03:36 PDT
Looks good.
Comment 8 Dimitri Glazkov (Google) 2010-06-24 15:12:25 PDT
Comment on attachment 59694 [details]
revised patch: minor fix

holy crap that's a long test :)
Comment 9 WebKit Commit Bot 2010-06-25 21:23:57 PDT
Comment on attachment 59694 [details]
revised patch: minor fix

Clearing flags on attachment: 59694

Committed r61938: <http://trac.webkit.org/changeset/61938>
Comment 10 WebKit Commit Bot 2010-06-25 21:24:02 PDT
All reviewed patches have been landed.  Closing bug.