Bug 66788 – [chromium] Make WebGL context current before querying for extensions

• Component: New Bugs
• Status: RESOLVED FIXED

[Iain Merrick]

Make WebGL context current before querying for extensions.

[Iain Merrick]

Created attachment 104867 [details]
Patch

[James Robinson]

Comment on attachment 104867 [details]
Patch

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

This patch is rooted at the wrong dir - it should be rooted at the root of the repo, so the paths show up as Source/WebCore/... - it looks like this is rooted inside the Source/ directory.  This means that the show more context links don't work in the review tool.

> WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

You can't commit with this message, there's an svn presubmit hook that will reject it.  Please explain how to test this (I think you can exercise this by flipping some local define, right?)

> WebCore/html/canvas/WebGLRenderingContext.cpp:387
> +    m_context->makeContextCurrent();

it would make more sense to me to do this in setupFlags(), closer to where we actually use the context.

Do we need to do the same thing in WebGLRenderingContext::maybeRestoreContext() ?

[James Robinson]

It looks like we don't call makeContextCurrent at all in WebGLRenderingContext.cpp.  What are the rules supposed to be for GraphicsContext3D::makeContextCurrent() in cross-platform code?

[Iain Merrick]

Created attachment 104910 [details]
Added asserts to catch (some) incorrect usage

I needed GraphicsContext3DInternal to handle onContextLost for this CL, which it was not currently doing. I've done this by inheriting it from WebGraphicsContextLostCallback, which means we #include WebGraphicsContext3D rather than forward-declaring it; is that okay, or does it need to be hidden inside an adapter object?

[James Robinson]

Comment on attachment 104910 [details]
Added asserts to catch (some) incorrect usage

Why did all the callback stuff change?

This feels a bit odd, I think it's checking at the wrong layer. Why is there only an ASSERT() in initializeExtensions?

[Nat Duca]

I'm not sure if this helps, but would it be easier to go to lazy-discovery of resource safety like I did in the GraphicsContext3D case? It would sidestep this discussion and vageueness about currentness.

[James Robinson]

Alternately we could handle this under the hood (by effectively calling makeContextCurrent() in the implementation) and say that's the way GraphicsContext3D works until someone has time to go in and put in proper assertions that verify the context really is current.

[Nat Duca]

Both are valid options. I'd lean toward shadowing that way it leads to less complications down the road if someone decides they want webgl on a secondary context. Could even add some comments to the constructor of WebGraphicsContext saying "dont put stuff here, yo!"

[Kenneth Russell]

Comment on attachment 104910 [details]
Added asserts to catch (some) incorrect usage

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

After our offline discussion I'm strongly inclined to push the necessary makeContextCurrent() calls into the implementation, which should reduce this patch to a one-liner. What do you think?

> Source/WebKit/chromium/src/GraphicsContext3DInternal.h:338
> +    bool m_makeContextCurrentWasCalled;

As the comment indicates, this bool is fragile and doesn't handle multiple contexts per thread. Per our offline discussion, it sounds like the better direction for GraphicsContext3D is to make it easier to use, i.e., assume that makeContextCurrent() is called implicitly for each of its other entry points. That will also simplify this patch.

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:801
> +    ASSERT(m_makeContextCurrentWasCalled);

Let's just call makeContextCurrent() here.

[Iain Merrick]

(In reply to comment #9)
> (From update of attachment 104910 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104910&action=review
> 
> After our offline discussion I'm strongly inclined to push the necessary makeContextCurrent() calls into the implementation, which should reduce this patch to a one-liner. What do you think?
> 
> > Source/WebKit/chromium/src/GraphicsContext3DInternal.h:338
> > +    bool m_makeContextCurrentWasCalled;
> 
> As the comment indicates, this bool is fragile and doesn't handle multiple contexts per thread. Per our offline discussion, it sounds like the better direction for GraphicsContext3D is to make it easier to use, i.e., assume that makeContextCurrent() is called implicitly for each of its other entry points. That will also simplify this patch.
> 
> > Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:801
> > +    ASSERT(m_makeContextCurrentWasCalled);
> 
> Let's just call makeContextCurrent() here.

And ignore the return value, right? That's what GraphicsContext3DOpenGL does.

[Kenneth Russell]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 104910 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=104910&action=review
> > 
> > After our offline discussion I'm strongly inclined to push the necessary makeContextCurrent() calls into the implementation, which should reduce this patch to a one-liner. What do you think?
> > 
> > > Source/WebKit/chromium/src/GraphicsContext3DInternal.h:338
> > > +    bool m_makeContextCurrentWasCalled;
> > 
> > As the comment indicates, this bool is fragile and doesn't handle multiple contexts per thread. Per our offline discussion, it sounds like the better direction for GraphicsContext3D is to make it easier to use, i.e., assume that makeContextCurrent() is called implicitly for each of its other entry points. That will also simplify this patch.
> > 
> > > Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:801
> > > +    ASSERT(m_makeContextCurrentWasCalled);
> > 
> > Let's just call makeContextCurrent() here.
> 
> And ignore the return value, right? That's what GraphicsContext3DOpenGL does.

Yeah, I guess so. Or assert that it's true.

[Iain Merrick]

Created attachment 104949 [details]
Just call makeContextCurrent() implicitly when it's needed

[Kenneth Russell]

Comment on attachment 104949 [details]
Just call makeContextCurrent() implicitly when it's needed

Thanks. Looks good to me.

[WebKit Review Bot]

Comment on attachment 104949 [details]
Just call makeContextCurrent() implicitly when it's needed

Clearing flags on attachment: 104949

Committed r93689: <http://trac.webkit.org/changeset/93689>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.