Bug 30091 - Change get... calls to latest spec
Summary: Change get... calls to latest spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-05 13:13 PDT by Chris Marrin
Modified: 2009-11-26 12:22 PST (History)
9 users (show)

See Also:


Attachments
Proposed patch (253.83 KB, patch)
2009-11-19 19:30 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Revised patch (240.42 KB, patch)
2009-11-20 20:41 PST, Kenneth Russell
oliver: review-
Details | Formatted Diff | Diff
Revised patch (202.50 KB, patch)
2009-11-23 11:31 PST, Kenneth Russell
oliver: review+
oliver: commit-queue-
Details | Formatted Diff | Diff
Revised patch (202.60 KB, patch)
2009-11-23 13:25 PST, Kenneth Russell
oliver: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Revised patch (191.96 KB, patch)
2009-11-23 20:22 PST, Kenneth Russell
oliver: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Revised patch (192.06 KB, application/octet-stream)
2009-11-24 10:24 PST, Kenneth Russell
no flags Details
Revised patch (192.06 KB, patch)
2009-11-24 10:26 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-10-05 13:13:33 PDT
We've changed the get API to return 'any' in many cases to reduce the number of call.
Comment 1 Kenneth Russell 2009-11-19 19:30:57 PST
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.
Comment 2 Oliver Hunt 2009-11-19 21:44:19 PST
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
Comment 3 Kenneth Russell 2009-11-20 01:13:12 PST
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.
Comment 4 Dimitri Glazkov (Google) 2009-11-20 09:16:36 PST
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 :)
Comment 5 Oliver Hunt 2009-11-20 09:56:05 PST
(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.
Comment 6 Chris Marrin 2009-11-20 11:42:15 PST
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.
Comment 7 Kenneth Russell 2009-11-20 19:03:48 PST
(In reply to comment #5)
>  At the very least you should drop the removal of CanvasNumberArray files

OK, I'll revert these files.
Comment 8 Kenneth Russell 2009-11-20 19:18:59 PST
(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.
Comment 9 Kenneth Russell 2009-11-20 20:41:08 PST
Created attachment 43641 [details]
Revised patch

Addressed all review feedback above. See ChangeLogs for revised descriptions. Re-tested WebKit and Chromium.
Comment 10 Kenneth Russell 2009-11-23 10:17:29 PST
Any comments on the revised patch? Would like to land this to start updating demos.
Comment 11 Oliver Hunt 2009-11-23 10:41:55 PST
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.
Comment 12 Kenneth Russell 2009-11-23 11:31:18 PST
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.
Comment 13 Oliver Hunt 2009-11-23 11:42:42 PST
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.
Comment 14 Kenneth Russell 2009-11-23 13:25:11 PST
Created attachment 43728 [details]
Revised patch

Renamed CleanupHelper to WebGLStateRestorer as suggested.
Comment 15 Oliver Hunt 2009-11-23 13:26:35 PST
Comment on attachment 43728 [details]
Revised patch

Cheers Ken :D
Comment 16 WebKit Commit Bot 2009-11-23 13:52:30 PST
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
Comment 17 Kenneth Russell 2009-11-23 13:59:39 PST
(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.
Comment 18 Kenneth Russell 2009-11-23 14:02:27 PST
(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.
Comment 19 Oliver Hunt 2009-11-23 14:03:55 PST
> > 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
Comment 20 Kenneth Russell 2009-11-23 16:01:22 PST
(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?
Comment 21 Adam Barth 2009-11-23 16:04:51 PST
> Emailed him; still waiting for reply.

I think he's on an airplane.  You can try committing and see what the buildbot thinks.
Comment 22 Kenneth Russell 2009-11-23 16:55:58 PST
(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.
Comment 23 Chris Marrin 2009-11-23 17:00:06 PST
I will try landing it manually. It will have to wait till tomorrow though
Comment 24 Eric Seidel (no email) 2009-11-23 19:58:00 PST
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.
Comment 25 Oliver Hunt 2009-11-23 20:06:44 PST
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.
Comment 26 Kenneth Russell 2009-11-23 20:12:11 PST
@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.
Comment 27 Kenneth Russell 2009-11-23 20:22:44 PST
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.
Comment 28 Kenneth Russell 2009-11-23 20:24:17 PST
Also rebuilt and retested WebKit and Chromium.
Comment 29 Oliver Hunt 2009-11-23 20:31:46 PST
Comment on attachment 43747 [details]
Revised patch

Any idea what's caused the 10k change in patch size?
Comment 30 Kenneth Russell 2009-11-23 20:37:25 PST
(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.
Comment 31 Oliver Hunt 2009-11-23 20:38:58 PST
Comment on attachment 43747 [details]
Revised patch

awesome!

Let's see if we can get this landed
Comment 32 WebKit Commit Bot 2009-11-23 20:52:37 PST
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
Comment 33 Eric Seidel (no email) 2009-11-23 21:00:23 PST
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
Comment 34 Kenneth Russell 2009-11-23 21:54:36 PST
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.
Comment 35 Kenneth Russell 2009-11-24 10:24:05 PST
Created attachment 43781 [details]
Revised patch
Comment 36 Kenneth Russell 2009-11-24 10:26:41 PST
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.
Comment 37 Chris Marrin 2009-11-24 12:12:31 PST
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
Comment 38 Chris Marrin 2009-11-24 12:15:25 PST
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.
Comment 39 Kenneth Russell 2009-11-24 12:20:23 PST
(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.
Comment 40 Kenneth Russell 2009-11-24 14:18:21 PST
Comment on attachment 43783 [details]
Revised patch

Obsoleting patch and canceling review.
Comment 41 Eric Seidel (no email) 2009-11-26 12:22:39 PST
Sadly the same failures we saw trying to get this landed, still occurred after the final patch was landed.  See bug 31919.