Summary: | Implement and test new framebuffer object attachment behavior | ||
---|---|---|---|
Product: | WebKit | Reporter: | Kenneth Russell <kbr> |
Component: | WebGL | Assignee: | Zhenyao Mo <zmo> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | cmarrin, commit-queue, dglazkov, fishd, oliver, webkit.review.bot |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | All | ||
OS: | All | ||
Attachments: |
Description
Kenneth Russell
2010-03-02 14:45:25 PST
Created attachment 50648 [details]
patch
Attachment 50648 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/canvas/WebGLRenderingContext.cpp:1226: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Total errors found: 1 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 50649 [details]
fixed style issue
Comment on attachment 50649 [details]
fixed style issue
This looks very good. A couple of minor comments, which from my standpoint aren't necessary to address in this initial patch. First, in calls like WebGLRenderingContext::framebufferRenderbuffer, consider passing DEPTH_STENCIL_ATTACHMENT into the GraphicsContext3D and letting it decide how it wants to dispatch it. This might help in future implementations on top of OpenGL 3 where the DEPTH_STENCIL attachment point actually exists. Second, in the test case, calls like shouldBe("gl.getError() == gl.NO_ERROR", "true") can be better written shouldBe("gl.getError()", "gl.NO_ERROR"). But again this looks very good and well tested.
Created attachment 50855 [details]
revised patch: responding to Ken Russell's review
We need to address http://bugs.webkit.org/show_bug.cgi?id=36194 before considering committing this or any additional WebGL changes. Created attachment 50864 [details]
revised patch : responding to Ken Russell's review
Created attachment 51580 [details]
revised patch: made changes after Ken Russell's refac
Comment on attachment 51580 [details]
revised patch: made changes after Ken Russell's refac
Looks great.
Created attachment 52236 [details]
revised patch: skip the framebuffer-object-attachment.html test on snow leopard
Because the bot does not support EXT_packed_depth_stencil, which is required by WebGL.
Comment on attachment 52236 [details] revised patch: skip the framebuffer-object-attachment.html test on snow leopard > Index: LayoutTests/platform/mac-snowleopard/Skipped > =================================================================== > --- LayoutTests/platform/mac-snowleopard/Skipped (revision 56464) > +++ LayoutTests/platform/mac-snowleopard/Skipped (working copy) > @@ -110,3 +110,7 @@ fast/forms/search-event-delay.html > # computed style on HW transforms are wrong in SL, but > # correct on other platforms. > animations/fill-mode-transform.html > + > +# Snow Leopard bot does not support EXT_packed_depth_stencil, which is required by WebGL. > +# https://bugs.webkit.org/show_bug.cgi?id=51580 This is a bogus bug ID. > +fast/canvas/webgl/framebuffer-object-attachment.html > \ No newline at end of file Created attachment 52238 [details]
revised patch: fixed the bogus bug ID.
Created attachment 52239 [details]
revised patch: fixed the bogus bug ID.
Comment on attachment 52239 [details] revised patch: fixed the bogus bug ID. > Index: LayoutTests/platform/mac-snowleopard/Skipped > =================================================================== > --- LayoutTests/platform/mac-snowleopard/Skipped (revision 56464) > +++ LayoutTests/platform/mac-snowleopard/Skipped (working copy) > @@ -110,3 +110,7 @@ fast/forms/search-event-delay.html > # computed style on HW transforms are wrong in SL, but > # correct on other platforms. > animations/fill-mode-transform.html > + > +# Snow Leopard bot does not support EXT_packed_depth_stencil, which is required by WebGL. > +# https://bugs.webkit.org/show_bug.cgi?id=35611 > +fast/canvas/webgl/framebuffer-object-attachment.html I'm not sure what the preferred style is in these skipped lists, but the last time I modified them, the reviewer wanted a new bug filed which detailed the plan for getting the test out of the skipped list. In this case, it would be to upgrade either the OS or the graphics hardware on the Snow Leopard bot so that it is capable of running WebGL properly. Created attachment 52248 [details] revised patch Created a new bug for snow leopard bot https://bugs.webkit.org/show_bug.cgi?id=36925 The skipped test comment is pointing to the created bug. Created attachment 52249 [details]
revised patch: resolving a conflict
Comment on attachment 52249 [details]
revised patch: resolving a conflict
Looks good to me.
Comment on attachment 52249 [details]
revised patch: resolving a conflict
Looks good as far as I can tell.
Comment on attachment 52249 [details] revised patch: resolving a conflict Clearing flags on attachment: 52249 Committed r57018: <http://trac.webkit.org/changeset/57018> All reviewed patches have been landed. Closing bug. |