Bug 31169 - Need to implement WebGLContextAttributes
Summary: Need to implement WebGLContextAttributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
: 31176 (view as bug list)
Depends on:
Blocks: 33416
  Show dependency treegraph
 
Reported: 2009-11-05 10:30 PST by Chris Marrin
Modified: 2010-01-14 12:07 PST (History)
5 users (show)

See Also:


Attachments
Patch (56.94 KB, patch)
2010-01-08 19:54 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Revised patch (56.94 KB, patch)
2010-01-08 20:53 PST, Kenneth Russell
oliver: review-
Details | Formatted Diff | Diff
Revised patch (54.52 KB, patch)
2010-01-11 15:05 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Revised patch (54.52 KB, patch)
2010-01-11 18:47 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Revised patch (54.43 KB, patch)
2010-01-12 11:36 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2009-11-05 10:30:02 PST
This object allows characteristics of the drawing buffer to be passed to getContext
Comment 1 Kenneth Russell 2010-01-08 19:13:59 PST
*** Bug 31176 has been marked as a duplicate of this bug. ***
Comment 2 Kenneth Russell 2010-01-08 19:54:20 PST
Created attachment 46194 [details]
Patch

Added the WebGLContextAttributes class and custom JavaScript bindings to accept a native object as the second argument to getContext("experimental-webgl") per the WebGL specification. Passed new attributes object down through HTMLCanvasElement::getContext to WebGLRenderingContext and GraphicsContext3D. Added getContextAttributes() to WebGLRenderingContext. Added test case ensuring that context attributes can be passed down and returned.  Tested in Safari and Chromium. The attributes will be hooked up to the creation of the OpenGL context in bug 33416.
Comment 3 WebKit Review Bot 2010-01-08 20:00:27 PST
Attachment 46194 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/HTMLCanvasElement.h:72:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1
Comment 4 Kenneth Russell 2010-01-08 20:53:17 PST
Created attachment 46195 [details]
Revised patch

Revised patch fixing style error
Comment 5 Chris Marrin 2010-01-08 21:06:26 PST
Comment on attachment 46195 [details]
Revised patch

