Bug 39855

Summary: Map GLsizei to long instead of unsigned long in WebGLRenderingContext and GraphicsContext3D
Product: WebKit Reporter: Zhenyao Mo <zmo>
Component: WebGLAssignee: Zhenyao Mo <zmo>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, enne, kbr, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch kbr: review+

Zhenyao Mo
Reported 2010-05-27 16:02:27 PDT
Currently GLsizei is mapped to unsigned, so instead of generating an INVALID_VALUE error, negative level/border/xoffset/yoffset/width/height are mapped to a very large positive number, which causes buffer size validation to fail (once we have them). Also, some functions map enum to unsigned, and some to unsigned long.
Attachments
Patch (20.93 KB, patch)
2010-12-23 15:13 PST, Zhenyao Mo 		no flags
DetailsFormatted DiffDiff
Patch (27.86 KB, patch)
2010-12-28 11:10 PST, Zhenyao Mo 		kbr: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Zhenyao Mo
Comment 1 2010-12-23 15:13:11 PST
Created attachment 77374 [details] Patch
Zhenyao Mo
Comment 2 2010-12-23 15:16:18 PST
There are a bunch of wrong types in GraphicsContext3D. It's less important, and it's much more than the violations in WebGLRenderingContext, so I don't include the fix in the same patch.
David Levin
Comment 3 2010-12-27 09:32:04 PST
Comment on attachment 77374 [details] Patch The code changes looks fine, but it should have some tests. It seems like you could call these method with negative values and verify that you get a failure.
Kenneth Russell
Comment 4 2010-12-27 20:41:13 PST
Comment on attachment 77374 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77374&action=review I'd like to point out that this patch just brings WebKit's implementation closer to spec compliance; GLsizei is typedef'ed to "long" in the WebGL spec, but up until this point we've had the wrong mapping in the associated WebKit IDL. Mo would know better than me how practical it is to test all of the entry points affected by this change. > WebCore/ChangeLog:8 > + No new tests. (OOPS!) This OOPS needs to either be fixed (by adding a new test) or removed.
Zhenyao Mo
Comment 5 2010-12-28 11:10:29 PST
Created attachment 77563 [details] Patch
Zhenyao Mo
Comment 6 2010-12-28 11:11:32 PST
(In reply to comment #5) > Created an attachment (id=77563) [details] > Patch This patch only added test cases to cover the affected functions. The OOPS is removed. Will sync with khronos once this is reviewed.
Kenneth Russell
Comment 7 2010-12-28 11:15:13 PST
Comment on attachment 77563 [details] Patch Looks excellent. Thanks for handling this and for the added test cases.
Zhenyao Mo
Comment 8 2010-12-28 13:27:23 PST
Committed r74719: <http://trac.webkit.org/changeset/74719>
Note You need to log in before you can comment on or make changes to this bug.