Summary: | Conformance Test 1.0.3 (Beta) function: bufferData undefined value failed. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jason Anderssen <janderssen> | ||||||
Component: | WebGL | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | dino, esprehn+autocc, ojan.autocc, webkit-bug-importer, webkit.review.bot | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Jason Anderssen
2013-03-06 16:14:02 PST
Created attachment 191905 [details]
Patch
Comment on attachment 191905 [details] Patch 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. Created attachment 192356 [details]
Patch
Comment on attachment 192356 [details]
Patch
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 on attachment 192356 [details] Patch Clearing flags on attachment: 192356 Committed r145334: <http://trac.webkit.org/changeset/145334> All reviewed patches have been landed. Closing bug. |