Bug 47848 – [chromium] Check getGraphicsResetStatusARB and reinitialize the renderer in an error is returned.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Alexey Marinichev]

This is a step towards proper handling of lost context events.

[Alexey Marinichev]

Created attachment 71082 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 71082 [details]
Patch

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.

[Alexey Marinichev]

Created attachment 71376 [details]
addressed comments

[Kenneth Russell]

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().

[WebKit Commit Bot]

Comment on attachment 71376 [details]
addressed comments

Clearing flags on attachment: 71376

Committed r70543: <http://trac.webkit.org/changeset/70543>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.