Bug 111641 - Conformance Test 1.0.3 (Beta) function: bufferData undefined value failed.
Summary: Conformance Test 1.0.3 (Beta) function: bufferData undefined value failed.
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Keywords: InRadar
Depends on:
Reported: 2013-03-06 16:14 PST by Jason Anderssen
Modified: 2013-03-10 15:39 PDT (History)
5 users (show)

See Also:

Patch (1.70 KB, patch)
2013-03-06 20:12 PST, Jason Anderssen
no flags Details | Formatted Diff | Diff
Patch (2.12 KB, patch)
2013-03-09 16:13 PST, Jason Anderssen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Anderssen 2013-03-06 16:14:02 PST
Base on the conformance test from kronos group : https://www.khronos.org/registry/webgl/sdk/tests/conformance/more/functions/bufferDataBadArgs.html, 
bufferData was not protecting against an undefined value being provided.
Comment 1 Jason Anderssen 2013-03-06 20:12:10 PST
Created attachment 191905 [details]
Comment 2 Radar WebKit Bug Importer 2013-03-08 02:21:22 PST
Comment 3 Dean Jackson 2013-03-08 02:27:28 PST
Comment on attachment 191905 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=191905&action=review

These may seem like nitpicking comments (and they are) but WebKit is especially strict about coding style. Sorry. You get used to it :) Make these easy fixes and the r+ is yours.

> Source/WebCore/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).
> +

In this space you should try to describe the bug and change in a little more detail. e.g. "The WebGL specification requires that buffer data cannot be of zero length. This is a new test in the Khronos 1.0.3 WebGL test suite."

Also, you should reference the Khronos test here. Typically we require a test for every WebKit commit, but in this case it's ok because we are in the process of incorporating the Khronos tests into WebKit, so it will be there eventually.

> Source/WebCore/ChangeLog:9
> +        (WebCore):

Remove this line.

> Source/WebCore/ChangeLog:10
> +        (WebCore::WebGLRenderingContext::bufferData):

You should always try to explain what you did in the changelog. In this case it is really easy - just something like "Synthesize error and return if size was 0."

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1115
> +        // undefined in WEBGL will parse in 0.

No need for this comment. Also, for future reference, WebKit coding style requires comments to start with a capital letter.
Comment 4 Jason Anderssen 2013-03-09 16:13:43 PST
Created attachment 192356 [details]
Comment 5 Dean Jackson 2013-03-10 15:31:42 PDT
Comment on attachment 192356 [details]

Looks good. I missed this in the first pass, but we could have simply changed the test above to if (size <= 0), but this is explicit for the undefined case so I guess it is ok.
Comment 6 WebKit Review Bot 2013-03-10 15:39:46 PDT
Comment on attachment 192356 [details]

Clearing flags on attachment: 192356

Committed r145334: <http://trac.webkit.org/changeset/145334>
Comment 7 WebKit Review Bot 2013-03-10 15:39:49 PDT
All reviewed patches have been landed.  Closing bug.