Bug 30091 – Change get... calls to latest spec

• Component: WebGL
• Status: RESOLVED FIXED

[Chris Marrin]

We've changed the get API to return 'any' in many cases to reduce the number of call.

[Kenneth Russell]

Created attachment 43541 [details]
Proposed patch

Removed old versions of get calls on WebGLRenderingContext and added new ones per spec returning "any". New code simplifies GraphicsContext3D and fixes previously unimplemented routines. Added custom JS and V8 bindings. Added exhaustive test case exercising all new code paths. Updated preexisting test cases for new APIs. Fixed preexisting bugs in WebKit's and Chrome's WebGL implementations. Cleaned up a couple of areas from recent Canvas -> WebGL renaming and deleted obsolete CanvasNumberArray.

Ran WebGL layout tests in WebKit (clean) and Chrome (couple of preexisting known failures) and manual WebGL tests in both browsers.

[Oliver Hunt]

Ken these huge patches are really hard to review, in general we only accept patches this large from people who have been involved in webkit for considerable periods (like hyatt for instance) -- could you please split this up into separate patches.

Renames should be limited as much as possible to just the rename and no other changes, they should also be done in terms of the do-webcore-rename script.

The additional mixing of tidying and the addition of new code is equally irksome, but while the renames are present it's difficult to determine how much of the refactoring is noise due to the rename vs. actual changes so the sooner we split those remaining renames out the better.  The renames should probably go into a separate bug, that blocks this one.

If you want I can do the appropriate rename-fu with do-webcore-rename

[Kenneth Russell]

There aren't any renames in this patch. Three now-bogus files were deleted -- CanvasNumberArray.cpp, CanvasNumberArray.h, and CanvasNumberArray.idl. The only other cleanups in this patch were moving around of some #includes and files in the WebCore.gypi / project.pbxproj.

The rest of the changes are what were needed to implement the new semantics of the get methods. Yes, the patch is large, but it is precisely what is needed and has been very well tested. It will be very painful for me to split it up into multiple patches since in the intermediate stages some of the get methods will have the correct (new) semantics and some of them will be incorrect. This is the last large patch that will be needed to bring the WebKit WebGL implementation in line with the current spec. Please review it as is.

I will be out of the office tomorrow, Friday, November 20.

[Dimitri Glazkov (Google)]

This does look like a chunky patch.

But in all fairness, most of it does seem fairly mechanical, aside from minor prettification (like CleanupHelper). And since Ken and Chris are collaborating on this code, perhaps maybe Chris could take a look and weigh in on the patch?

I am all for small, digestible patches, but these guys did write this code, so I am willing to consider them the "hyatts" of WebGL in WebKit :)

[Oliver Hunt]

