Expose EXT_occlusion_query extensions to WebKit compositor.
Created attachment 131623 [details] Patch
Created attachment 131625 [details] Patch
Created attachment 131630 [details] Patch Fix typo
Enne, are you comfortable reviewing WGC3D extensions? @kbr is out. Happy to bounce to James but I know his queue is deep.
Comment on attachment 131630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131630&action=review > Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:49 > + // GL_CHROMIUM_fence Is this what you want to call it? GL_CHROMIM_commands_issued_occlusion_query? > Source/WebKit/chromium/tests/FakeWebGraphicsContext3D.h:258 > + virtual WebGLId createQueryEXT() { return 1; } Is 1 a safe return value?
Comment on attachment 131630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131630&action=review > Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:90 > + // GL_CHROMIUM_fence > + COMMANDS_ISSUED_CHROMIUM = 0x84F2 Where does this get used? Are there not fence setting or checking functions? >> Source/WebKit/chromium/tests/FakeWebGraphicsContext3D.h:258 >> + virtual WebGLId createQueryEXT() { return 1; } > > Is 1 a safe return value? For this fake WGC3D, yes. It's what we do above for other object-creating functions. If some test needs to create real IDs, it can override and do that itself.
(In reply to comment #5) > (From update of attachment 131630 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131630&action=review > > > Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:49 > > + // GL_CHROMIUM_fence > > Is this what you want to call it? GL_CHROMIM_commands_issued_occlusion_query? Maybe GL_CHROMIM_occlusion_query_commands_issued in that case but in this patch I'm just being consistent with the extension name in Gregg's CL http://codereview.chromium.org/9694025/ so lets move the name discussion over there instead. > > > Source/WebKit/chromium/tests/FakeWebGraphicsContext3D.h:258 > > + virtual WebGLId createQueryEXT() { return 1; } > > Is 1 a safe return value? not used at the moment and returning 1 is consistent with the other fake createX() functions.
(In reply to comment #6) > (From update of attachment 131630 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131630&action=review > > > Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:90 > > + // GL_CHROMIUM_fence > > + COMMANDS_ISSUED_CHROMIUM = 0x84F2 > > Where does this get used? Are there not fence setting or checking functions? The name doesn't currently reflect this but it's an extension on top of the GL_EXT_occlusion_query extension. You can find a usage example in this patch: https://bugs.webkit.org/show_bug.cgi?id=81004 and maybe also as part of the unit test for this CL: http://codereview.chromium.org/9694025/
Created attachment 131763 [details] Patch
Please wait for approval from fishd@chromium.org, abarth@webkit.org or jamesr@chromium.org before submitting because this patch contains changes to the Chromium platform API.
Attachment 131763 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1 Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 132504 [details] Patch
Please review the patch. The following chromium change need to land before we can put it in the commit queue. https://chromiumcodereview.appspot.com/9719030/
Comment on attachment 132504 [details] Patch Sure! Looks good to me.
Comment on attachment 132504 [details] Patch All required chromium side changes have landed.
jamesr: Can you approve the WebGraphicsContext3D API changes?
Comment on attachment 132504 [details] Patch Looks OK
Comment on attachment 132504 [details] Patch Clearing flags on attachment: 132504 Committed r111207: <http://trac.webkit.org/changeset/111207>
All reviewed patches have been landed. Closing bug.
Reopening to attach new patch.
Created attachment 132625 [details] Patch
Whoops, wrong bug
Rolled out in bug 81561.
Created attachment 132862 [details] Patch Add base implementations of EXT_occlusion_query functions to be consistent with other extensions.
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
API changes ok. Didn't look at impl.
How were missing implementations causing test errors? Or, am I misunderstanding what the problem was?
Spent a significant amount of time looking at what could possible cause the runtime problem that was the reason for reverting this patch. The only difference I've found compared to other extensions is the lack of base implementations for functions in WebGraphicsContext3D.h. I changed this in the latest patch to be consistent with other extension functions, although I'm not sure how this change can affect a runtime error.
(In reply to comment #28) > Spent a significant amount of time looking at what could possible cause the runtime problem that was the reason for reverting this patch. The only difference I've found compared to other extensions is the lack of base implementations for functions in WebGraphicsContext3D.h. I changed this in the latest patch to be consistent with other extension functions, although I'm not sure how this change can affect a runtime error. Yeah, it certainly doesn't sound like that would result in runtime errors. If you haven't already, could you run a try job with this patch to run whatever tests were failing just as a sanity check? If that passes, I'm happy to re-R+/CQ+ this.
Comment on attachment 132862 [details] Patch http://build.chromium.org/p/tryserver.chromium/builders/mac_gpu/builds/92 looks clean. Let's try this again.
Comment on attachment 132862 [details] Patch Clearing flags on attachment: 132862 Committed r111597: <http://trac.webkit.org/changeset/111597>