Bug 53722 – Hook up WebGraphicsContext3D::setContextLostCallback.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Alexey Marinichev]

This adds necessary plumbing to forward the context lost callback from command buffer to WebGraphicsContext3D.

[Alexey Marinichev]

Created attachment 81129 [details]
Patch

[Alexey Marinichev]

Created attachment 81163 [details]
Patch

[Kenneth Russell]

Comment on attachment 81163 [details]
Patch

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

Looks good overall. r- for the minor issue of the naked new which needs to be cleaned up. One question.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:378
> +    m_context->setContextLostCallback(new WebGLRenderingContextLostCallback(this));

naked new. Should use adoptPtr(new ...);

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:1110
> +    m_impl->setContextLostCallback(m_contextLostCallbackAdapter.get());

Is there an ownership issue here? Does the WebGraphicsContext3D take ownership of the passed callback like GraphicsContext3D and WebGLRenderingContext or not?

[Alexey Marinichev]

Created attachment 81300 [details]
Patch

[Alexey Marinichev]

> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:378
> > +    m_context->setContextLostCallback(new WebGLRenderingContextLostCallback(this));
> 
> naked new. Should use adoptPtr(new ...);

Done.

> 
> > Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:1110
> > +    m_impl->setContextLostCallback(m_contextLostCallbackAdapter.get());
> 
> Is there an ownership issue here? Does the WebGraphicsContext3D take ownership of the passed callback like GraphicsContext3D and WebGLRenderingContext or not?

WebGraphicsContext3DCommandBufferImpl has an OnSwapBuffers call, OnContextLost was added to it in a similar fashion.  Whoever owns WebGraphicsContext3D gets to own the callback.

GraphicsContext3DInternal::m_contextLostCallbackAdapter is an adapter to allow using GraphicsContext3D::ContextLostCallback in WebGraphicsContext3D that does not know about GraphicsContext3D.

WebGLRenderingContext doesn't seem to own GraphicsContext3D -- it has it as RefPtr.  It looks like WebGLRenderingContextLostCallback::m_contextLostCallback should really be a weak pointer -- we don't want to crash if WebGLRenderingContext is already gone and somebody still holds on to GraphicsContext3D.

Need to redo WebGLRenderingContextLostCallback!

[Alexey Marinichev]

Created attachment 81327 [details]
Patch

[Alexey Marinichev]

The callback is owned by the longest-lived object.

When WebGLRenderingContext's destructor is called, it will unregister context lost callback from m_context, so callback for stale context will not be called.

[Alexey Marinichev]

Created attachment 81333 [details]
Patch

[Alexey Marinichev]

Removed webkit.pyc-2.4 that shouldn't be there.

[Kenneth Russell]

Comment on attachment 81333 [details]
Patch

Looks good to me.

[WebKit Commit Bot]

Comment on attachment 81333 [details]
Patch

Rejecting attachment 81333 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build'..." exit_code: 2

Last 500 characters of output:
-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/WebKitLoseContext.o /mnt/git/webkit-commit-queue/Source/WebCore/html/canvas/WebKitLoseContext.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
	CompileC /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/WebGLRenderingContext.o /mnt/git/webkit-commit-queue/Source/WebCore/html/canvas/WebGLRenderingContext.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(35 failures)


Full output: http://queues.webkit.org/results/7697740

[Alexey Marinichev]

Created attachment 81374 [details]
added virtual destructors

[Kenneth Russell]

Comment on attachment 81374 [details]
added virtual destructors

Let's try again.

[WebKit Commit Bot]

Comment on attachment 81374 [details]
added virtual destructors

Rejecting attachment 81374 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-a..." exit_code: 2

Last 500 characters of output:
OPS" in a ChangeLog file.
 at /usr/local/git/libexec/git-core/git-svn line 574


Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Updating OpenSource
From git://git.webkit.org/WebKit
   1a2fd8e..9df0b31  master     -> origin/master
	M	Source/WebKit2/ChangeLog
	M	Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp
r77759 = 9df0b31bee4fd30e9b61fe18d160d81c9eba4cf7 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.

Full output: http://queues.webkit.org/results/7698959

[Kenneth Russell]

Looks like the failure was caused by the extra section in Source/WebKit/chromium/ChangeLog.

[Alexey Marinichev]

Created attachment 81483 [details]
reran prepare-ChangeLog yet again

[Alexey Marinichev]

Created attachment 81489 [details]
rebased, cleaned up changelog

[Kenneth Russell]

Comment on attachment 81489 [details]
rebased, cleaned up changelog

Looks fine to me; hopefully third time's the charm.

[WebKit Commit Bot]

Comment on attachment 81489 [details]
rebased, cleaned up changelog

Clearing flags on attachment: 81489

Committed r77900: <http://trac.webkit.org/changeset/77900>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/77900 might have broken Qt Linux Release
The following tests are not passing:
http/tests/websocket/tests/websocket-protocol-ignored.html