(In reply to comment #3)
> There aren't any renames in this patch. Three now-bogus files were deleted --
> CanvasNumberArray.cpp, CanvasNumberArray.h, and CanvasNumberArray.idl. The only
> other cleanups in this patch were moving around of some #includes and files in
> the WebCore.gypi / project.pbxproj.

Why does the removal of CanvasNumberArray* have to go in this patch?  There are no dependencies that I can see -- as far as i can tell they could be removed before, or they could be removed after this patch lands.

There is absolutely no reason for it to be part of this patch.

> The rest of the changes are what were needed to implement the new semantics of
> the get methods. Yes, the patch is large, but it is precisely what is needed
> and has been very well tested. It will be very painful for me to split it up
> into multiple patches since in the intermediate stages some of the get methods
> will have the correct (new) semantics and some of them will be incorrect. This
> is the last large patch that will be needed to bring the WebKit WebGL
> implementation in line with the current spec. Please review it as is.
No.  At the very least you should drop the removal of CanvasNumberArray files

I'm sorry if this seems harsh, but you need to understand that large patches are _not_ good.  You should always favour incremental patches over large do-everything-at-once style patches.

[Chris Marrin]

Comment on attachment 43541 [details]
Proposed patch

I leave the final review to Ollie, but here is my take:

> Index: WebCore/WebCore.gypi
> ===================================================================
> --- WebCore/WebCore.gypi	(revision 51226)
> +++ WebCore/WebCore.gypi	(working copy)
> @@ -1271,6 +1271,20 @@
>              'history/HistoryItem.h',
>              'history/PageCache.cpp',
>              'history/PageCache.h',
> +            'html/canvas/CanvasGradient.cpp',
> +            'html/canvas/CanvasGradient.h',
> +            'html/canvas/CanvasObject.cpp',
> +            'html/canvas/CanvasObject.h',
> +            'html/canvas/CanvasPattern.cpp',
> +            'html/canvas/CanvasPattern.h',
> +            'html/canvas/CanvasPixelArray.cpp',
> +            'html/canvas/CanvasPixelArray.h',
> +            'html/canvas/CanvasRenderingContext.cpp',
> +            'html/canvas/CanvasRenderingContext.h',
> +            'html/canvas/CanvasRenderingContext2D.cpp',
> +            'html/canvas/CanvasRenderingContext2D.h',
> +            'html/canvas/CanvasStyle.cpp',
> +            'html/canvas/CanvasStyle.h',
>              'html/canvas/WebGLArray.cpp',
>              'html/canvas/WebGLArray.h',
>              'html/canvas/WebGLArrayBuffer.cpp',
> @@ -1283,32 +1297,20 @@
>              'html/canvas/WebGLFloatArray.h',
>              'html/canvas/WebGLFramebuffer.cpp',
>              'html/canvas/WebGLFramebuffer.h',
> -            'html/canvas/CanvasGradient.cpp',
> -            'html/canvas/CanvasGradient.h',
> +            'html/canvas/WebGLGetInfo.cpp',
> +            'html/canvas/WebGLGetInfo.h',
>              'html/canvas/WebGLIntArray.cpp',
>              'html/canvas/WebGLIntArray.h',
> -            'html/canvas/CanvasObject.cpp',
> -            'html/canvas/CanvasObject.h',
> -            'html/canvas/CanvasPattern.cpp',
> -            'html/canvas/CanvasPattern.h',
> -            'html/canvas/CanvasPixelArray.cpp',
> -            'html/canvas/CanvasPixelArray.h',
>              'html/canvas/WebGLProgram.cpp',
>              'html/canvas/WebGLProgram.h',
>              'html/canvas/WebGLRenderbuffer.cpp',
>              'html/canvas/WebGLRenderbuffer.h',
> -            'html/canvas/CanvasRenderingContext.cpp',
> -            'html/canvas/CanvasRenderingContext.h',
> -            'html/canvas/CanvasRenderingContext2D.cpp',
> -            'html/canvas/CanvasRenderingContext2D.h',
>              'html/canvas/WebGLRenderingContext.cpp',
>              'html/canvas/WebGLRenderingContext.h',
>              'html/canvas/WebGLShader.cpp',
>              'html/canvas/WebGLShader.h',
>              'html/canvas/WebGLShortArray.cpp',
>              'html/canvas/WebGLShortArray.h',
> -            'html/canvas/CanvasStyle.cpp',
> -            'html/canvas/CanvasStyle.h',
>              'html/canvas/WebGLTexture.cpp',
>              'html/canvas/WebGLTexture.h',
>              'html/canvas/WebGLUnsignedByteArray.cpp',

There are many files being added or removed from this file that seem to have nothing to do with the patch. I think the only thing that should be in this file is the addition of WebGLGetInfo


> Index: WebCore/WebCore.xcodeproj/project.pbxproj
> ===================================================================
> --- WebCore/WebCore.xcodeproj/project.pbxproj	(revision 51226)
> +++ WebCore/WebCore.xcodeproj/project.pbxproj	(working copy)
> @@ -1199,6 +1199,8 @@
>  		65DF323C09D1DE65000BE325 /* JSCanvasPattern.h in Headers */ = {isa = PBXBuildFile; fileRef = 65DF323609D1DE65000BE325 /* JSCanvasPattern.h */; };
>  		65DF326109D1E199000BE325 /* UserAgentStyleSheetsData.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 656581AF09D14EE6000E61D7 /* UserAgentStyleSheetsData.cpp */; };
>  		65FEA86909833ADE00BED4AB /* Page.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 65FEA86809833ADE00BED4AB /* Page.cpp */; };
> +		6E51D61C10B23CC200FDE74A /* WebGLGetInfo.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6E51D61A10B23CC200FDE74A /* WebGLGetInfo.cpp */; };
> +		6E51D61D10B23CC200FDE74A /* WebGLGetInfo.h in Headers */ = {isa = PBXBuildFile; fileRef = 6E51D61B10B23CC200FDE74A /* WebGLGetInfo.h */; };
>  		72626E020EF022FE00A07E20 /* FontFastPath.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 72626E010EF022FE00A07E20 /* FontFastPath.cpp */; };
>  		754133A8102E00E800075D00 /* InspectorTimelineAgent.h in Headers */ = {isa = PBXBuildFile; fileRef = 754133A7102E00E800075D00 /* InspectorTimelineAgent.h */; };
>  		754133AA102E00F400075D00 /* InspectorTimelineAgent.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 754133A9102E00F400075D00 /* InspectorTimelineAgent.cpp */; };
> @@ -6507,6 +6509,8 @@
>  		65DF323609D1DE65000BE325 /* JSCanvasPattern.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; path = JSCanvasPattern.h; sourceTree = "<group>"; };
>  		65F80697054D9F86008BF776 /* BlockExceptions.mm */ = {isa = PBXFileReference; fileEncoding = 30; indentWidth = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = BlockExceptions.mm; sourceTree = "<group>"; tabWidth = 8; usesTabs = 0; };
>  		65FEA86809833ADE00BED4AB /* Page.cpp */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.cpp; path = Page.cpp; sourceTree = "<group>"; };
> +		6E51D61A10B23CC200FDE74A /* WebGLGetInfo.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = WebGLGetInfo.cpp; path = canvas/WebGLGetInfo.cpp; sourceTree = "<group>"; };
> +		6E51D61B10B23CC200FDE74A /* WebGLGetInfo.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = WebGLGetInfo.h; path = canvas/WebGLGetInfo.h; sourceTree = "<group>"; };
>  		72626E010EF022FE00A07E20 /* FontFastPath.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = FontFastPath.cpp; sourceTree = "<group>"; };
>  		754133A7102E00E800075D00 /* InspectorTimelineAgent.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = InspectorTimelineAgent.h; sourceTree = "<group>"; };
>  		754133A9102E00F400075D00 /* InspectorTimelineAgent.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = InspectorTimelineAgent.cpp; sourceTree = "<group>"; };
> @@ -10620,6 +10624,25 @@
>  		49484FAE102CF01E00187DD3 /* canvas */ = {
>  			isa = PBXGroup;
>  			children = (
> +				49484FB3102CF23C00187DD3 /* CanvasGradient.cpp */,
> +				49484FB4102CF23C00187DD3 /* CanvasGradient.h */,
> +				49484FB5102CF23C00187DD3 /* CanvasGradient.idl */,
> +				49C7B9B41042D32F0009D447 /* CanvasObject.cpp */,
> +				49C7B9B51042D32F0009D447 /* CanvasObject.h */,
> +				49484FB6102CF23C00187DD3 /* CanvasPattern.cpp */,
> +				49484FB7102CF23C00187DD3 /* CanvasPattern.h */,
> +				49484FB8102CF23C00187DD3 /* CanvasPattern.idl */,
> +				49484FB9102CF23C00187DD3 /* CanvasPixelArray.cpp */,
> +				49484FBA102CF23C00187DD3 /* CanvasPixelArray.h */,
> +				49484FBB102CF23C00187DD3 /* CanvasPixelArray.idl */,
> +				49C7B9BC1042D32F0009D447 /* CanvasRenderingContext.cpp */,
> +				49C7B9BD1042D32F0009D447 /* CanvasRenderingContext.h */,
> +				49C7B9BE1042D32F0009D447 /* CanvasRenderingContext.idl */,
> +				49484FBC102CF23C00187DD3 /* CanvasRenderingContext2D.cpp */,
> +				49484FBD102CF23C00187DD3 /* CanvasRenderingContext2D.h */,
> +				49484FBE102CF23C00187DD3 /* CanvasRenderingContext2D.idl */,
> +				49484FBF102CF23C00187DD3 /* CanvasStyle.cpp */,
> +				49484FC0102CF23C00187DD3 /* CanvasStyle.h */,

Same here. Why all these added files?


> Index: WebCore/bindings/js/JSCanvasNumberArrayCustom.cpp
> ===================================================================
> --- WebCore/bindings/js/JSCanvasNumberArrayCustom.cpp	(revision 51226)
> +++ WebCore/bindings/js/JSCanvasNumberArrayCustom.cpp	(working copy)
...
> Index: WebCore/html/canvas/CanvasNumberArray.cpp
> ===================================================================
> --- WebCore/html/canvas/CanvasNumberArray.cpp	(revision 51226)
> +++ WebCore/html/canvas/CanvasNumberArray.cpp	(working copy)
...
> -#endif // ENABLE(3D_CANVAS)
> Index: WebCore/html/canvas/CanvasNumberArray.h
> ===================================================================
> --- WebCore/html/canvas/CanvasNumberArray.h	(revision 51226)
> +++ WebCore/html/canvas/CanvasNumberArray.h	(working copy)
...
> Index: WebCore/html/canvas/CanvasNumberArray.idl
> ===================================================================
> --- WebCore/html/canvas/CanvasNumberArray.idl	(revision 51226)
> +++ WebCore/html/canvas/CanvasNumberArray.idl	(working copy)

These files are unrelated to this patch


> Index: WebCore/html/canvas/CanvasObject.cpp
> ===================================================================
> --- WebCore/html/canvas/CanvasObject.cpp	(revision 51226)
> +++ WebCore/html/canvas/CanvasObject.cpp	(working copy)
> @@ -34,6 +34,7 @@ namespace WebCore {
>      
>  CanvasObject::CanvasObject(WebGLRenderingContext* context)
>      : m_object(0)
> +    , m_shouldDeleteObject(true)
>      , m_context(context)
>  {
>  }
> @@ -44,24 +45,26 @@ CanvasObject::~CanvasObject()
>          m_context->removeObject(this);
>  }
>  
> -void CanvasObject::setObject(Platform3DObject object)
> +void CanvasObject::setObject(Platform3DObject object, bool shouldDeleteObject)
>  {
>      if (object == m_object)
>          return;
> -        
>      deleteObject();
>      m_object = object;
> +    m_shouldDeleteObject = shouldDeleteObject;
>  }

I'd like to see some comment about why shouldDeleteObject is needed.


>  void WebGLBuffer::_deleteObject(Platform3DObject object)
> Index: WebCore/html/canvas/WebGLBuffer.h
> ===================================================================
> --- WebCore/html/canvas/WebGLBuffer.h	(revision 51226)
> +++ WebCore/html/canvas/WebGLBuffer.h	(working copy)
> @@ -32,19 +32,24 @@
>  #include <wtf/RefCounted.h>
>  
>  namespace WebCore {
> -    
> +

It would make the patch less confusing if you avoided deleting whitespace like this


> Index: WebCore/html/canvas/WebGLFramebuffer.cpp
> ===================================================================
> --- WebCore/html/canvas/WebGLFramebuffer.cpp	(revision 51226)
> +++ WebCore/html/canvas/WebGLFramebuffer.cpp	(working copy)
> @@ -40,7 +40,7 @@ PassRefPtr<WebGLFramebuffer> WebGLFrameb
>  WebGLFramebuffer::WebGLFramebuffer(WebGLRenderingContext* ctx)
>      : CanvasObject(ctx)
>  {
> -    setObject(context()->graphicsContext3D()->createFramebuffer());
> +    setObject(context()->graphicsContext3D()->createFramebuffer(), true);
>  }

A default value on the second parameter would avoid these changes.


>  
>  void WebGLFramebuffer::_deleteObject(Platform3DObject object)
> Index: WebCore/html/canvas/WebGLGetInfo.cpp
> ===================================================================
> 
> Property changes on: WebCore/html/canvas/WebGLGetInfo.cpp
> ___________________________________________________________________
> Name: svn:eol-style
>    + LF

Why is this property needed? (elsewhere as well)


> Index: WebCore/html/canvas/WebGLRenderingContext.cpp
> ===================================================================
> --- WebCore/html/canvas/WebGLRenderingContext.cpp	(revision 51226)
> +++ WebCore/html/canvas/WebGLRenderingContext.cpp	(working copy)
> @@ -39,17 +39,210 @@
>  #include "HTMLCanvasElement.h"
>  #include "HTMLImageElement.h"
>  #include "ImageBuffer.h"
> +#include "NotImplemented.h"
>  #include "RenderBox.h"
>  #include "RenderLayer.h"
>  
>  namespace WebCore {
>  
> +// OpenGL constants needed for bindBuffer (and, implicitly, getParameter)
> +#define GL_ARRAY_BUFFER                     0x8892
> +#define GL_ELEMENT_ARRAY_BUFFER             0x8893
> +
> +// OpenGL constants needed for bindFramebuffer (and, implicitly, getParameter)
> +#define GL_FRAMEBUFFER                      0x8D40

I have a patch (https://bugs.webkit.org/show_bug.cgi?id=31239) which places all these into an enum in GraphicsContext3D.h. That's where they should go, so please lift that code instead.


> +class CleanupHelper {
> +public:
> +    CleanupHelper(WebGLRenderingContext* context,
> +                  bool changed)
> +        : m_context(context)
> +        , m_changed(changed)
> +    {
> +    }
> +
> +    ~CleanupHelper()
> +    {
> +        m_context->cleanupAfterGraphicsCall(m_changed);
> +    }

This is a nice idea. But it concerns me because it will take local storage in every function and will generally be more expensive in every call. Plus a change like this doesn't belong in this patch.

[Kenneth Russell]

(In reply to comment #5)
>  At the very least you should drop the removal of CanvasNumberArray files

OK, I'll revert these files.

[Kenneth Russell]

(In reply to comment #6)
> (From update of attachment 43541 [details])
> I leave the final review to Ollie, but here is my take:
> 
> > Index: WebCore/WebCore.gypi
> > ===================================================================
> > --- WebCore/WebCore.gypi	(revision 51226)
> > +++ WebCore/WebCore.gypi	(working copy)
> > @@ -1271,6 +1271,20 @@
> >              'history/HistoryItem.h',
> >              'history/PageCache.cpp',
> >              'history/PageCache.h',
> > +            'html/canvas/CanvasGradient.cpp',
> > +            'html/canvas/CanvasGradient.h',
> > +            'html/canvas/CanvasObject.cpp',
> > +            'html/canvas/CanvasObject.h',
> > +            'html/canvas/CanvasPattern.cpp',
> > +            'html/canvas/CanvasPattern.h',
> > +            'html/canvas/CanvasPixelArray.cpp',
> > +            'html/canvas/CanvasPixelArray.h',
> > +            'html/canvas/CanvasRenderingContext.cpp',
> > +            'html/canvas/CanvasRenderingContext.h',
> > +            'html/canvas/CanvasRenderingContext2D.cpp',
> > +            'html/canvas/CanvasRenderingContext2D.h',
> > +            'html/canvas/CanvasStyle.cpp',
> > +            'html/canvas/CanvasStyle.h',
> >              'html/canvas/WebGLArray.cpp',
> >              'html/canvas/WebGLArray.h',
> >              'html/canvas/WebGLArrayBuffer.cpp',
> > @@ -1283,32 +1297,20 @@
> >              'html/canvas/WebGLFloatArray.h',
> >              'html/canvas/WebGLFramebuffer.cpp',
> >              'html/canvas/WebGLFramebuffer.h',
> > -            'html/canvas/CanvasGradient.cpp',
> > -            'html/canvas/CanvasGradient.h',
> > +            'html/canvas/WebGLGetInfo.cpp',
> > +            'html/canvas/WebGLGetInfo.h',
> >              'html/canvas/WebGLIntArray.cpp',
> >              'html/canvas/WebGLIntArray.h',
> > -            'html/canvas/CanvasObject.cpp',
> > -            'html/canvas/CanvasObject.h',
> > -            'html/canvas/CanvasPattern.cpp',
> > -            'html/canvas/CanvasPattern.h',
> > -            'html/canvas/CanvasPixelArray.cpp',
> > -            'html/canvas/CanvasPixelArray.h',
> >              'html/canvas/WebGLProgram.cpp',
> >              'html/canvas/WebGLProgram.h',
> >              'html/canvas/WebGLRenderbuffer.cpp',
> >              'html/canvas/WebGLRenderbuffer.h',
> > -            'html/canvas/CanvasRenderingContext.cpp',
> > -            'html/canvas/CanvasRenderingContext.h',
> > -            'html/canvas/CanvasRenderingContext2D.cpp',
> > -            'html/canvas/CanvasRenderingContext2D.h',
> >              'html/canvas/WebGLRenderingContext.cpp',
> >              'html/canvas/WebGLRenderingContext.h',
> >              'html/canvas/WebGLShader.cpp',
> >              'html/canvas/WebGLShader.h',
> >              'html/canvas/WebGLShortArray.cpp',
> >              'html/canvas/WebGLShortArray.h',
> > -            'html/canvas/CanvasStyle.cpp',
> > -            'html/canvas/CanvasStyle.h',
> >              'html/canvas/WebGLTexture.cpp',
> >              'html/canvas/WebGLTexture.h',
> >              'html/canvas/WebGLUnsignedByteArray.cpp',
> 
> There are many files being added or removed from this file that seem to have
> nothing to do with the patch. I think the only thing that should be in this
> file is the addition of WebGLGetInfo

All I was doing was re-alphabetizing the files after the Canvas -> WebGL rename, but I'll undo these changes.

> > Index: WebCore/WebCore.xcodeproj/project.pbxproj
> > ===================================================================
> > --- WebCore/WebCore.xcodeproj/project.pbxproj	(revision 51226)
> > +++ WebCore/WebCore.xcodeproj/project.pbxproj	(working copy)
> > @@ -1199,6 +1199,8 @@
> >  		65DF323C09D1DE65000BE325 /* JSCanvasPattern.h in Headers */ = {isa = PBXBuildFile; fileRef = 65DF323609D1DE65000BE325 /* JSCanvasPattern.h */; };
> >  		65DF326109D1E199000BE325 /* UserAgentStyleSheetsData.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 656581AF09D14EE6000E61D7 /* UserAgentStyleSheetsData.cpp */; };
> >  		65FEA86909833ADE00BED4AB /* Page.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 65FEA86809833ADE00BED4AB /* Page.cpp */; };
> > +		6E51D61C10B23CC200FDE74A /* WebGLGetInfo.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6E51D61A10B23CC200FDE74A /* WebGLGetInfo.cpp */; };
> > +		6E51D61D10B23CC200FDE74A /* WebGLGetInfo.h in Headers */ = {isa = PBXBuildFile; fileRef = 6E51D61B10B23CC200FDE74A /* WebGLGetInfo.h */; };
> >  		72626E020EF022FE00A07E20 /* FontFastPath.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 72626E010EF022FE00A07E20 /* FontFastPath.cpp */; };
> >  		754133A8102E00E800075D00 /* InspectorTimelineAgent.h in Headers */ = {isa = PBXBuildFile; fileRef = 754133A7102E00E800075D00 /* InspectorTimelineAgent.h */; };
> >  		754133AA102E00F400075D00 /* InspectorTimelineAgent.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 754133A9102E00F400075D00 /* InspectorTimelineAgent.cpp */; };
> > @@ -6507,6 +6509,8 @@
> >  		65DF323609D1DE65000BE325 /* JSCanvasPattern.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; path = JSCanvasPattern.h; sourceTree = "<group>"; };
> >  		65F80697054D9F86008BF776 /* BlockExceptions.mm */ = {isa = PBXFileReference; fileEncoding = 30; indentWidth = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = BlockExceptions.mm; sourceTree = "<group>"; tabWidth = 8; usesTabs = 0; };
> >  		65FEA86809833ADE00BED4AB /* Page.cpp */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.cpp; path = Page.cpp; sourceTree = "<group>"; };
> > +		6E51D61A10B23CC200FDE74A /* WebGLGetInfo.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = WebGLGetInfo.cpp; path = canvas/WebGLGetInfo.cpp; sourceTree = "<group>"; };
> > +		6E51D61B10B23CC200FDE74A /* WebGLGetInfo.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = WebGLGetInfo.h; path = canvas/WebGLGetInfo.h; sourceTree = "<group>"; };
> >  		72626E010EF022FE00A07E20 /* FontFastPath.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = FontFastPath.cpp; sourceTree = "<group>"; };
> >  		754133A7102E00E800075D00 /* InspectorTimelineAgent.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = InspectorTimelineAgent.h; sourceTree = "<group>"; };
> >  		754133A9102E00F400075D00 /* InspectorTimelineAgent.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = InspectorTimelineAgent.cpp; sourceTree = "<group>"; };
> > @@ -10620,6 +10624,25 @@
> >  		49484FAE102CF01E00187DD3 /* canvas */ = {
> >  			isa = PBXGroup;
> >  			children = (
> > +				49484FB3102CF23C00187DD3 /* CanvasGradient.cpp */,
> > +				49484FB4102CF23C00187DD3 /* CanvasGradient.h */,
> > +				49484FB5102CF23C00187DD3 /* CanvasGradient.idl */,
> > +				49C7B9B41042D32F0009D447 /* CanvasObject.cpp */,
> > +				49C7B9B51042D32F0009D447 /* CanvasObject.h */,
> > +				49484FB6102CF23C00187DD3 /* CanvasPattern.cpp */,
> > +				49484FB7102CF23C00187DD3 /* CanvasPattern.h */,
> > +				49484FB8102CF23C00187DD3 /* CanvasPattern.idl */,
> > +				49484FB9102CF23C00187DD3 /* CanvasPixelArray.cpp */,
> > +				49484FBA102CF23C00187DD3 /* CanvasPixelArray.h */,
> > +				49484FBB102CF23C00187DD3 /* CanvasPixelArray.idl */,
> > +				49C7B9BC1042D32F0009D447 /* CanvasRenderingContext.cpp */,
> > +				49C7B9BD1042D32F0009D447 /* CanvasRenderingContext.h */,
> > +				49C7B9BE1042D32F0009D447 /* CanvasRenderingContext.idl */,
> > +				49484FBC102CF23C00187DD3 /* CanvasRenderingContext2D.cpp */,
> > +				49484FBD102CF23C00187DD3 /* CanvasRenderingContext2D.h */,
> > +				49484FBE102CF23C00187DD3 /* CanvasRenderingContext2D.idl */,
> > +				49484FBF102CF23C00187DD3 /* CanvasStyle.cpp */,
> > +				49484FC0102CF23C00187DD3 /* CanvasStyle.h */,
> 
> Same here. Why all these added files?

Just re-alphabetizing. I'll undo these changes.

> > Index: WebCore/bindings/js/JSCanvasNumberArrayCustom.cpp
> > ===================================================================
> > --- WebCore/bindings/js/JSCanvasNumberArrayCustom.cpp	(revision 51226)
> > +++ WebCore/bindings/js/JSCanvasNumberArrayCustom.cpp	(working copy)
> ...
> > Index: WebCore/html/canvas/CanvasNumberArray.cpp
> > ===================================================================
> > --- WebCore/html/canvas/CanvasNumberArray.cpp	(revision 51226)
> > +++ WebCore/html/canvas/CanvasNumberArray.cpp	(working copy)
> ...
> > -#endif // ENABLE(3D_CANVAS)
> > Index: WebCore/html/canvas/CanvasNumberArray.h
> > ===================================================================
> > --- WebCore/html/canvas/CanvasNumberArray.h	(revision 51226)
> > +++ WebCore/html/canvas/CanvasNumberArray.h	(working copy)
> ...
> > Index: WebCore/html/canvas/CanvasNumberArray.idl
> > ===================================================================
> > --- WebCore/html/canvas/CanvasNumberArray.idl	(revision 51226)
> > +++ WebCore/html/canvas/CanvasNumberArray.idl	(working copy)
> 
> These files are unrelated to this patch

I'll revert them.

> > Index: WebCore/html/canvas/CanvasObject.cpp
> > ===================================================================
> > --- WebCore/html/canvas/CanvasObject.cpp	(revision 51226)
> > +++ WebCore/html/canvas/CanvasObject.cpp	(working copy)
> > @@ -34,6 +34,7 @@ namespace WebCore {
> >      
> >  CanvasObject::CanvasObject(WebGLRenderingContext* context)
> >      : m_object(0)
> > +    , m_shouldDeleteObject(true)
> >      , m_context(context)
> >  {
> >  }
> > @@ -44,24 +45,26 @@ CanvasObject::~CanvasObject()
> >          m_context->removeObject(this);
> >  }
> >  
> > -void CanvasObject::setObject(Platform3DObject object)
> > +void CanvasObject::setObject(Platform3DObject object, bool shouldDeleteObject)
> >  {
> >      if (object == m_object)
> >          return;
> > -        
> >      deleteObject();
> >      m_object = object;
> > +    m_shouldDeleteObject = shouldDeleteObject;
> >  }
> 
> I'd like to see some comment about why shouldDeleteObject is needed.

I'll add a comment.

> >  void WebGLBuffer::_deleteObject(Platform3DObject object)
> > Index: WebCore/html/canvas/WebGLBuffer.h
> > ===================================================================
> > --- WebCore/html/canvas/WebGLBuffer.h	(revision 51226)
> > +++ WebCore/html/canvas/WebGLBuffer.h	(working copy)
> > @@ -32,19 +32,24 @@
> >  #include <wtf/RefCounted.h>
> >  
> >  namespace WebCore {
> > -    
> > +
> 
> It would make the patch less confusing if you avoided deleting whitespace like
> this

I'll undo the whitespace changes.

> > Index: WebCore/html/canvas/WebGLFramebuffer.cpp
> > ===================================================================
> > --- WebCore/html/canvas/WebGLFramebuffer.cpp	(revision 51226)
> > +++ WebCore/html/canvas/WebGLFramebuffer.cpp	(working copy)
> > @@ -40,7 +40,7 @@ PassRefPtr<WebGLFramebuffer> WebGLFrameb
> >  WebGLFramebuffer::WebGLFramebuffer(WebGLRenderingContext* ctx)
> >      : CanvasObject(ctx)
> >  {
> > -    setObject(context()->graphicsContext3D()->createFramebuffer());
> > +    setObject(context()->graphicsContext3D()->createFramebuffer(), true);
> >  }
> 
> A default value on the second parameter would avoid these changes.

I thought it would be better to have the parameter more explicit, but I'll add a default of true.

> >  void WebGLFramebuffer::_deleteObject(Platform3DObject object)
> > Index: WebCore/html/canvas/WebGLGetInfo.cpp
> > ===================================================================
> > 
> > Property changes on: WebCore/html/canvas/WebGLGetInfo.cpp
> > ___________________________________________________________________
> > Name: svn:eol-style
> >    + LF
> 
> Why is this property needed? (elsewhere as well)

I have this set in my Subversion properties for the Chromium coding guidelines. I'll remove it.

> > Index: WebCore/html/canvas/WebGLRenderingContext.cpp
> > ===================================================================
> > --- WebCore/html/canvas/WebGLRenderingContext.cpp	(revision 51226)
> > +++ WebCore/html/canvas/WebGLRenderingContext.cpp	(working copy)
> > @@ -39,17 +39,210 @@
> >  #include "HTMLCanvasElement.h"
> >  #include "HTMLImageElement.h"
> >  #include "ImageBuffer.h"
> > +#include "NotImplemented.h"
> >  #include "RenderBox.h"
> >  #include "RenderLayer.h"
> >  
> >  namespace WebCore {
> >  
> > +// OpenGL constants needed for bindBuffer (and, implicitly, getParameter)
> > +#define GL_ARRAY_BUFFER                     0x8892
> > +#define GL_ELEMENT_ARRAY_BUFFER             0x8893
> > +
> > +// OpenGL constants needed for bindFramebuffer (and, implicitly, getParameter)
> > +#define GL_FRAMEBUFFER                      0x8D40
> 
> I have a patch (https://bugs.webkit.org/show_bug.cgi?id=31239) which places all
> these into an enum in GraphicsContext3D.h. That's where they should go, so
> please lift that code instead.

I'll do that.

> > +class CleanupHelper {
> > +public:
> > +    CleanupHelper(WebGLRenderingContext* context,
> > +                  bool changed)
> > +        : m_context(context)
> > +        , m_changed(changed)
> > +    {
> > +    }
> > +
> > +    ~CleanupHelper()
> > +    {
> > +        m_context->cleanupAfterGraphicsCall(m_changed);
> > +    }
> 
> This is a nice idea. But it concerns me because it will take local storage in
> every function and will generally be more expensive in every call. Plus a
> change like this doesn't belong in this patch.

This is an essential part of the patch. Without it all of the get routines are significantly more complex and error-prone since they can not just return a value as their last statement. The local storage requirements, if the compiler doesn't completely eliminate them, are negligible and the cost is exactly the same as the previous code structure. Please let's leave this as is.

[Kenneth Russell]

Created attachment 43641 [details]
Revised patch

Addressed all review feedback above. See ChangeLogs for revised descriptions. Re-tested WebKit and Chromium.

[Kenneth Russell]

Any comments on the revised patch? Would like to land this to start updating demos.

[Oliver Hunt]

Comment on attachment 43641 [details]
Revised patch

The WebGLGetInfo::Type enum does not match the webkit style guidelines -- http://webkit.org/coding/coding-style.html -- They should be camel cased, with Type as a prefix rather than T_

I don't like the CleanupHelper class changes -- i believe they can be made in an independent patch the preceeds the main fix.  Currently there is a lot of noise due to your updating every cleanupAfterGraphicsCall call site despite those sites not actually being involved in what the patch is meant to be doing.

I really like the changes to GraphicsContext3D -- anything that removes DOM objects from the platform layer is a good thing.  It's something of a mistake that they were there to begin with.

I'd like to make those methods that depend on WebGLGetInfo to be automatically generated, but i believe doing so would be to involved to require you to do.

Unfortunately you'll have to resolve the two issues i mentioned before i can r+ this.

[Kenneth Russell]

Created attachment 43722 [details]
Revised patch

Changed WebGLGetInfo enums to camel case. Removed use of CleanupHelper from all non-get methods. Rebuilt and retested WebKit and Chromium.

[Oliver Hunt]

Comment on attachment 43722 [details]
Revised patch

r=me, only concern is that the more i think about it, the less i like the name CleanupHelper -- WebGLStateRestorer maybe?  I'd like the WebGL prefix at least so it's not just sitting around with the generic "CleanupHelper" name.

[Kenneth Russell]

Created attachment 43728 [details]
Revised patch

Renamed CleanupHelper to WebGLStateRestorer as suggested.

[Oliver Hunt]

Comment on attachment 43728 [details]
Revised patch

Cheers Ken :D

[WebKit Commit Bot]

Comment on attachment 43728 [details]
Revised patch

Rejecting patch 43728 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11657 test cases.
fast/canvas/webgl/gl-object-get-calls.html -> failed

Exiting early after 1 failures. 4967 tests run.
85.57s total testing time

4966 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

[Kenneth Russell]

(In reply to comment #16)
> (From update of attachment 43728 [details])
> Rejecting patch 43728 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari',
> '--quiet', '--exit-after-n-failures=1']" exit_code: 1
> Running build-dumprendertree
> Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
> Testing 11657 test cases.
> fast/canvas/webgl/gl-object-get-calls.html -> failed
> 
> Exiting early after 1 failures. 4967 tests run.
> 85.57s total testing time
> 
> 4966 test cases (99%) succeeded
> 1 test case (<1%) had incorrect layout
> 1 test case (<1%) had stderr output

How can I get more information about why this patch failed the commit queue? This test is one whose expected output is changed by this patch. I've run it locally and it works fine.

[Kenneth Russell]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 43728 [details] [details])
> > Rejecting patch 43728 from commit-queue.
> > 
> > Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari',
> > '--quiet', '--exit-after-n-failures=1']" exit_code: 1
> > Running build-dumprendertree
> > Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
> > Testing 11657 test cases.
> > fast/canvas/webgl/gl-object-get-calls.html -> failed
> > 
> > Exiting early after 1 failures. 4967 tests run.
> > 85.57s total testing time
> > 
> > 4966 test cases (99%) succeeded
> > 1 test case (<1%) had incorrect layout
> > 1 test case (<1%) had stderr output
> 
> How can I get more information about why this patch failed the commit queue?
> This test is one whose expected output is changed by this patch. I've run it
> locally and it works fine.

Oops, I misread the name of the test case. This is the new one associated with this patch. Regardless, it runs fine on my machine so I would need the stderr output from the run on the commit bot to diagnose the failure.

[Oliver Hunt]

> > How can I get more information about why this patch failed the commit queue?
> > This test is one whose expected output is changed by this patch. I've run it
> > locally and it works fine.
> 
> Oops, I misread the name of the test case. This is the new one associated with
> this patch. Regardless, it runs fine on my machine so I would need the stderr
> output from the run on the commit bot to diagnose the failure.

I'd recommend talking to eric seidel

[Kenneth Russell]

(In reply to comment #19)
> > > How can I get more information about why this patch failed the commit queue?
> > > This test is one whose expected output is changed by this patch. I've run it
> > > locally and it works fine.
> > 
> > Oops, I misread the name of the test case. This is the new one associated with
> > this patch. Regardless, it runs fine on my machine so I would need the stderr
> > output from the run on the commit bot to diagnose the failure.
> 
> I'd recommend talking to eric seidel

Emailed him; still waiting for reply.

I've run this test on two machines with very different graphics cards so far and it passes on both. Is there any chance that the bot didn't apply all the changes, in particular LayoutTests/fast/js/resources/js-test-pre.js?

[Adam Barth]

> Emailed him; still waiting for reply.

I think he's on an airplane.  You can try committing and see what the buildbot thinks.

[Kenneth Russell]

(In reply to comment #21)
> > Emailed him; still waiting for reply.
> 
> I think he's on an airplane.  You can try committing and see what the buildbot
> thinks.

Oliver, Chris, what would you think about landing this patch manually?

I certainly don't want to introduce a failing layout test but we really need these API changes in place to finalize demos for the initial public release of the spec. If it turns out the layout test is failing on many machines I'll file a bug to comment out its body and adjust the expected output.

[Chris Marrin]

I will try landing it manually. It will have to wait till tomorrow though

[Eric Seidel (no email)]

Comment on attachment 43728 [details]
Revised patch

Sorry for the delay.  Here is the diff:

--- /tmp/layout-test-results/fast/canvas/webgl/gl-object-get-calls-expected.txt	2009-11-23 13:52:23.000000000 -0800
+++ /tmp/layout-test-results/fast/canvas/webgl/gl-object-get-calls-actual.txt	2009-11-23 13:52:23.000000000 -0800
@@ -7,7 +7,7 @@
 PASS gl.getError() is 0
 PASS gl.getError() is 0
 PASS gl.getError() is 0
-PASS gl.checkFramebufferStatus(gl.FRAMEBUFFER) is gl.FRAMEBUFFER_COMPLETE
+FAIL gl.checkFramebufferStatus(gl.FRAMEBUFFER) should be 36053. Was 36061.
 PASS gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE) is gl.TEXTURE
 PASS gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_OBJECT_NAME) is non-null.
 PASS gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_TEXTURE_LEVEL) is 0

The lack of commit-queue support for uploading failure diffs is sorta covered by bug 28286.  I can look into fixing that.

[Oliver Hunt]

Oddly that's indicating it's getting the error code FRAMEBUFFER_UNSUPPORTED from checkFramebufferStatus -- any ideas as to why that may be?

--Oliver

(In reply to comment #24)
> (From update of attachment 43728 [details])
> Sorry for the delay.  Here is the diff:
> 
> --- /tmp/layout-test-results/fast/canvas/webgl/gl-object-get-calls-expected.txt
>    2009-11-23 13:52:23.000000000 -0800
> +++ /tmp/layout-test-results/fast/canvas/webgl/gl-object-get-calls-actual.txt  
>  2009-11-23 13:52:23.000000000 -0800
> @@ -7,7 +7,7 @@
>  PASS gl.getError() is 0
>  PASS gl.getError() is 0
>  PASS gl.getError() is 0
> -PASS gl.checkFramebufferStatus(gl.FRAMEBUFFER) is gl.FRAMEBUFFER_COMPLETE
> +FAIL gl.checkFramebufferStatus(gl.FRAMEBUFFER) should be 36053. Was 36061.
>  PASS gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER,
> gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE) is gl.TEXTURE
>  PASS gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER,
> gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_OBJECT_NAME) is non-null.
>  PASS gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER,
> gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_TEXTURE_LEVEL) is 0
> 
> The lack of commit-queue support for uploading failure diffs is sorta covered
> by bug 28286.  I can look into fixing that.

[Kenneth Russell]

@Eric: thanks for the diff. Fixing 28286 would be really helpful.

@Oliver: yes, I think it's because I forgot to initialize the minification and magnification filters on the framebuffer's texture to disable the use of mipmaps. I have another fix to the new code, plus merging with Chris's changes from today, and will upload a new patch shortly.

(In reply to comment #25)
> Oddly that's indicating it's getting the error code FRAMEBUFFER_UNSUPPORTED
> from checkFramebufferStatus -- any ideas as to why that may be?
> 
> --Oliver
> 
> (In reply to comment #24)
> > (From update of attachment 43728 [details] [details])
> > Sorry for the delay.  Here is the diff:
> > 
> > --- /tmp/layout-test-results/fast/canvas/webgl/gl-object-get-calls-expected.txt
> >    2009-11-23 13:52:23.000000000 -0800
> > +++ /tmp/layout-test-results/fast/canvas/webgl/gl-object-get-calls-actual.txt  
> >  2009-11-23 13:52:23.000000000 -0800
> > @@ -7,7 +7,7 @@
> >  PASS gl.getError() is 0
> >  PASS gl.getError() is 0
> >  PASS gl.getError() is 0
> > -PASS gl.checkFramebufferStatus(gl.FRAMEBUFFER) is gl.FRAMEBUFFER_COMPLETE
> > +FAIL gl.checkFramebufferStatus(gl.FRAMEBUFFER) should be 36053. Was 36061.
> >  PASS gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER,
> > gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE) is gl.TEXTURE
> >  PASS gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER,
> > gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_OBJECT_NAME) is non-null.
> >  PASS gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER,
> > gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_TEXTURE_LEVEL) is 0
> > 
> > The lack of commit-queue support for uploading failure diffs is sorta covered
> > by bug 28286.  I can look into fixing that.

[Kenneth Russell]

Created attachment 43747 [details]
Revised patch

Merged with WebGL changes from earlier today.

Fixed problem in new test case where minification and magnification filters on FBO's texture were not initialized. This is probably why the patch failed the commit queue the first time.

Fixed bug in bounds checking logic in WebGLRenderingContext::activeTexture uncovered by Google's WebGL demos. Made activeTexture raise DOMException per spec and actually throw an exception to prevent this kind of error from being silently squelched.

[Kenneth Russell]

Also rebuilt and retested WebKit and Chromium.

[Oliver Hunt]

Comment on attachment 43747 [details]
Revised patch

Any idea what's caused the 10k change in patch size?

[Kenneth Russell]

(In reply to comment #29)
> (From update of attachment 43747 [details])
> Any idea what's caused the 10k change in patch size?

Chris incorporated the enums into GraphicsContext3D.h which I had previously copied from his patch.

[Oliver Hunt]

Comment on attachment 43747 [details]
Revised patch

awesome!

Let's see if we can get this landed

[WebKit Commit Bot]

Comment on attachment 43747 [details]
Revised patch

Rejecting patch 43747 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11663 test cases.
fast/canvas/webgl/gl-object-get-calls.html -> failed

Exiting early after 1 failures. 4970 tests run.
89.07s total testing time

4969 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

[Eric Seidel (no email)]

commit-queue shows the same exact failure diff again this time:
--- /tmp/layout-test-results/fast/canvas/webgl/gl-object-get-calls-expected.txt	2009-11-23 20:52:29.000000000 -0800
+++ /tmp/layout-test-results/fast/canvas/webgl/gl-object-get-calls-actual.txt	2009-11-23 20:52:29.000000000 -0800
@@ -7,7 +7,7 @@
 PASS gl.getError() is 0
 PASS gl.getError() is 0
 PASS gl.getError() is 0
-PASS gl.checkFramebufferStatus(gl.FRAMEBUFFER) is gl.FRAMEBUFFER_COMPLETE
+FAIL gl.checkFramebufferStatus(gl.FRAMEBUFFER) should be 36053. Was 36061.
 PASS gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE) is gl.TEXTURE
 PASS gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_OBJECT_NAME) is non-null.
 PASS gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.FRAMEBUFFER_ATTACHMENT_TEXTURE_LEVEL) is 0

