Bug 50364 - Throw webglcontextlost and webglcontextrestored events when a WebGL context is lost and restored.
Summary: Throw webglcontextlost and webglcontextrestored events when a WebGL context i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-01 18:06 PST by Alexey Marinichev
Modified: 2010-12-06 22:07 PST (History)
5 users (show)

See Also:


Attachments
Patch (5.72 KB, patch)
2010-12-01 18:15 PST, Alexey Marinichev
no flags Details | Formatted Diff | Diff
Patch (7.76 KB, patch)
2010-12-03 15:23 PST, Alexey Marinichev
no flags Details | Formatted Diff | Diff
Patch (7.46 KB, patch)
2010-12-06 15:35 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 2010-12-01 18:06:31 PST
This adds a call to getGraphicsResetStatusARB to see if the context was lost.  If it was it will attempt to create a new one.  A javascript event is thrown in both cases.
Comment 1 Alexey Marinichev 2010-12-01 18:15:05 PST
Created attachment 75342 [details]
Patch
Comment 2 Kenneth Russell 2010-12-02 11:27:54 PST
Comment on attachment 75342 [details]
Patch

As we've discussed offline you should start a timer when the context is lost -- but only if the user has registered a webglcontextrestored callback on the canvas. The timer would poll periodically to see whether it can successfully recreate the context, and once it does, send the webglcontextrestored event. You can find examples of setting up periodic callbacks in the implementation of DOMWindow::setInterval (WebCore/page/DOMWindow.cpp). The current implementation relies on the app making WebGL calls against the lost context in order to discover that the context has been restored, which isn't supposed to be the programming model according to the WebGL spec.
Comment 3 Alexey Marinichev 2010-12-03 15:23:33 PST
Created attachment 75555 [details]
Patch
Comment 4 Kenneth Russell 2010-12-06 13:02:24 PST
Comment on attachment 75555 [details]
Patch

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

Looks good overall. A few minor issues.

> WebCore/html/canvas/WebGLRenderingContext.cpp:97
> +        startOneShot(1);

Instead of 1, use a const double with a descriptive name like secondsBetweenRestoreAttempts.

> WebCore/html/canvas/WebGLRenderingContext.cpp:106
> +            startOneShot(1);

Const double instead of 1.

> WebCore/html/canvas/WebGLRenderingContext.cpp:2125
> +        m_restoreTimer.startOneShot(1);

Const double instead of 1.

> WebCore/html/canvas/WebGLRenderingContext.h:67
> +class WebGLRenderingContextRestoreTimer : public TimerBase {
> +public:
> +    WebGLRenderingContextRestoreTimer(WebGLRenderingContext* context) : m_context(context) { }
> +private:
> +    virtual void fired();
> +    WebGLRenderingContext* m_context;
> +};

In WebKit code each class generally goes into its own header file. In this case I think you ought to be able to forward declare this class as long as you change the data member to a RefPtr below.

> WebCore/html/canvas/WebGLRenderingContext.h:357
> +    WebGLRenderingContextRestoreTimer m_restoreTimer;

Please try making this a RefPtr<WebGLRenderingContextRestoreTimer> and see whether that allows the declaration and definition of WebGLRenderingContextRestoreTimer to move to the .cpp file.
Comment 5 Alexey Marinichev 2010-12-06 15:35:23 PST
Created attachment 75741 [details]
Patch
Comment 6 Alexey Marinichev 2010-12-06 15:36:48 PST
Introduced secondsBetweenRestoreAttempts as requested.

Also, rather than adding another header file, I moved WebGLRenderingContextRestoreTimer inside WebGLRenderingContext.  friend statement is no longer needed.
Comment 7 Kenneth Russell 2010-12-06 16:23:49 PST
Comment on attachment 75741 [details]
Patch

Looks good.
Comment 8 WebKit Commit Bot 2010-12-06 17:35:32 PST
Comment on attachment 75741 [details]
Patch

Rejecting patch 75741 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 1
ERROR: Working directory has local commits, pass --force-clean to continue.

Full output: http://queues.webkit.org/results/6786072
Comment 9 Kenneth Russell 2010-12-06 17:46:38 PST
Comment on attachment 75741 [details]
Patch

Let's try again.
Comment 10 WebKit Commit Bot 2010-12-06 20:24:35 PST
Comment on attachment 75741 [details]
Patch

Clearing flags on attachment: 75741

Committed r73424: <http://trac.webkit.org/changeset/73424>
Comment 11 WebKit Commit Bot 2010-12-06 20:24:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Commit Bot 2010-12-06 22:07:40 PST
The commit-queue encountered the following flaky tests while processing attachment 75741 [details]:

fast/media/mq-aspect-ratio.html
fast/dom/onerror-img.html

Please file bugs against the tests.  These tests were authored by dino@apple.com.  The commit-queue is continuing to process your patch.