Bug 47848

Summary: [chromium] Check getGraphicsResetStatusARB and reinitialize the renderer in an error is returned.
Product: WebKit Reporter: Alexey Marinichev <amarinichev>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Severity: Normal CC: commit-queue, enne, kbr, vangelis
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Description Flags
addressed comments none

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.