Bug 47848 - [chromium] Check getGraphicsResetStatusARB and reinitialize the renderer in an error is returned.
Summary: [chromium] Check getGraphicsResetStatusARB and reinitialize the renderer in a...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2010-10-18 14:18 PDT by Alexey Marinichev
Modified: 2010-10-26 11:19 PDT (History)
4 users (show)

See Also:

Patch (5.73 KB, patch)
2010-10-18 14:23 PDT, Alexey Marinichev
no flags Details | Formatted Diff | Diff
addressed comments (5.24 KB, patch)
2010-10-20 18:07 PDT, 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-10-18 14:18:45 PDT
This is a step towards proper handling of lost context events.
Comment 1 Alexey Marinichev 2010-10-18 14:23:59 PDT
Created attachment 71082 [details]
Comment 2 Vangelis Kokkevis 2010-10-18 16:33:06 PDT
Comment on attachment 71082 [details]

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

> WebKit/chromium/src/WebViewImpl.cpp:2484
> +void WebViewImpl::reallocateRenderer(void)

I don't believe that (void) is common in WebKit for methods that take no arguments.

> WebKit/chromium/src/WebViewImpl.cpp:2487
> +    RefPtr<GraphicsContext3D> newContext = GraphicsContext3D::create(context->getContextAttributes(), m_page->chrome(), GraphicsContext3D::RenderDirectlyToHostWindow);

It's not guaranteed that either the newContext or the layerRenderer will be non-null. We need to check against failures.  There's some logic to keep track of failed creation attempts in WebViewImpl::setIsAcceleratedCompositingActive() that we'll need to use here as well.
If the context is lost, what are the chances we'll be able to re-create it right away?  If we fail to create a context should we give up after the first attempt?  We'll need to figure out what happens in a real context lost event on windows.

> WebKit/chromium/src/WebViewImpl.cpp:2494
> +    m_layerRenderer->rootLayer()->setNeedsDisplayRecursive();

I think that instead of calling setNeedsDisplayRecursive, you can modify LayerChromium::setLayerRenderer to call setNeedsDisplay() after calling cleanupResources(). That way, even layers that are currently not connected to layer hierarchy for some reason will be marked dirty before trying to render.
Comment 3 Alexey Marinichev 2010-10-20 18:07:16 PDT
Created attachment 71376 [details]
addressed comments
Comment 4 Kenneth Russell 2010-10-25 19:37:31 PDT
Comment on attachment 71376 [details]
addressed comments

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

This looks good. Couple of tiny comments, not necessary to address (marking cq+).

> WebCore/ChangeLog:6
> +        renderer in an error is returned.

in -> if

> WebKit/chromium/ChangeLog:6
> +        renderer in an error is returned.

in -> if

> WebKit/chromium/src/WebViewImpl.cpp:2489
> +    RefPtr<LayerRendererChromium> layerRenderer = LayerRendererChromium::create(newContext);

Strictly speaking this should probably be newContext.release().
Comment 5 WebKit Commit Bot 2010-10-26 11:18:58 PDT
Comment on attachment 71376 [details]
addressed comments

Clearing flags on attachment: 71376

Committed r70543: <http://trac.webkit.org/changeset/70543>
Comment 6 WebKit Commit Bot 2010-10-26 11:19:03 PDT
All reviewed patches have been landed.  Closing bug.