> Index: WebCore/platform/graphics/GraphicsContext3D.h
> ===================================================================
> --- WebCore/platform/graphics/GraphicsContext3D.h	(revision 53013)
> +++ WebCore/platform/graphics/GraphicsContext3D.h	(working copy)
> @@ -55,6 +55,7 @@ namespace WebCore {
>      class WebGLActiveInfo;
>      class WebGLArray;
>      class WebGLBuffer;
> +    class WebGLContextAttributes;
>      class WebGLUnsignedByteArray;
>      class WebGLFloatArray;
>      class WebGLFramebuffer;
> @@ -382,7 +383,7 @@ namespace WebCore {
>              INVALID_FRAMEBUFFER_OPERATION = 0x0506
>          };
>          
> -        static PassOwnPtr<GraphicsContext3D> create();
> +        static PassOwnPtr<GraphicsContext3D> create(WebGLContextAttributes* attrs);
>          virtual ~GraphicsContext3D();
>  
>  #if PLATFORM(MAC)
> @@ -459,6 +460,12 @@ namespace WebCore {
>  
>          void getBufferParameteriv(unsigned long target, unsigned long pname, int* value);
>  
> +        void getContextAttributes(bool& alpha,
> +                                  bool& depth,
> +                                  bool& stencil,
> +                                  bool& antialias,
> +                                  bool& premultipliedAlpha);
> +
>          unsigned long getError();
>  
>          void getFloatv(unsigned long pname, float* value);
> @@ -602,11 +609,16 @@ namespace WebCore {
>          void synthesizeGLError(unsigned long error);
>  
>      private:        
> -        GraphicsContext3D();
> +        GraphicsContext3D(WebGLContextAttributes* attrs);
>  
>          int m_currentWidth, m_currentHeight;
>          
>  #if PLATFORM(MAC)
> +        bool m_alphaAttr;
> +        bool m_depthAttr;
> +        bool m_stencilAttr;
> +        bool m_antialiasAttr;
> +        bool m_premultipliedAlphaAttr;
>          Vector<Vector<float> > m_vertexArray;

It might be better to put these into a little helper class to make it easier to pass them in and out. It would make it easier to expand in the future as well.In fact you could use an instance of this class to hold the data in the WebGLContextAttributes object to avoid a lot of copying.
Comment 6 WebKit Review Bot 2010-01-09 08:42:28 PST
Attachment 46195 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/176398
Comment 7 Kenneth Russell 2010-01-09 14:04:45 PST
(In reply to comment #5)
> It might be better to put these into a little helper class to make it easier to
> pass them in and out. It would make it easier to expand in the future as
> well.In fact you could use an instance of this class to hold the data in the
> WebGLContextAttributes object to avoid a lot of copying.

I agree, but do you have a suggestion on where that class should go? It seems that putting it in WebCore/html/canvas is not allowed, because headers in WebCore/platform/graphics are not supposed to refer upward. I suppose it could be a nested class of GraphicsContext3D, such as GraphicsContext3D::Attributes.
Comment 8 Oliver Hunt 2010-01-09 15:55:43 PST
Comment on attachment 46195 [details]
Revised patch

I think a problem here is that you're making GraphicsContext3D depend on the DOM type WebGLContextAttributes -- it should not be.  The dependence on DOM types in GraphicsContext3D is a serious design bug that we need to fix, adding yyet more dependencies is a bad thing.

I would suggest that these arguments simply be passed as either bool or enum parameters to GraphicsContext3D constructor.  I'd also like to get shared storage and implementation of these attributes, rather than having platform ifdefs scattered throughout the code.  We spent a lot of time rectifying this in the 2d context, doing it in the 3d context seems bad.
Comment 9 Chris Marrin 2010-01-10 08:03:42 PST
Put the class in GraphicsContext3D.h and use it in WebGLContextAttributes. Then you can pass the class to GraphicsContext3D APIs and avoid any WebGLContextAttributes dependencies.
Comment 10 Kenneth Russell 2010-01-11 15:05:36 PST
Created attachment 46313 [details]
Revised patch

Added GraphicsContext3D::Attributes struct as the bridge between the DOM and graphics layers to avoid abstraction violations. Revised ChangeLog descriptions. Also fixed up build failures on qt and gtk bots from last patch.
Comment 11 WebKit Review Bot 2010-01-11 15:12:46 PST
Attachment 46313 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/HTMLCanvasElement.h:72:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1
Comment 12 Chris Marrin 2010-01-11 15:38:33 PST
It might be better to use a class for GraphicsContext3D so it can have proper initializers and you don't have to worry about the members having a rational value.
Comment 13 Kenneth Russell 2010-01-11 18:46:06 PST
(In reply to comment #12)
> It might be better to use a class for GraphicsContext3D so it can have proper
> initializers and you don't have to worry about the members having a rational
> value.

I considered that, but that will duplicate the setter/getter code in WebGLContextAttributes. It's simpler to leave it as a struct since at the top level these values come in via WebGLContextAttributes.

I'll upload a new patch fixing the style issue.
Comment 14 Kenneth Russell 2010-01-11 18:47:23 PST
Created attachment 46330 [details]
Revised patch

Fixed style issue in previous patch.
Comment 15 Chris Marrin 2010-01-12 10:46:52 PST
(In reply to comment #13)
> (In reply to comment #12)
> > It might be better to use a class for GraphicsContext3D so it can have proper
> > initializers and you don't have to worry about the members having a rational
> > value.
> 
> I considered that, but that will duplicate the setter/getter code in
> WebGLContextAttributes. It's simpler to leave it as a struct since at the top
> level these values come in via WebGLContextAttributes.

I'm not sure what you mean. You can keep the member variables public and just add a ctor which will init them to the correct default. In fact, you can just leave it as a struct and add a ctor, since the only difference between the struct and class keywords is that everything in a struct is implicitly public.

I just like to avoid the possibility of uninitialized variables when we have the mechanisms to avoid it, mostly for free.
Comment 16 Kenneth Russell 2010-01-12 10:51:56 PST
(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > It might be better to use a class for GraphicsContext3D so it can have proper
> > > initializers and you don't have to worry about the members having a rational
> > > value.
> > 
> > I considered that, but that will duplicate the setter/getter code in
> > WebGLContextAttributes. It's simpler to leave it as a struct since at the top
> > level these values come in via WebGLContextAttributes.
> 
> I'm not sure what you mean. You can keep the member variables public and just
> add a ctor which will init them to the correct default. In fact, you can just
> leave it as a struct and add a ctor, since the only difference between the
> struct and class keywords is that everything in a struct is implicitly public.
> 
> I just like to avoid the possibility of uninitialized variables when we have
> the mechanisms to avoid it, mostly for free.

I assumed it would be bad form to add a constructor to a struct. Since it isn't I'll make this change.
Comment 17 Kenneth Russell 2010-01-12 11:36:05 PST
Created attachment 46385 [details]
Revised patch

Moved initialization of attribute flags into constructor on GraphicsContext3D::Attributes struct.
Comment 18 Oliver Hunt 2010-01-13 11:24:49 PST
Comment on attachment 46385 [details]
Revised patch

r=me
Comment 19 WebKit Commit Bot 2010-01-13 21:50:56 PST
Comment on attachment 46385 [details]
Revised patch

Clearing flags on attachment: 46385

Committed r53238: <http://trac.webkit.org/changeset/53238>
Comment 20 WebKit Commit Bot 2010-01-13 21:51:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Eric Seidel (no email) 2010-01-14 00:01:57 PST
This checkin broke Tiger.  Sadly the commit-queue can't detect that yet.
http://build.webkit.org/builders/Tiger%20Intel%20Release/builds/7737/steps/compile-webkit/logs/stdio

I'll fix it.  It should be a very simple fix.
Comment 22 Eric Seidel (no email) 2010-01-14 00:04:28 PST
(This, btw, is no negative reflection on you kbr.  Simply logging everything in the bug for posterity.  The commit-queue should be bullet-proof and know how to clean up after itself when it break things.  I'm working on teaching it how to roll out its own commits when it can be sure they broke things.)
Comment 23 Eric Seidel (no email) 2010-01-14 00:21:50 PST
Committed r53243: <http://trac.webkit.org/changeset/53243>
Comment 24 Kenneth Russell 2010-01-14 12:02:47 PST
Sorry about that. I'm surprised the qt/gtk bots didn't catch that mistake since they caught other areas where I hadn't built with ENABLE(3D_CANVAS) turned off.
Comment 25 Eric Seidel (no email) 2010-01-14 12:07:09 PST
The EWS bots don't build debug yet.  They should.  We'll make them do so as soon as they're fast enough. :)  I've filed bug 33681 about this.