Bug 53722 - Hook up WebGraphicsContext3D::setContextLostCallback.
Summary: Hook up WebGraphicsContext3D::setContextLostCallback.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Alexey Marinichev
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-03 15:10 PST by Alexey Marinichev
Modified: 2011-02-07 22:54 PST (History)
5 users (show)

See Also:


Attachments
Patch (7.66 KB, patch)
2011-02-03 15:51 PST, Alexey Marinichev
no flags Details | Formatted Diff | Diff
Patch (14.92 KB, patch)
2011-02-03 18:15 PST, Alexey Marinichev
no flags Details | Formatted Diff | Diff
Patch (14.93 KB, patch)
2011-02-04 15:08 PST, Alexey Marinichev
no flags Details | Formatted Diff | Diff
Patch (15.19 KB, patch)
2011-02-04 17:40 PST, Alexey Marinichev
no flags Details | Formatted Diff | Diff
Patch (9.37 KB, patch)
2011-02-04 18:18 PST, Alexey Marinichev
no flags Details | Formatted Diff | Diff
added virtual destructors (10.47 KB, patch)
2011-02-05 14:26 PST, Alexey Marinichev
no flags Details | Formatted Diff | Diff
reran prepare-ChangeLog yet again (10.52 KB, patch)
2011-02-07 09:41 PST, Alexey Marinichev
no flags Details | Formatted Diff | Diff
rebased, cleaned up changelog (10.04 KB, patch)
2011-02-07 10:32 PST, Alexey Marinichev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Marinichev 2011-02-03 15:10:54 PST
This adds necessary plumbing to forward the context lost callback from command buffer to WebGraphicsContext3D.
Comment 1 Alexey Marinichev 2011-02-03 15:51:58 PST
Created attachment 81129 [details]
Patch
Comment 2 Alexey Marinichev 2011-02-03 18:15:18 PST
Created attachment 81163 [details]
Patch
Comment 3 Kenneth Russell 2011-02-04 13:20:26 PST
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?
Comment 4 Alexey Marinichev 2011-02-04 15:08:25 PST
Created attachment 81300 [details]
Patch
Comment 5 Alexey Marinichev 2011-02-04 15:39:26 PST
> > 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!
Comment 6 Alexey Marinichev 2011-02-04 17:40:57 PST
Created attachment 81327 [details]
Patch
Comment 7 Alexey Marinichev 2011-02-04 17:45:31 PST
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.
Comment 8 Alexey Marinichev 2011-02-04 18:18:18 PST
Created attachment 81333 [details]
Patch
Comment 9 Alexey Marinichev 2011-02-04 18:18:43 PST
Removed webkit.pyc-2.4 that shouldn't be there.
Comment 10 Kenneth Russell 2011-02-04 18:32:46 PST
Comment on attachment 81333 [details]
Patch

Looks good to me.
Comment 11 WebKit Commit Bot 2011-02-04 19:14:03 PST
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
Comment 12 Alexey Marinichev 2011-02-05 14:26:21 PST
Created attachment 81374 [details]
added virtual destructors
Comment 13 Kenneth Russell 2011-02-05 21:17:14 PST
Comment on attachment 81374 [details]
added virtual destructors

Let's try again.
Comment 14 WebKit Commit Bot 2011-02-05 22:03:04 PST
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
Comment 15 Kenneth Russell 2011-02-06 10:34:18 PST
Looks like the failure was caused by the extra section in Source/WebKit/chromium/ChangeLog.
Comment 16 Alexey Marinichev 2011-02-07 09:41:25 PST
Created attachment 81483 [details]
reran prepare-ChangeLog yet again
Comment 17 Alexey Marinichev 2011-02-07 10:32:52 PST
Created attachment 81489 [details]
rebased, cleaned up changelog
Comment 18 Kenneth Russell 2011-02-07 10:49:00 PST
Comment on attachment 81489 [details]
rebased, cleaned up changelog

Looks fine to me; hopefully third time's the charm.
Comment 19 WebKit Commit Bot 2011-02-07 22:25:31 PST
Comment on attachment 81489 [details]
rebased, cleaned up changelog

Clearing flags on attachment: 81489

Committed r77900: <http://trac.webkit.org/changeset/77900>
Comment 20 WebKit Commit Bot 2011-02-07 22:25:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 WebKit Review Bot 2011-02-07 22:54:19 PST
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