Bug 66788 - [chromium] Make WebGL context current before querying for extensions
Summary: [chromium] Make WebGL context current before querying for extensions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-23 11:04 PDT by Iain Merrick
Modified: 2011-08-23 19:11 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.09 KB, patch)
2011-08-23 11:06 PDT, Iain Merrick
jamesr: review-
Details | Formatted Diff | Diff
Added asserts to catch (some) incorrect usage (8.50 KB, patch)
2011-08-23 13:58 PDT, Iain Merrick
no flags Details | Formatted Diff | Diff
Just call makeContextCurrent() implicitly when it's needed (1.68 KB, patch)
2011-08-23 18:03 PDT, Iain Merrick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Iain Merrick 2011-08-23 11:04:05 PDT
Make WebGL context current before querying for extensions.
Comment 1 Iain Merrick 2011-08-23 11:06:26 PDT
Created attachment 104867 [details]
Patch
Comment 2 James Robinson 2011-08-23 11:19:29 PDT
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() ?
Comment 3 James Robinson 2011-08-23 11:22:24 PDT
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?
Comment 4 Iain Merrick 2011-08-23 13:58:02 PDT
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?
Comment 5 James Robinson 2011-08-23 14:37:00 PDT
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?
Comment 6 Nat Duca 2011-08-23 14:54:42 PDT
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.
Comment 7 James Robinson 2011-08-23 15:00:18 PDT
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.
Comment 8 Nat Duca 2011-08-23 15:05:13 PDT
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!"
Comment 9 Kenneth Russell 2011-08-23 17:13:03 PDT
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.
Comment 10 Iain Merrick 2011-08-23 17:37:10 PDT
(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.
Comment 11 Kenneth Russell 2011-08-23 17:42:28 PDT
(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.
Comment 12 Iain Merrick 2011-08-23 18:03:41 PDT
Created attachment 104949 [details]
Just call makeContextCurrent() implicitly when it's needed
Comment 13 Kenneth Russell 2011-08-23 18:07:34 PDT
Comment on attachment 104949 [details]
Just call makeContextCurrent() implicitly when it's needed

Thanks. Looks good to me.
Comment 14 WebKit Review Bot 2011-08-23 19:11:24 PDT
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>
Comment 15 WebKit Review Bot 2011-08-23 19:11:29 PDT
All reviewed patches have been landed.  Closing bug.