[Kenneth Russell]

My best guess at this point is that the graphics driver on the commit bots is buggy and does not support GL_DEPTH_COMPONENT16 as it must per the OpenGL spec. We'll need to work around this in the WebGL implementation, but that will probably require some iterative hacking. I propose we comment out the affected portion of the test case, update the expected file, and check in the rest of the patch, filing a follow-on bug to do this workaround and enable the rest of the test. Comments? If we're in agreement, I'll resubmit the patch.

[Kenneth Russell]

Created attachment 43781 [details]
Revised patch

[Kenneth Russell]

Created attachment 43783 [details]
Revised patch

Commented out portion of test case failing on commit bots and regenerated expected.txt. Will work around this in a follow-on bug. Rest of the code is unchanged.

[Chris Marrin]

Landed: http://trac.webkit.org/changeset/51348

There were 3 problems:

1) The toJS function in JSWebGLRenderingContextCustom.cpp needed to be made
static

2) gl-get-calls.html was failing. I commented out the tests and opened
https://bugs.webkit.org/show_bug.cgi?id=31842

3) gl-object-get-calls.html will fail on some drivers (as pointed out in a
previous post), so I changed it to use DEPTH_ATTACHMENT rather than
DEPTH_ATTACHMENT16 and opened https://bugs.webkit.org/show_bug.cgi?id=31843

[Chris Marrin]

Ken, we collided. I landed the patch from 2009-11-23 20:22 PST. Please let me know what changes you made in the last two patches. If they have to do with the failing tests, I believe I fixed those. See the comments and the newly posted bugs and let me know if there is more you feel is needed.

[Kenneth Russell]

(In reply to comment #38)
> Ken, we collided. I landed the patch from 2009-11-23 20:22 PST. Please let me
> know what changes you made in the last two patches. If they have to do with the
> failing tests, I believe I fixed those. See the comments and the newly posted
> bugs and let me know if there is more you feel is needed.

The patch from 2009-11-24 10:24 PST was an accident.

The only difference between the one you landed and 2009-11-24 10:26 PST was the removal of the check for FRAMEBUFFER_COMPLETE in the gl-object-get-calls test. I'm surprised gl-get-calls was failing; let's talk offline about which graphics card you ran on.

[Kenneth Russell]

Comment on attachment 43783 [details]
Revised patch

Obsoleting patch and canceling review.

[Eric Seidel (no email)]

Sadly the same failures we saw trying to get this landed, still occurred after the final patch was landed.  See bug 31919.