[chromium][Not for review] Add a setVisibility method to WebGraphicsContext3D.
Created attachment 108852 [details] Patch
Created attachment 111025 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment on attachment 111025 [details] Patch Attachment 111025 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10066573
Kenneth, it looked like you were a good reviewer for this patch, but if someone else is more appropriate, please let me know (and sorry to bother). This patch will give WebGraphicsContext3D implementations the opportunity to release some needless graphics resources.
Comment on attachment 111025 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111025&action=review > Source/WebKit/chromium/public/WebGraphicsContext3D.h:144 > + virtual void setVisibility(bool visible) = 0; It seems clear that this is only useful for the compositor's context. What resources are released when called with argument "false"? Would it make more sense to just destroy the context and recreate it from scratch when visibility becomes true again? Without knowing more, it seems to me that any releasing of resources would be better done at the WebKit level. Also, you would need to either temporarily provide an empty body for this or land all of the Chromium side implementations first (and roll Source/WebKit/chromium/DEPS) in order to fix the pure virtual compilation problem on the EWS bots.
I'm not sure that I understand the comment about being compositor specific. One could imagine other contexts (like Pepper3D or WebGL) wanting to release some resources if they aren't visible. E.g. AFAIK for WebGL, we double buffer; we could release one of those buffers in the GLES2Decoder if we had the appropriate signals plumbed (perhaps we'd only want to do that when the compositor context is not visible?).
(In reply to comment #3) > Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. I defer to Ken Russell for this change. I don't need to review it further.
(In reply to comment #7) > I'm not sure that I understand the comment about being compositor specific. One could imagine other contexts (like Pepper3D or WebGL) wanting to release some resources if they aren't visible. E.g. AFAIK for WebGL, we double buffer; we could release one of those buffers in the GLES2Decoder if we had the appropriate signals plumbed (perhaps we'd only want to do that when the compositor context is not visible?). The signaling in the current patch is compositor specific so the intent wasn't clear. The direction WebGL is going is to move the back buffer handling up to WebKit. See bug 53201. This means that any discarding of secondary buffers could be done in common code without plumbing anything through the WebGraphicsContext3D interface. The issue of secondary buffers doesn't really become relevant for WebGL until threaded compositing is in place though. I'd still like to see the Chromium side patch that uses this signal.
(In reply to comment #9) > (In reply to comment #7) > > I'm not sure that I understand the comment about being compositor specific. One could imagine other contexts (like Pepper3D or WebGL) wanting to release some resources if they aren't visible. E.g. AFAIK for WebGL, we double buffer; we could release one of those buffers in the GLES2Decoder if we had the appropriate signals plumbed (perhaps we'd only want to do that when the compositor context is not visible?). > > The signaling in the current patch is compositor specific so the intent wasn't clear. > > The direction WebGL is going is to move the back buffer handling up to WebKit. See bug 53201. This means that any discarding of secondary buffers could be done in common code without plumbing anything through the WebGraphicsContext3D interface. The issue of secondary buffers doesn't really become relevant for WebGL until threaded compositing is in place though. > > I'd still like to see the Chromium side patch that uses this signal. The Chromium patch is: http://codereview.chromium.org/7890046
Comment on attachment 111025 [details] Patch OK, this seems fine at least as an interim solution. Source/WebKit/chromium/DEPS has since been rolled forward, so I think this will pass the commit queue now. r=me
Comment on attachment 111025 [details] Patch Rejecting attachment 111025 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: pport/Source/WebKit/chromium/webkit/support/test_webkit_platform_support.o] Error 1 make: *** Waiting for unfinished jobs.... Failed to run "['Tools/Scripts/build-webkit', '--debug', '--chromium', '--update-chromium']" exit_code: 2 c/WebGraphicsContext3D.h:144: note: virtual void WebKit::WebGraphicsContext3D::setVisibility(bool) make: *** [out/Debug/obj.target/webkit_support/Source/WebKit/chromium/webkit/support/test_webkit_platform_support.o] Error 1 make: *** Waiting for unfinished jobs.... Full output: http://queues.webkit.org/results/10142031
Michal, you'll have to add the new virtual to src/webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc / .h downstream in Chromium, and once that's committed, roll forward chromium_rev in Source/WebKit/chromium/DEPS.
Created attachment 111812 [details] Patch
Comment on attachment 111812 [details] Patch Looks good to me.
Comment on attachment 111812 [details] Patch Clearing flags on attachment: 111812 Committed r98036: <http://trac.webkit.org/changeset/98036>
All reviewed patches have been landed. Closing bug.