Bug 28018 – Need to implement Canvas3d/WebGL for 3D rendering

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Chris Marrin]

This bug is the placeholder for the WebGL patches. WebGL is a Khronos effort to add a 3D rendering context to the Canvas element. The API is compatible with OpenGL ES 2.0. The spec is not yet public, so this implementation is subject to change as the spec evolves. The first implementation will be a fairly complete implementation of ES 2.0, and will support Image and Canvas image sources.

[Chris Marrin]

Created attachment 34145 [details]
Patch 1 of 4: Add ENABLE_3D_CANVAS flag to the build

[Chris Marrin]

Created attachment 34146 [details]
Patch 2 of 4: New Canvas 3D files

[Chris Marrin]

Created attachment 34149 [details]
Patch 3 of 4: Adding new files to the build

[Chris Marrin]

Created attachment 34151 [details]
Patch 4 of 4: Connect everything up

[Dave Hyatt]

Comment on attachment 34145 [details]
Patch 1 of 4: Add ENABLE_3D_CANVAS flag to the build

r=me

[Oliver Hunt]

Comment on attachment 34146 [details]
Patch 2 of 4: New Canvas 3D files


> +static inline PassRefPtr<CanvasNumberArray> toCanvasNumberArray(JSC::ExecState* exec, const JSValue& value)
This is fairly inefficient, and will allow non-array types, eg:
toCanvasArray({length:5}) will produce a CanvasFloatArray of 5 NaNs

> +{
> +    if (!value.isObject())
> +        return 0;
> +        
> +    JSObject* array = asObject(value);
> +    int length = array->get(exec, Identifier(exec, "length")).toInt32(exec);
> +    RefPtr<CanvasNumberArray> numberArray = CanvasNumberArray::create(length);
> +    for (int i = 0; i < length; ++i) {
> +        float value = array->get(exec, i).toFloat(exec);
Because this is a generic accessor function (and because toFloat may throw) you need:
JSValue v = array->get(exec, i);
if (exec->hadException())
    return 0;
float value = v.toFloat(exec);
if (exec->hadException())
    return 0;

> +        numberArray->data()[i] = value;
> +    }
> +    
> +    return numberArray;
> +}

If you disallow non-array types (i'm not sure if you want to do this or not) then you can devirtualise and remove the get

> +
> +JSValue JSCanvasRenderingContext3D::glBufferData(JSC::ExecState* exec, JSC::ArgList const& args)
> +{
> +    unsigned target = args.at(0).toInt32(exec);
> +    unsigned usage = args.at(2).toInt32(exec);
These may throw -- and the values are untrusted (they may not even be present)
> +    RefPtr<CanvasNumberArray> numberArray = toCanvasNumberArray(exec, args.at(1));
numberArray may be null where is this handled?

> +
> +JSValue JSCanvasRenderingContext3D::glBufferSubData(JSC::ExecState* exec, JSC::ArgList const& args)
> +{
> +    unsigned target = args.at(0).toInt32(exec);
> +    unsigned offset = args.at(1).toInt32(exec);
ditto

> +JSValue JSCanvasRenderingContext3D::glVertexAttrib(JSC::ExecState* exec, JSC::ArgList const& args)
> +{
> +    unsigned indx = args.at(0).toInt32(exec);
ditto

> +// void glVertexAttribPointer (in unsigned long indx, in long size, in unsigned long type, in boolean normalized, in unsigned long stride, in CanvasNumberArray array);
> +JSValue JSCanvasRenderingContext3D::glVertexAttribPointer(JSC::ExecState* exec, JSC::ArgList const& args)
> +{
> +    unsigned indx = args.at(0).toInt32(exec);
> +    unsigned size = args.at(1).toInt32(exec);
> +    unsigned type = args.at(2).toInt32(exec);
> +    unsigned normalized = args.at(3).toInt32(exec);
> +    bool stride = args.at(4).toBoolean(exec);
ditto

> +JSValue JSCanvasRenderingContext3D::glUniformMatrix(JSC::ExecState* exec, JSC::ArgList const& args)
> +{
> +    unsigned location = args.at(0).toInt32(exec);
> +    unsigned count = args.at(1).toInt32(exec);
> +    bool transpose = args.at(2).toBoolean(exec);
> +
ditto

> +    if (!args.at(3).isObject())
> +        return jsUndefined();
> +        
> +    WebKitCSSMatrix* matrix = toWebKitCSSMatrix(args.at(3));
> +    if (matrix)
> +        impl()->glUniformMatrix(location, transpose, matrix);
> +    else {
> +        JSObject* array = asObject(args.at(3));
Same as before are you sure we want to allow non-array arguments?

> +JSValue JSCanvasRenderingContext3D::glUniformf(JSC::ExecState* exec, JSC::ArgList const& args)
> +{
> +    unsigned location = args.at(0).toInt32(exec);
> +    
> +    switch(args.size()) {
> +        case 2:
> +            if (args.at(1).isObject()) {
> +                RefPtr<CanvasNumberArray> numberArray = toCanvasNumberArray(exec, args.at(1));
> +                impl()->glUniform(location, numberArray.get());
> +            }
> +            else
> +                impl()->glUniform(location, (float) args.at(1).toNumber(exec));
toNumber may throw, should be using a C++ cast (woo! a style comment! that'll annoy smfr :D)

> +            break;
> +        case 3: 
> +            impl()->glUniform(location, (float) args.at(1).toNumber(exec),
> +                                        (float) args.at(2).toNumber(exec)); 
> +            break;
> +        case 4: 
> +            impl()->glUniform(location, (float) args.at(1).toNumber(exec),
> +                                        (float) args.at(2).toNumber(exec),
> +                                        (float) args.at(3).toNumber(exec)); 
> +            break;
> +        case 5: 
> +            impl()->glUniform(location, (float) args.at(1).toNumber(exec),
> +                                        (float) args.at(2).toNumber(exec),
> +                                        (float) args.at(3).toNumber(exec),
> +                                        (float) args.at(4).toNumber(exec)); 
> +            break;

You should probably throw an exception if the numbe rof arguments doesn't match any expected count.

> +JSValue JSCanvasRenderingContext3D::glUniformi(JSC::ExecState* exec, JSC::ArgList const& args)
> +{
Same as before, i really wish we have auto codegen for variadic arguments

> +JSValue JSCanvasRenderingContext3D::glDrawElements(JSC::ExecState* exec, JSC::ArgList const& args)
> +{
> +    unsigned mode = args.at(0).toInt32(exec);
> +    unsigned type = args.at(1).toInt32(exec);
> +    
> +    unsigned int count = 0;
> +    
> +    // If the third param is not an object, it is a number, which is the count.
> +    // In this case if there is a 4th param, it is the offset. If there is no
> +    // 4th param, the offset is 0
> +    if (!args.at(2).isObject()) {
> +        count = args.at(2).toInt32(exec);
> +        unsigned int offset = (args.size() > 3) ? args.at(3).toInt32(exec) : 0;
> +        impl()->glDrawElements(mode, count, type, (void*) offset);
> +    } else {
> +        void* passedIndices = 0;
passedIndices should just be an OwnFastMallocPtr, and you should use fastMalloc in place of malloc.
It would be better if you used tryFastMalloc, and null check the result so we don't crash if malloc fails.

> +        JSObject* array = asObject(args.at(2));
> +        count = array->get(exec, Identifier(exec, "length")).toInt32(exec);
> +
> +        if (type == GL_UNSIGNED_BYTE) {
> +            unsigned char* indices = (unsigned char*) malloc(count * sizeof(unsigned short));
fastmAlloc should be used instead of malloc, also should use c style cast.

> +        } else if (type == GL_UNSIGNED_SHORT) {
> +            unsigned short* indices = (unsigned short*) malloc(count * sizeof(unsigned short));
ditto
> +        } else
TypeError should be thrown on argument mismatch
> +            return jsUndefined();
> +            
> +        impl()->glDrawElements(mode, count, type, passedIndices);
> +        free(passedIndices);
OwnFastMallocPtr saves manually calling free
> +    }
> +
> +    return jsUndefined();
> +}
> +
> +// void texImage2DHTML(in unsigned long target, in unsigned long level, in HTMLImageElement image);
> +JSValue JSCanvasRenderingContext3D::texImage2D(ExecState* exec, const ArgList& args)
> +{ 
> +    CanvasRenderingContext3D* context = impl();
> +    unsigned target = args.at(0).toInt32(exec);
> +    unsigned level = args.at(1).toInt32(exec);
exception and untrusted args again
> +    
> +    // The image parameter can be a <img> or <canvas> element.
> +    JSValue value = args.at(2);
> +    if (!value.isObject())
> +        return throwError(exec, TypeError);
> +    JSObject* o = asObject(value);
> +    
> +    ExceptionCode ec = 0;
> +    if (o->inherits(&JSHTMLImageElement::s_info)) {
> +        HTMLImageElement* imgElt = static_cast<HTMLImageElement*>(static_cast<JSHTMLElement*>(o)->impl());
> +        context->texImage2D(target, level, imgElt, ec);
> +    } else if (o->inherits(&JSHTMLCanvasElement::s_info)) {
> +        HTMLCanvasElement* canvas = static_cast<HTMLCanvasElement*>(static_cast<JSHTMLElement*>(o)->impl());
> +        context->texImage2D(target, level, canvas, ec);
<video> should also be allowed here

> +// void texSubImage2DHTML(in unsigned long target, in unsigned long level, in unsigned long xoff, in unsigned long yoff, in unsigned long width, in unsigned long height, in HTMLImageElement image);
> +JSValue JSCanvasRenderingContext3D::texSubImage2D(ExecState* exec, const ArgList& args)
> +{ 
> +    CanvasRenderingContext3D* context = impl();
> +    unsigned target = args.at(0).toInt32(exec);
> +    unsigned level = args.at(1).toInt32(exec);
> +    unsigned xoff = args.at(2).toInt32(exec);
> +    unsigned yoff = args.at(3).toInt32(exec);
> +    unsigned width = args.at(4).toInt32(exec);
> +    unsigned height = args.at(5).toInt32(exec);
ditto
> +    
> +    ExceptionCode ec = 0;
> +    if (o->inherits(&JSHTMLImageElement::s_info)) {
> +        HTMLImageElement* imgElt = static_cast<HTMLImageElement*>(static_cast<JSHTMLElement*>(o)->impl());
> +        context->texSubImage2D(target, level, xoff, yoff, width, height, imgElt, ec);
> +    } else if (o->inherits(&JSHTMLCanvasElement::s_info)) {
> +        HTMLCanvasElement* canvas = static_cast<HTMLCanvasElement*>(static_cast<JSHTMLElement*>(o)->impl());
> +        context->texSubImage2D(target, level, xoff, yoff, width, height, canvas, ec);
ditto


> +JSValue JSHTMLCanvasElement::getContext(JSC::ExecState* exec, JSC::ArgList const& args)
> +{
> +    HTMLCanvasElement* imp = static_cast<HTMLCanvasElement*>(impl());
> +    const UString& contextId = args.at(0).toString(exec);
> +
> +    JSValue result = jsUndefined();
> +    
> +    if (contextId == "2d")
> +        result = toJS(exec, WTF::getPtr(reinterpret_cast<CanvasRenderingContext2D*>(imp->getContext(contextId))));
> +#if ENABLE(3D_CANVAS)
> +    else if (contextId == "webkit-3d")
> +        result = toJS(exec, WTF::getPtr(reinterpret_cast<CanvasRenderingContext3D*>(imp->getContext(contextId))));
> +#endif
> +    
> +    return result;
> +}
This makes me sad, we should probably make toJS(CanvasRenderingContext*) do the coercion there, rather than making this a custom binding.

> Index: WebCore/html/CanvasByteArray.cpp
> +    String CanvasByteArray::toString() const
> +    {
> +        String s = "[ ";
> +        for (size_t i = 0; i < m_data.size(); ++i) {
> +            if (i != 0)
> +                s += ", ";
> +            s += String::number(m_data[i]);
> +        }
> +        
> +        s += " ]";
> +        return s;
> +    }

> +        [DontEnum] DOMString toString();
Most other DOM objects do not have toString implementations, so this seems unusual.  Its syntax also does not match the syntax standard array toString, i would suggest removing this.

> +        virtual void _deleteObject(Platform3DObject);
why the _?

> +    String CanvasNumberArray::toString() const
> +    {
> +        String s = "[ ";
> +        for (size_t i = 0; i < m_data.size(); ++i) {
> +            if (i != 0)
> +                s += ", ";
> +            s += String::number(m_data[i]);
> +        }
> +        
> +        s += " ]";
> +        return s;
> +    }

> +        [DontEnum] DOMString toString();

Same again for CanvasNumberArray.  I have a feeling that these are sufficiently similar that we probably jut want

template <typename T> class CanvasArray {
...
};
class CanvasByteArray : CanvasArray<uint8> { ... }
class CanvasNumberArray : CanvasArray<float> { ... }
> +
> +        /* ClearBufferMask */
> +        const unsigned int GL_DEPTH_BUFFER_BIT               = 0x00000100;
...
> +        const unsigned int GL_MAX_RENDERBUFFER_SIZE          = 0x84E8;
I thought that we had decided that most of these constants would not be present as they are unused and unnecessary?

> +        void         glActiveTexture (in unsigned long texture);
Textures aren't identified by numeric idents so why is this here? Also why the gl prefixes this doesn't seem to match the spec

> +}
> +
> +-(void)drawInCGLContext:(CGLContextObj)glContext pixelFormat:(CGLPixelFormatObj)pixelFormat forLayerTime:(CFTimeInterval)timeInterval displayTime:(const CVTimeStamp *)timeStamp
> +{
> +    CGRect frame = [self frame];
> +        
> +    // draw the FBO into the layer
> +    glViewport(0, 0, frame.size.width, frame.size.height);
> +    glMatrixMode(GL_PROJECTION);
> +    glLoadIdentity();
> +    glOrtho(-1, 1, -1, 1, -1, 1);
> +    glMatrixMode(GL_MODELVIEW);
> +    glLoadIdentity();
> +
> +    glEnable(GL_TEXTURE_2D);
> +    glBindTexture(GL_TEXTURE_2D, m_texture);
> +    
> +	glBegin(GL_TRIANGLE_FAN);
> +        glTexCoord2f(0, 0);
> +		glVertex2f(-1, -1);
> +        glTexCoord2f(1, 0);
> +		glVertex2f(1, -1);
> +        glTexCoord2f(1, 1);
> +		glVertex2f(1, 1);
> +        glTexCoord2f(0, 1);
> +		glVertex2f(-1, 1);
> +	glEnd();
> +    
> +    glBindTexture(GL_TEXTURE_2D, 0);
> +    glDisable(GL_TEXTURE_2D);
We draw the fbo with a triangle fan? say what?

> Index: WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
> ===================================================================
> --- WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp	(revision 0)
> +++ WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp	(revision 0)
> @@ -0,0 +1,1570 @@
> +/*
> + * Copyright (C) 2003, 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
2009

There are plenty of C-style casts in the remaining code and uses of malloc/free instead of fastMalloc/fastFree

[Simon Fraser (smfr)]

Comment on attachment 34149 [details]
Patch 3 of 4: Adding new files to the build

I'd prefer that we make html/canvas and put all the new files there, and make new groupings in the Xcode project.

[Simon Fraser (smfr)]

Comment on attachment 34151 [details]
Patch 4 of 4: Connect everything up

Minusing for the recalcStyle() hack that we need to figure out a better way to do.

[Dimitri Glazkov (Google)]

Comment on attachment 34151 [details]
Patch 4 of 4: Connect everything up


> +        [Custom,V8Custom] DOMObject getCSSCanvasContext(in DOMString contextId, in DOMString name, in long width, in long height);

This can be just [Custom]

[Dimitri Glazkov (Google)]

Comment on attachment 34146 [details]
Patch 2 of 4: New Canvas 3D files

Whoa, this is a chunky patch. Can we break it up into smaller pieces perhaps?

> +    /*
> +     ec = 0;
> +     
> +     if (!canvas)
> +     return;
> +     
> +     ensureContext;
> +     ImageBuffer* buffer = canvas->buffer();
> +     if (!buffer)
> +     return;
> +     
> +     Image* img = buffer->image();
> +     if (!img)
> +     return;
> +     
> +     ::glTexSubImage2D(target, level, xoff, yoff, width, height, GL_RGBA, GL_UNSIGNED_BYTE, img->data()->data());
> +      */
> +}
> +
> +}
> +
> +#endif // ENABLE(3D_CANVAS)

Probably didn't mean to leave this commented code in.

[Dimitri Glazkov (Google)]

BTW, really exciting stuff!

[Sam Weinig]

Comment on attachment 34149 [details]
Patch 3 of 4: Adding new files to the build

RE: patch 3.  What about the rest of the build systems?

[Simon Fraser (smfr)]

Comment on attachment 34146 [details]
Patch 2 of 4: New Canvas 3D files

My feedback:

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 46804)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,407 @@
> [phone]  Chris Marrin  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        All of the new Canvas 3D files
> +        https://bugs.webkit.org/show_bug.cgi?id=28018
> +
> +        Patch 2 of 4. This is just the new files. They are not compiled yet

You should edit down the changelog to just show each new file, and add a brief description of what each file does.

You should do an initial patch that moves all existing canvas files to html/canvas, and then add your new files there.

> Index: WebCore/bindings/js/JSCanvasNumberArrayCustom.cpp
> ===================================================================

Wrong license. This should be the 2-clause license with a date of 2009. This also applies to all the other new files.


> Index: WebCore/bindings/js/JSCanvasRenderingContext3DCustom.cpp
> ===================================================================

> +JSValue JSCanvasRenderingContext3D::glBufferData(JSC::ExecState* exec, JSC::ArgList const& args)
> +{
> +    unsigned target = args.at(0).toInt32(exec);
> +    unsigned usage = args.at(2).toInt32(exec);

Should you check the number of args (here and elsewhere)?

> +JSValue JSCanvasRenderingContext3D::glUniformMatrix(JSC::ExecState* exec, JSC::ArgList const& args)
> +{
> +    unsigned location = args.at(0).toInt32(exec);
> +    unsigned count = args.at(1).toInt32(exec);
> +    bool transpose = args.at(2).toBoolean(exec);
> +    
> +    if (!args.at(3).isObject())
> +        return jsUndefined();
> +        
> +    WebKitCSSMatrix* matrix = toWebKitCSSMatrix(args.at(3));
> +    if (matrix)
> +        impl()->glUniformMatrix(location, transpose, matrix);
> +    else {
> +        JSObject* array = asObject(args.at(3));
> +        int length = array->get(exec, Identifier(exec, "length")).toInt32(exec);
> +        if (length) {
> +            // If the first element is a WebKitCSSMatrix, assume all are

Won't this crash if this assumption is false?

> +JSValue JSCanvasRenderingContext3D::glUniformf(JSC::ExecState* exec, JSC::ArgList const& args)

> +                impl()->glUniform(location, (float) args.at(1).toNumber(exec));

Use static_cast<>, no space after )

> +JSValue JSCanvasRenderingContext3D::glDrawElements(JSC::ExecState* exec, JSC::ArgList const& args)
> +{
> +    unsigned mode = args.at(0).toInt32(exec);
> +    unsigned type = args.at(1).toInt32(exec);
> +    
> +    unsigned int count = 0;
> +    
> +    // If the third param is not an object, it is a number, which is the count.
> +    // In this case if there is a 4th param, it is the offset. If there is no
> +    // 4th param, the offset is 0

Do these assumptions needs to be checked in the code?

> +    if (!args.at(2).isObject()) {
> +        count = args.at(2).toInt32(exec);
> +        unsigned int offset = (args.size() > 3) ? args.at(3).toInt32(exec) : 0;
> +        impl()->glDrawElements(mode, count, type, (void*) offset);
> +    } else {
> +        void* passedIndices = 0;
> +        JSObject* array = asObject(args.at(2));
> +        count = array->get(exec, Identifier(exec, "length")).toInt32(exec);
> +
> +        if (type == GL_UNSIGNED_BYTE) {
> +            unsigned char* indices = (unsigned char*) malloc(count * sizeof(unsigned short));

Why is this sizeof(unsigned short)?

> Index: WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp

> +    JSValue result = jsUndefined();
> +    
> +    if (contextId == "2d")
> +        result = toJS(exec, WTF::getPtr(reinterpret_cast<CanvasRenderingContext2D*>(imp->getContext(contextId))));
> +#if ENABLE(3D_CANVAS)
> +    else if (contextId == "webkit-3d")
> +        result = toJS(exec, WTF::getPtr(reinterpret_cast<CanvasRenderingContext3D*>(imp->getContext(contextId))));
> +#endif
> +    
> +    return result;

This would be somewhat cleaner with early returns.

> Index: WebCore/html/CanvasBuffer.cpp
> ===================================================================


> +    CanvasBuffer::CanvasBuffer(GraphicsContext3D* context)
> +    : CanvasObject(context)
> +    {
> +        GLuint o;
> +        glGenBuffers(1, &o);
> +        setObject(o);
> +    }
> +    
> +    void CanvasBuffer::_deleteObject(Platform3DObject object)
> +    {
> +        glDeleteBuffers(1, &object);
> +    }

Should the '1' be a named constant?

> Index: WebCore/html/CanvasByteArray.cpp
> ===================================================================

> +    String CanvasByteArray::toString() const
> +    {
> +        String s = "[ ";
> +        for (size_t i = 0; i < m_data.size(); ++i) {
> +            if (i != 0)
> +                s += ", ";
> +            s += String::number(m_data[i]);
> +        }
> +        
> +        s += " ]";
> +        return s;

Lots of very inefficient string appends here. Should use append().

> Index: WebCore/html/CanvasByteArray.h
> ===================================================================

> +        Vector<uint8_t>& data() { return m_data; }

Is it ok that this is mutable?

> Index: WebCore/html/CanvasFramebuffer.cpp
> ===================================================================

> +#include <OpenGL/OpenGL.h>

It seems wrong for code outside of platform/ to be including OpenGL.h. This code needs to be platform-neutral.

> Index: WebCore/html/CanvasNumberArray.cpp
> ===================================================================

> +    String CanvasNumberArray::toString() const
> +    {
> +        String s = "[ ";
> +        for (size_t i = 0; i < m_data.size(); ++i) {
> +            if (i != 0)
> +                s += ", ";
> +            s += String::number(m_data[i]);
> +        }
> +        
> +        s += " ]";

Should use append here too.

> Index: WebCore/html/CanvasNumberArray.h
> ===================================================================

> +    class CanvasNumberArray : public RefCounted<CanvasNumberArray> {
> +    public:
> +        static PassRefPtr<CanvasNumberArray> create(unsigned length);
> +        
> +        Vector<float>& data() { return m_data; }
> +
> +        unsigned length() const { return m_data.size(); }
> +        float item(unsigned index) { return (index >= m_data.size()) ? 0 : m_data[index]; }

Should be const.

> Index: WebCore/html/CanvasObject.cpp
> ===================================================================

> +    void CanvasObject::setObject(Platform3DObject object)
> +    {
> +        deleteObject();
> +        m_object = object;
> +    }

What if m_object == object?

> Index: WebCore/html/CanvasProgram.cpp
> ===================================================================

> +#include <OpenGL/OpenGL.h>

Bad layering.

> Index: WebCore/html/CanvasRenderbuffer.cpp
> ===================================================================

> +#include <OpenGL/OpenGL.h>

Bad layering.


> +    CanvasRenderbuffer::CanvasRenderbuffer(GraphicsContext3D* context)
> +    : CanvasObject(context)
> +    {
> +        GLuint o;
> +        glGenRenderbuffersEXT(1, &o);
> +        setObject(o);
> +    }
> +    
> +    void CanvasRenderbuffer::_deleteObject(Platform3DObject object)
> +    {
> +        glDeleteRenderbuffersEXT(1, &object);
> +    }

Should the '1' be a named constant?

> Index: WebCore/html/CanvasRenderingContext3D.cpp
> ===================================================================

> +void CanvasRenderingContext3D::updateContext()
> +{
> +    if (m_canvas->renderBox() && m_canvas->renderBox()->hasLayer())
> +        m_canvas->renderBox()->layer()->rendererContentChanged();
> +}

What does "update" really mean?

> +#define leave(update) { m_context.checkError(); if (update) updateContext(); }

Macros should be in uppercase. Use an inline function instead? 'leave' is a really bad name.
Also, boolean arguments are evil. Make 2 inline functions.

> +void CanvasRenderingContext3D::glReleaseShaderCompiler ()
> +{
> +    //::glReleaseShaderCompilerEXT();
> +    leave(false);
> +}

Why is this commented out?

> +PassRefPtr<CanvasProgram> CanvasRenderingContext3D::createProgram ()

Extra space after the function name (here and in many other places).

> Index: WebCore/html/CanvasRenderingContext3D.h
> ===================================================================

> +        virtual bool is3d() { return true; }

Should be const.

> +        void glActiveTexture (unsigned long texture);
> +        void glAttachShader (CanvasProgram* program, CanvasShader* shader);

Need to fix the spacing with all of these. In some cases the parameter names can be removed

> Index: WebCore/html/CanvasRenderingContext3D.idl
> ===================================================================

> +        readonly attribute HTMLCanvasElement canvas;
> +        
> +        void         glActiveTexture (in unsigned long texture);

Need to fix the spacing with all these methods declarations.

> Index: WebCore/html/CanvasRenderingContext.h
> ===================================================================

> +namespace WebCore {
> +
> +    class CanvasObject;
> +    class HTMLCanvasElement;
> +
> +    class CanvasRenderingContext : Noncopyable {
> +    public:
> +        CanvasRenderingContext(HTMLCanvasElement*);
> +        virtual ~CanvasRenderingContext() { }
> +        
> +        void ref();
> +        void deref();
> +        
> +        HTMLCanvasElement* canvas() const { return m_canvas; }
> +        
> +        virtual bool is2d() { return false; }
> +        virtual bool is3d() { return false; }
> +        virtual bool is3d_1_1() { return false; }
> +        virtual bool is3d_fixed() { return false; }

These should all be const.

> Index: WebCore/html/CanvasShader.cpp
> ===================================================================

> +#include <OpenGL/OpenGL.h>

Layering violation.

> Index: WebCore/html/CanvasTexture.cpp
> ===================================================================

> +#include <OpenGL/OpenGL.h>
Ditto.

> +    CanvasTexture::CanvasTexture(GraphicsContext3D* context)
> +    : CanvasObject(context)
> +    {
> +        GLuint o;
> +        glGenTextures(1, &o);
> +        setObject(o);
> +    }
> +    
> +    void CanvasTexture::_deleteObject(Platform3DObject object)
> +    {
> +        glDeleteTextures(1, &object);
> +    }

Should the '1' be a named constant?

> Index: WebCore/platform/graphics/GraphicsContext3D.cpp
> ===================================================================

> +void  GraphicsContext3D::removeObject(CanvasObject* object)

Extra space after the void.

> +{
> +    for (size_t i = 0; i < m_canvasObjects.size(); ++i)
> +        if (m_canvasObjects[i] == object) {
> +            m_canvasObjects.remove(i);
> +            return;

Is this linear search going to be a performance problem?
Is there a guarantee that the object will only be in the list once? Maybe use a HashSet?

> Index: WebCore/platform/graphics/GraphicsContext3D.h
> ===================================================================

> +    class GraphicsContext3D : Noncopyable {
> +    public:
> +        GraphicsContext3D();
> +        virtual ~GraphicsContext3D();
> +       
> +        PlatformGraphicsContext3D platformGraphicsContext3D() const;
> +        
> +        void checkError() const;
> +        
> +        void glActiveTexture (unsigned long texture);
> +        void glAttachShader (CanvasProgram* program, CanvasShader* shader);

Spacing and parameter names need fixing here too.

> Index: WebCore/platform/graphics/mac/Canvas3DLayer.h
> ===================================================================

> +-(id) initWithContext:(CGLContextObj) context texture:(GLuint) texture;

No spaces after the ) here.

> Index: WebCore/platform/graphics/mac/Canvas3DLayer.mm
> ===================================================================


> +@implementation Canvas3DLayer
> +
> +-(id) initWithContext:(CGLContextObj) context texture:(GLuint) texture

No spaces after )

> +{
> +    m_contextObj = context;
> +    m_texture = texture;
> +    [super init];
> +    return self;
> +}

The canonical form is:

if ((self = [super init])) {
  // do own init here
}
return self;

> +-(void)drawInCGLContext:(CGLContextObj)glContext pixelFormat:(CGLPixelFormatObj)pixelFormat forLayerTime:(CFTimeInterval)timeInterval displayTime:(const CVTimeStamp *)timeStamp
> +{
> +    CGRect frame = [self frame];
> +        
> +    // draw the FBO into the layer
> +    glViewport(0, 0, frame.size.width, frame.size.height);
> +    glMatrixMode(GL_PROJECTION);
> +    glLoadIdentity();
> +    glOrtho(-1, 1, -1, 1, -1, 1);
> +    glMatrixMode(GL_MODELVIEW);
> +    glLoadIdentity();
> +
> +    glEnable(GL_TEXTURE_2D);
> +    glBindTexture(GL_TEXTURE_2D, m_texture);
> +    
> +	glBegin(GL_TRIANGLE_FAN);
> +        glTexCoord2f(0, 0);
> +		glVertex2f(-1, -1);
> +        glTexCoord2f(1, 0);
> +		glVertex2f(1, -1);
> +        glTexCoord2f(1, 1);
> +		glVertex2f(1, 1);
> +        glTexCoord2f(0, 1);
> +		glVertex2f(-1, 1);
> +	glEnd();

You have tabs here.

> +    glBindTexture(GL_TEXTURE_2D, 0);
> +    glDisable(GL_TEXTURE_2D);
> +
> +	// Call super to finalize the drawing. By default all it does is call glFlush().
> +	[super drawInCGLContext:glContext pixelFormat:pixelFormat forLayerTime:timeInterval displayTime:timeStamp];

More tabs.

> Index: WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
> ===================================================================


> +void GraphicsContext3D::checkError() const
> +{
> +    GLenum error = ::glGetError();
> +    if (error != GL_NO_ERROR)
> +        fprintf(stderr, "*** GL error %d\n", error);
> +}

Shouldn't these propagate back to Console?

> +#define ensureContext _ensureContext(m_contextObj)

Make the macro uppercase, or, preferably, just use an inline method.
WebKit macros are normally written to look like functions, i.e. with parens.

> +void  GraphicsContext3D::glAttachShader (CanvasProgram* program, CanvasShader* shader)

Spacing needs cleanup.

> +void  GraphicsContext3D::glClearColor(double r, double g, double b, double a)
> +{
> +    ensureContext;
> +    if (isnan(r))
> +        r = 0;
> +    if (isnan(g))
> +        g = 0;
> +    if (isnan(b))
> +        b = 0;
> +    if (isnan(a))
> +        a = 1;

It seems like NANs should never get down to GraphicsContext3D; the upper layers should do this conversion.

> +    ::glClearColor((float) r, (float) g, (float) b, (float) a);

static_cast please.

> +void  GraphicsContext3D::glFinish ()
> +{
> +    ensureContext;
> +    ::glFinish();
> +}
> +
> +

Extra blank line.

> +void  GraphicsContext3D::glReleaseShaderCompiler ()
> +{
> +    ensureContext;
> +    //::glReleaseShaderCompilerEXT();

Why is this commented out? If it's a feature that is not yet implemented, then maybe use an #ifdef?


> +void  GraphicsContext3D::glUniformMatrix(long location, long count, bool transpose, CanvasNumberArray*array)

Space after the *

> +    int n = array->data().size() / count;
> +    if (n < 2)
> +        return;
> +    
> +    if (n > 4)
> +        n = 4;

Should these be asserting?

> +void  GraphicsContext3D::glUniformMatrix(long location, bool transpose, const Vector<WebKitCSSMatrix*>& array)
> +{
> +    int count = array.size();
> +    float* floats = (float*) malloc(count * 16 * sizeof(float));
> +    
> +    for (int i = 0; i < count; ++i) {
> +        floats[i*16+ 0] = (float) array[i]->m11();

static_cast

> +void  GraphicsContext3D::glUniformMatrix(long location, bool transpose, const WebKitCSSMatrix* matrix)
> +{

> +    floats[ 0] = (float) matrix->m11();

Ditto

> +void  GraphicsContext3D::glViewport(long x, long y, unsigned long width, unsigned long height)
> +{
> +    ensureContext;
> +    if (isnan(x))
> +        x = 0;
> +    if (isnan(y))
> +        y = 0;
> +    if (isnan(width))
> +        width = 100;
> +    if (isnan(height))
> +        height = 100;

Again, seems like NANs should be filtered out higher up. Arbitrary values (like 100) should not live in platform code.


> +// Assumes the texture you want to go into is bound
> +static void imageToTexture(Image* image, unsigned target, unsigned level)
> +{
> +    if (!image)
> +        return;
> +    
> +    CGImageRef textureImage = image->getCGImageRef();
> +    if (!textureImage)
> +        return;
> +    
> +    size_t textureWidth = CGImageGetWidth(textureImage);
> +    size_t textureHeight = CGImageGetHeight(textureImage);
> +    
> +    GLubyte* textureData = (GLubyte*) malloc(textureWidth*textureHeight*4);
> +    CGContextRef textureContext = CGBitmapContextCreate(textureData, textureWidth, textureHeight, 8, textureWidth*4, 
> +                                                        CGImageGetColorSpace(textureImage), kCGImageAlphaPremultipliedLast);
> +    

Spaces around the * please.

[Simon Fraser (smfr)]

Comment on attachment 34151 [details]
Patch 4 of 4: Connect everything up

> Index: WebCore/ChangeLog
> ===================================================================

> +        Connect everything up
> +        https://bugs.webkit.org/show_bug.cgi?id=28018
> +
> +        Changes to CanvasElement, GraphicsLayer, the style logic and
> +        rendering logic for Canvas 3D.

I'd like to see more detail here. At least annotate the change list to describe
the substantive changes to each file.

> Index: WebCore/bindings/js/JSDocumentCustom.cpp
> ===================================================================

> +JSValue JSDocument::getCSSCanvasContext(JSC::ExecState* exec, JSC::ArgList const& args)

Why did you have to add this function?

> +{
> +    const UString& contextId = args.at(0).toString(exec);
> +    const UString& name = args.at(1).toString(exec);
> +    int width = args.at(2).toInt32(exec);
> +    int height = args.at(3).toInt32(exec);

Check the argument count?

> +    if (contextId == "2d")
> +        result = toJS(exec, WTF::getPtr(reinterpret_cast<CanvasRenderingContext2D*>(impl()->getCSSCanvasContext(contextId, name, width, height))));
> +#if ENABLE(3D_CANVAS)
> +    else if (contextId == "webkit-3d")
> +        result = toJS(exec, WTF::getPtr(reinterpret_cast<CanvasRenderingContext3D*>(impl()->getCSSCanvasContext(contextId, name, width, height))));
> +#endif
> +    
> +    return result;

Early returns would be clearer.

> Index: WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp
> ===================================================================

> + * Copyright (C) 2007 Apple Inc. All rights reserved.

Date needs fixing.

> +JSValue JSHTMLCanvasElement::getContext(JSC::ExecState* exec, JSC::ArgList const& args)
> +{
> +    HTMLCanvasElement* imp = static_cast<HTMLCanvasElement*>(impl());
> +    const UString& contextId = args.at(0).toString(exec);
> +
> +    JSValue result = jsUndefined();
> +    
> +    if (contextId == "2d")
> +        result = toJS(exec, WTF::getPtr(reinterpret_cast<CanvasRenderingContext2D*>(imp->getContext(contextId))));
> +#if ENABLE(3D_CANVAS)
> +    else if (contextId == "webkit-3d")
> +        result = toJS(exec, WTF::getPtr(reinterpret_cast<CanvasRenderingContext3D*>(imp->getContext(contextId))));
> +#endif
> +    
> +    return result;

Again, prefer early returns.

> Index: WebCore/dom/Document.h
> ===================================================================

> +    CanvasRenderingContext* getCSSCanvasContext(const String& type, const String& name, int width, int height);

Wow, so, after your changes, can I reference a canvas3D in CSS as background image?

> Index: WebCore/dom/Element.cpp
> ===================================================================
> --- WebCore/dom/Element.cpp	(revision 46800)
> +++ WebCore/dom/Element.cpp	(working copy)
> @@ -821,6 +821,11 @@ void Element::recalcStyle(StyleChange ch
>      if (hasParentStyle && (change >= Inherit || needsStyleRecalc())) {
>          RefPtr<RenderStyle> newStyle = document()->styleSelector()->styleForElement(this);
>          StyleChange ch = diff(currentStyle, newStyle.get());
> +
> +        // We need to force a set style if we have a 3D canvas without a compositing layer
> +        if (renderer() && renderer()->isCanvas3D() && !renderer()->hasLayer())
> +            ch = Force;
> +

We need to do this differently. There's no guarantee that recalcStyle will happen after you
request a canvas-3d context.

We should talk to hyatt about how best to make a layer for a canvas as soon as the 3d context is requested.

> Index: WebCore/html/CanvasRenderingContext2D.h
> ===================================================================

> +        virtual bool is2d() { return true; }

Should be const

> Index: WebCore/html/HTMLCanvasElement.cpp
> ===================================================================

>  CanvasRenderingContext* HTMLCanvasElement::getContext(const String& type)
>  {
>      if (type == "2d") {
> -        if (!m_2DContext)
> -            m_2DContext.set(new CanvasRenderingContext2D(this));
> -        return m_2DContext.get();
> +        if (m_context && !m_context->is2d())
> +            m_context.clear();
> +        if (!m_context)
> +            m_context.set(new CanvasRenderingContext2D(this));
>      }
> -    return 0;
> +#if ENABLE(3D_CANVAS)    
> +    else if (type == "webkit-3d") {
> +        if (m_context && !m_context->is3d())
> +            m_context.clear();
> +        if (!m_context) {
> +            m_context.set(new CanvasRenderingContext3D(this));
> +            setNeedsStyleRecalc();
> +        }
> +    }
> +#endif
> +    else if (m_context.get())
> +        m_context.clear();
> +    
> +    return m_context.get();

I think this needs some comments explaining the behavior when the author requests
both 2d and 3d rendering contexts on the same canvas.

> +bool HTMLCanvasElement::is3D() const
> +{
> +    CanvasRenderingContext* ctx = m_context.get();
> +    return ctx && !ctx->is2d();
> +}

Is the entire canvas really in 3d mode, or can I mix and match?

> +PlatformGraphicsContext3D HTMLCanvasElement::context3D() const
> +{
> +    if (m_context && m_context->is3d())
> +        return static_cast<CanvasRenderingContext3D*>(m_context.get())->context3D();
> +    else
> +        return 0;

No 'else' after 'return'.

> +Platform3DObject HTMLCanvasElement::texture3D() const
> +{
> +    if (m_context && m_context->is3d())
> +        return static_cast<CanvasRenderingContext3D*>(m_context.get())->texture3D();
> +    else
> +        return 0;

Ditto.

> Index: WebCore/page/DOMWindow.idl
> ===================================================================
> --- WebCore/page/DOMWindow.idl	(revision 46800)
> +++ WebCore/page/DOMWindow.idl	(working copy)
> @@ -406,6 +406,7 @@ module window {
>          attribute [CustomGetter] HTMLOptionElementConstructor Option; // Usable with new operator
>  
>          attribute CanvasRenderingContext2DConstructor CanvasRenderingContext2D;
> +        attribute [Conditional=3D_CANVAS] CanvasRenderingContext3DConstructor CanvasRenderingContext3D;
>          attribute TextMetricsConstructor TextMetrics;

This change is going to affect layout tests I think.

> Index: WebCore/platform/graphics/GraphicsLayer.h
> ===================================================================

> +#if PLATFORM(MAC)
> +//#include <OpenGL/OpenGL.h>
> +#endif

Do you need this?

> +#if ENABLE(3D_CANVAS)
> +    virtual void setContentsToCanvas3D(PlatformLayer*) { }
> +    virtual void redrawCanvas3D() { }

Do you really mean redraw, or is it something else?

> +#if ENABLE(3D_CANVAS)
> +    void setHas3DLayer(bool has3DLayer, PlatformGraphicsContext3D context, Platform3DObject texture);
> +#endif

I'm not sure why you need both this and setContentsToCanvas3D()? Also, I think the last two params
should have default arguments so you can call setHas3DLayer(false).

> +#if ENABLE(3D_CANVAS)
> +    bool m_3d : 1;

I don't like the name of this member variable; it's really not clear what it means.

Also, does it belong in the base class? Using GraphicsLayer for a canvas-3d might be a Mac-specific thing.

> Index: WebCore/platform/graphics/mac/GraphicsLayerCA.mm
> ===================================================================

> +#if ENABLE(3D_CANVAS)
> +void GraphicsLayerCA::setContentsToCanvas3D(PlatformLayer* layer)
> +{
> +
> +    
> +    

Lots of whitespace there.

> +#if ENABLE(3D_CANVAS)
> +void GraphicsLayer::setHas3DLayer(bool has3DLayer, PlatformGraphicsContext3D context, Platform3DObject texture)
> +{
> +    BEGIN_BLOCK_OBJC_EXCEPTIONS
> +
> +    if (has3DLayer && !m_3d) {
> +        // create the inner 3d layer
> +        PlatformLayer* layer = [[Canvas3DLayer alloc] initWithContext:(CGLContextObj) context texture: (GLuint) texture];

No spaces after the )

> +        [layer setName:@"3D Layer"];

Names should only be set in debug.

> +void GraphicsLayerCA::redrawCanvas3D()
> +{
> +    if (m_3d)
> +        [m_contentsLayer.get() setNeedsDisplay];
> +}

This needs to go through the batching mechanism.

> Index: WebCore/rendering/RenderBoxModelObject.h
> ===================================================================
> --- WebCore/rendering/RenderBoxModelObject.h	(revision 46800)
> +++ WebCore/rendering/RenderBoxModelObject.h	(working copy)
> @@ -59,7 +59,14 @@ public:
>  
>      bool hasSelfPaintingLayer() const;
>      RenderLayer* layer() const { return m_layer; }
> -    virtual bool requiresLayer() const { return isRoot() || isPositioned() || isRelPositioned() || isTransparent() || hasOverflowClip() || hasTransform() || hasMask() || hasReflection(); }
> +    virtual bool requiresLayer() const
> +    {
> +        return isRoot() || isPositioned() || isRelPositioned() || isTransparent() || hasOverflowClip() || hasTransform() || hasMask() || hasReflection()
> +#if USE(ACCELERATED_COMPOSITING)
> +                || isCanvas3D()
> +#endif

FYI, you've added a virtual method call to this somewhat hot method; all the other tests are inlined.

> Index: WebCore/rendering/RenderLayerBacking.cpp
> ===================================================================

> +        else if (renderer()->isCanvas3D()) {
> +            m_hasDirectlyCompositedContent = true;
> +            HTMLCanvasElement* canvas = reinterpret_cast<HTMLCanvasElement*>(renderer()->node());
> +            if (canvas && canvas->context3D()) {
> +                m_graphicsLayer->setHas3DLayer(true, canvas->context3D(), canvas->texture3D());
> +                m_graphicsLayer->setDrawsContent(true);

I'm not sure you need the setDrawsContent(true) since you don't want normal backing store for the layer.

[Kenneth Russell]

Glad to see this work landing.

The current WebGL spec uses CanvasRenderingContextGL as the JavaScript interface name. To avoid future file renamings, which are problematic with SVN, should CanvasRenderingContext3D and GraphicsContext3D be renamed to CanvasRenderingContextGL and GraphicsContextGL before initial checkin?

Also, the UUIDs for many of the new IDL files are duplicates of those from CanvasRenderingContext2D. I don't know whether the COM bindings are still used, but these bogus UUIDs should either be removed or replaced with real ones generated by a tool like http://www.famkruithof.net/uuid/uuidgen .

[Mark Rowe (bdash)]

(In reply to comment #15)
> Glad to see this work landing.
> 
> The current WebGL spec uses CanvasRenderingContextGL as the JavaScript
> interface name. To avoid future file renamings, which are problematic with SVN,
> should CanvasRenderingContext3D and GraphicsContext3D be renamed to
> CanvasRenderingContextGL and GraphicsContextGL before initial checkin?

File renaming isn't a problem in SVN.  We do it relatively frequently.

[Kenneth Russell]

(In reply to comment #16)
> (In reply to comment #15)
> > Glad to see this work landing.
> > 
> > The current WebGL spec uses CanvasRenderingContextGL as the JavaScript
> > interface name. To avoid future file renamings, which are problematic with SVN,
> > should CanvasRenderingContext3D and GraphicsContext3D be renamed to
> > CanvasRenderingContextGL and GraphicsContextGL before initial checkin?
> 
> File renaming isn't a problem in SVN.  We do it relatively frequently.

Doesn't it require an intermediate checkin of the renamed file before changing the contents under the new name? That was my experience with Java sources and it was a real pain because the build would be broken for a period of time.

[Chris Marrin]

Landed the first patch (Add ENABLE_3D_CANVAS flag to the build)

Sending        WebCore/ChangeLog
Sending        WebCore/Configurations/FeatureDefines.xcconfig
Sending        WebKit/mac/ChangeLog
Sending        WebKit/mac/Configurations/FeatureDefines.xcconfig
Sending        WebKitTools/ChangeLog
Sending        WebKitTools/Scripts/build-webkit
Transmitting file data ......
Committed revision 46844.

[Chris Marrin]

Created attachment 38385 [details]
Patch for new Canvas3D files

This patch incorporates comments from Simon and Oliver. Note that this is not all the new files. I've left a few of the JS binding files out since they will require more rework, and this gives a more manageable set of files to review.

[Simon Fraser (smfr)]

Comment on attachment 38385 [details]
Patch for new Canvas3D files

The new files should be in html/canvas.

[Simon Fraser (smfr)]

Comment on attachment 38385 [details]
Patch for new Canvas3D files

New files should use the 2-clause license, e.g. like WebKitCSSKeyframeRule

[Oliver Hunt]

Comment on attachment 38385 [details]
Patch for new Canvas3D files

Files are in the wrong directory :D

Minor style gripe: CanvasObject::CanvasObject -> the member constructors should be indented

GraphicsContext3D::checkError -> I'd prefer notImplemented() in place of what's currently there

GraphicsContext3D::reshape -> ASSERT(0) should be ASSERT_NOT_REACHED() or notImplemented()

GraphicsContext3D::texSubImage2D -> We don't check in commented out code

Other than these gripes, r=me -- when landed file a bug on updating to current API

[Chris Marrin]

Sending        WebCore/ChangeLog
Adding         WebCore/bindings/js/JSCanvasNumberArrayCustom.cpp
Adding         WebCore/html/canvas/CanvasObject.cpp
Adding         WebCore/html/canvas/CanvasObject.h
Adding         WebCore/html/canvas/CanvasRenderingContext.cpp
Adding         WebCore/html/canvas/CanvasRenderingContext.h
Adding         WebCore/html/canvas/CanvasRenderingContext.idl
Adding         WebCore/html/canvas/CanvasRenderingContext3D.cpp
Adding         WebCore/html/canvas/CanvasRenderingContext3D.h
Adding         WebCore/html/canvas/CanvasRenderingContext3D.idl
Adding         WebCore/platform/graphics/GraphicsContext3D.cpp
Adding         WebCore/platform/graphics/GraphicsContext3D.h
Adding         WebCore/platform/graphics/mac/Canvas3DLayer.h
Adding         WebCore/platform/graphics/mac/Canvas3DLayer.mm
Adding         WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
Transmitting file data ...............
Committed revision 47645.

[Chris Marrin]

Comment on attachment 34145 [details]
Patch 1 of 4: Add ENABLE_3D_CANVAS flag to the build

Landed this patch

[Chris Marrin]

Comment on attachment 38385 [details]
Patch for new Canvas3D files

Landed this patch

[Chris Marrin]

Created attachment 38426 [details]
Patch with new GL Object files

[Simon Fraser (smfr)]

Comment on attachment 38426 [details]
Patch with new GL Object files


> diff --git a/WebCore/html/canvas/CanvasObject.h b/WebCore/html/canvas/CanvasObject.h
> index 55cc525..d89264a 100644
> --- a/WebCore/html/canvas/CanvasObject.h
> +++ b/WebCore/html/canvas/CanvasObject.h
> @@ -52,6 +52,8 @@ namespace WebCore {
>      protected:
>          CanvasObject(GraphicsContext3D*);
>          virtual void _deleteObject(Platform3DObject) = 0;
> +        
> +        GraphicsContext3D* context() { return m_context; }

Should be |const|

The rest looks good.

[Chris Marrin]

Sending        WebCore/ChangeLog
Adding         WebCore/html/canvas/CanvasBuffer.cpp
Adding         WebCore/html/canvas/CanvasBuffer.h
Adding         WebCore/html/canvas/CanvasBuffer.idl
Adding         WebCore/html/canvas/CanvasFramebuffer.cpp
Adding         WebCore/html/canvas/CanvasFramebuffer.h
Adding         WebCore/html/canvas/CanvasFramebuffer.idl
Sending        WebCore/html/canvas/CanvasObject.h
Adding         WebCore/html/canvas/CanvasProgram.cpp
Adding         WebCore/html/canvas/CanvasProgram.h
Adding         WebCore/html/canvas/CanvasProgram.idl
Adding         WebCore/html/canvas/CanvasRenderbuffer.cpp
Adding         WebCore/html/canvas/CanvasRenderbuffer.h
Adding         WebCore/html/canvas/CanvasRenderbuffer.idl
Sending        WebCore/html/canvas/CanvasRenderingContext3D.cpp
Sending        WebCore/html/canvas/CanvasRenderingContext3D.h
Adding         WebCore/html/canvas/CanvasShader.cpp
Adding         WebCore/html/canvas/CanvasShader.h
Adding         WebCore/html/canvas/CanvasShader.idl
Adding         WebCore/html/canvas/CanvasTexture.cpp
Adding         WebCore/html/canvas/CanvasTexture.h
Adding         WebCore/html/canvas/CanvasTexture.idl
Deleting       WebCore/platform/graphics/GraphicsContext3D.cpp
Sending        WebCore/platform/graphics/GraphicsContext3D.h
Sending        WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
Transmitting file data ........................
Committed revision 47670.

[Chris Marrin]

Comment on attachment 38426 [details]
Patch with new GL Object files

Patch was landed

[Dimitri Glazkov (Google)]

Build fix landed as http://trac.webkit.org/changeset/47671. http://trac.webkit.org/changeset/47669 started using CanvasRenderingContext and it wasn't yet hooked up to build. It didn't fix everything, because CanvasRenderingContext is also typedef'd to CanvasRenderingContext2D in HTMLCanvasElement.h. Fixing more ...

[Sam Weinig]

Any reason the new files aren't in any of the project files?

[Chris Marrin]

Ack! Sorry. I was trying to commit using git svn and I obviously botched it. Those files should never have been committed. I will fix them right now

[Chris Marrin]

Created attachment 38460 [details]
Patch with remaining new files

[Simon Fraser (smfr)]

Comment on attachment 38460 [details]
Patch with remaining new files

> Index: WebCore/ChangeLog
> ===================================================================

> +        * bindings/js/JSHTMLCanvasElementCustom.cpp: Added Canvas3D method behind an ifdef
> +        * html/CanvasByteArray.cpp: Added. Efficient array of bytes for passing to GL functions
> +        * html/CanvasByteArray.h: Added.
> +        * html/CanvasByteArray.idl: Added.
> +        * html/CanvasNumberArray.cpp: Added. Efficient array of 32 bit floats for passing to GL functions
> +        * html/CanvasNumberArray.h: Added.
> +        * html/CanvasNumberArray.idl: Added.

New files are going to go in html/canvas, right?

> Index: WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp
> ===================================================================

> +#if ENABLE(3D_CANVAS)
> +JSValue JSHTMLCanvasElement::getContext(JSC::ExecState* exec, JSC::ArgList const& args)
> +{
> +    HTMLCanvasElement* imp = static_cast<HTMLCanvasElement*>(impl());
> +    const UString& contextId = args.at(0).toString(exec);
> +
> +    if (contextId == "2d")
> +        return toJS(exec, WTF::getPtr(reinterpret_cast<CanvasRenderingContext2D*>(imp->getContext(contextId))));
> +    else if (contextId == "webkit-3d")
> +        return toJS(exec, WTF::getPtr(reinterpret_cast<CanvasRenderingContext3D*>(imp->getContext(contextId))));

No 'else' after 'return'.

> Index: WebCore/html/CanvasByteArray.cpp
> ===================================================================

> + * Copyright (C) 2008, 2009 Apple Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.

Should be 2-clause license. Are the dates right? Same for other files.

> Index: WebCore/html/CanvasNumberArray.h
> ===================================================================

> +namespace WebCore {
> +
> +    class String;
> +    
> +    class CanvasNumberArray : public RefCounted<CanvasNumberArray> {
> +    public:
> +        static PassRefPtr<CanvasNumberArray> create(unsigned length);
> +        
> +        Vector<float>& data() { return m_data; }
> +
> +        unsigned length() const { return m_data.size(); }
> +        float item(unsigned index) { return (index >= m_data.size()) ? 0 : m_data[index]; }

This should be 'const'. Do you want a const data() accessor too?

Fix the above, and r=me

[Chris Marrin]

Sending        WebCore/ChangeLog
Sending        WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp
Adding         WebCore/html/canvas/CanvasByteArray.cpp
Adding         WebCore/html/canvas/CanvasByteArray.h
Adding         WebCore/html/canvas/CanvasByteArray.idl
Adding         WebCore/html/canvas/CanvasNumberArray.cpp
Adding         WebCore/html/canvas/CanvasNumberArray.h
Adding         WebCore/html/canvas/CanvasNumberArray.idl
Transmitting file data ........
Committed revision 47708.

[Chris Marrin]

Comment on attachment 38460 [details]
Patch with remaining new files

Landed this patch

[Chris Marrin]

Created attachment 38483 [details]
Patch to add Canvas3D files to build

Note that the 3D-Canvas flag is off by default, so most of this code is not yet used. But the addition of CanvasRenderingContext as the base class for CanvasRenderingContext2D is in all builds

[Simon Fraser (smfr)]

Comment on attachment 38483 [details]
Patch to add Canvas3D files to build

> Index: WebCore/bindings/js/JSDocumentCustom.cpp
> ===================================================================

> +    if (contextId == "2d")
> +        result = toJS(exec, WTF::getPtr(reinterpret_cast<CanvasRenderingContext2D*>(impl()->getCSSCanvasContext(contextId, name, width, height))));
> +#if ENABLE(3D_CANVAS)
> +    else if (contextId == "webkit-3d")
> +        result = toJS(exec, WTF::getPtr(reinterpret_cast<CanvasRenderingContext3D*>(impl()->getCSSCanvasContext(contextId, name, width, height))));
> +#endif
> +    
> +    return result;

No 'else' after 'return'.

> Index: WebCore/html/HTMLCanvasElement.cpp
> ===================================================================

> +#if ENABLE(3D_CANVAS)    
> +bool HTMLCanvasElement::is3D() const
> +{
> +    CanvasRenderingContext* ctx = m_context.get();
> +    return ctx && !ctx->is2d();

Why not ctx && ctx->is3d() ?

> +PlatformGraphicsContext3D HTMLCanvasElement::context3D() const
> +{
> +    if (m_context && m_context->is3d())
> +        return static_cast<CanvasRenderingContext3D*>(m_context.get())->context3D();
> +    else
> +        return 0;

No 'else' after 'return'.

> +Platform3DObject HTMLCanvasElement::texture3D() const
> +{
> +    if (m_context && m_context->is3d())
> +        return static_cast<CanvasRenderingContext3D*>(m_context.get())->texture3D();
> +    else
> +        return 0;

No 'else' after 'return'.

> Index: WebCore/html/HTMLCanvasElement.h
> ===================================================================

> -    CanvasRenderingContext2D* renderingContext2D() { return m_2DContext.get(); }
> +    CanvasRenderingContext* renderingContext() { return m_context.get(); }

Method should be 'const'.

[Oliver Hunt]

Comment on attachment 38483 [details]
Patch to add Canvas3D files to build


> +JSValue JSDocument::getCSSCanvasContext(JSC::ExecState* exec, JSC::ArgList const& args)
> +{
> +    const UString& contextId = args.at(0).toString(exec);
> +    const UString& name = args.at(1).toString(exec);
> +    int width = args.at(2).toInt32(exec);
> +    int height = args.at(3).toInt32(exec);
> +
> +    JSValue result = jsUndefined();
> +    
> +    if (contextId == "2d")
> +        result = toJS(exec, WTF::getPtr(reinterpret_cast<CanvasRenderingContext2D*>(impl()->getCSSCanvasContext(contextId, name, width, height))));
> +#if ENABLE(3D_CANVAS)
> +    else if (contextId == "webkit-3d")
> +        result = toJS(exec, WTF::getPtr(reinterpret_cast<CanvasRenderingContext3D*>(impl()->getCSSCanvasContext(contextId, name, width, height))));
> +#endif
> +    
> +    return result;
> +}
rather than result =...

i'd prefer return toJS ... return toJS ... return jsUndefined();

>  CanvasRenderingContext* HTMLCanvasElement::getContext(const String& type)
>  {
>      if (type == "2d") {
> -        if (!m_2DContext)
> -            m_2DContext.set(new CanvasRenderingContext2D(this));
> -        return m_2DContext.get();
> +        if (m_context && !m_context->is2d())
> +            m_context.clear();
> +            if (!m_context)
> +                m_context.set(new CanvasRenderingContext2D(this));
>      }
> -    return 0;
> +#if ENABLE(3D_CANVAS)    
> +    else if (type == "webkit-3d") {
> +        if (m_context && !m_context->is3d())
> +            m_context.clear();
> +        if (!m_context) {
> +            m_context.set(new CanvasRenderingContext3D(this));
> +            setNeedsStyleRecalc();
> +        }
> +    }
> +#endif
> +    else if (m_context.get())
> +        m_context.clear();
> +    
> +    return m_context.get();

What happens when you ask for an invalid context type (before this patch) is you get undefined back, but the context remains live.  After this patch the context is destroyed.  My reading of the spec says that this is incorrect.  A minor fix is to change 
> +    else if (m_context.get())
> +        m_context.clear();
to
> +    else
> +        return 0;

I feel better way would be to make the if (2d) and if (3d) paths return directly, then you can just drop all the else blocks, and return 0 at the end

> +PlatformGraphicsContext3D HTMLCanvasElement::context3D() const
> +{
> +    if (m_context && m_context->is3d())
> +        return static_cast<CanvasRenderingContext3D*>(m_context.get())->context3D();
> +    else
> +        return 0;
drop the else and return 0 directly.

> +}
> +
> +Platform3DObject HTMLCanvasElement::texture3D() const
> +{
> +    if (m_context && m_context->is3d())
> +        return static_cast<CanvasRenderingContext3D*>(m_context.get())->texture3D();
> +    else
> +        return 0;
> +}
ditto

I think simon had a few comments so if you address those as well, r=me

[Chris Marrin]

Created attachment 38551 [details]
Replacement patch to add Canvas3D files to build

[Chris Marrin]

Created attachment 38555 [details]
One more replacement patch to add Canvas3D files to build

[Chris Marrin]

Comment on attachment 38555 [details]
One more replacement patch to add Canvas3D files to build

This file adds project files for all the builds, to add CanvasRenderingContext

[Simon Fraser (smfr)]

Comment on attachment 38555 [details]
One more replacement patch to add Canvas3D files to build

>  CanvasRenderingContext* HTMLCanvasElement::getContext(const String& type)
>  {
> +    // A Canvas can either be "2D" or "3D" never both. If you request a 2D canvas and the existing
> +    // context is already 2D, just return that. If the existing context is 3D, then destroy it
> +    // before creating a new 2D context. Vice versa when requesting a 3D canvas. Requesting a
> +    // context with any other type string will destroy any existing context.
>      if (type == "2d") {
> -        if (!m_2DContext)
> -            m_2DContext.set(new CanvasRenderingContext2D(this));
> -        return m_2DContext.get();
> +        if (m_context && !m_context->is2d())
> +            m_context.clear();
> +            if (!m_context)
> +                m_context.set(new CanvasRenderingContext2D(this));
>      }
> -    return 0;
> +#if ENABLE(3D_CANVAS)    
> +    else if (type == "webkit-3d") {
> +        if (m_context && !m_context->is3d())
> +            m_context.clear();
> +        if (!m_context) {
> +            m_context.set(new CanvasRenderingContext3D(this));
> +            setNeedsStyleRecalc();
> +        }
> +    }
> +#endif

This logic seems wrong. I don't grok the if (!m_context) tests.

> @@ -192,8 +212,8 @@ void HTMLCanvasElement::reset()
>      bool hadImageBuffer = m_createdImageBuffer;
>      m_createdImageBuffer = false;
>      m_imageBuffer.clear();
> -    if (m_2DContext)
> -        m_2DContext->reset();
> +    if (m_context && m_context->is2d())
> +        static_cast<CanvasRenderingContext2D*>(m_context.get())->reset();

What about the 3d context? Is there a notion of 'reset' for 3d?


> -    CanvasRenderingContext2D* renderingContext2D() { return m_2DContext.get(); }
> +    CanvasRenderingContext* renderingContext() { return m_context.get(); }

Still missing the 'const'.

> Index: WebCore/html/canvas/CanvasRenderingContext.idl
> ===================================================================
> --- WebCore/html/canvas/CanvasRenderingContext.idl	(revision 47748)
> +++ WebCore/html/canvas/CanvasRenderingContext.idl	(working copy)
> @@ -27,12 +27,12 @@ module html {
>  
>      interface [
>          GenerateConstructor,
> +        CustomToJS,
>          InterfaceUUID=98fb48ae-7216-489c-862b-8e1217fc4443,
>          ImplementationUUID=ab4f0781-152f-450e-9546-5b3987491a54
>      ] CanvasRenderingContext {
>  
>          readonly attribute HTMLCanvasElement canvas;
> -        readonly attribute DOMString type;

Why did this go away?

[Simon Fraser (smfr)]

Comment on attachment 38555 [details]
One more replacement patch to add Canvas3D files to build

r=me with comments addressed

[Chris Marrin]

Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/JavaScriptCore.exp
Sending        LayoutTests/ChangeLog
Sending        LayoutTests/fast/dom/prototype-inheritance-2-expected.txt
Sending        WebCore/ChangeLog
Sending        WebCore/DerivedSources.cpp
Sending        WebCore/DerivedSources.make
Sending        WebCore/GNUmakefile.am
Sending        WebCore/WebCore.gypi
Sending        WebCore/WebCore.pro
Sending        WebCore/WebCore.vcproj/WebCore.vcproj
Sending        WebCore/WebCore.xcodeproj/project.pbxproj
Sending        WebCore/WebCoreSources.bkl
Sending        WebCore/bindings/js/JSCanvasRenderingContext2DCustom.cpp
Sending        WebCore/bindings/js/JSDOMBinding.cpp
Sending        WebCore/bindings/js/JSDocumentCustom.cpp
Sending        WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp
Sending        WebCore/dom/Document.cpp
Sending        WebCore/dom/Document.h
Sending        WebCore/html/HTMLCanvasElement.cpp
Sending        WebCore/html/HTMLCanvasElement.h
Sending        WebCore/html/canvas/CanvasObject.cpp
Sending        WebCore/html/canvas/CanvasRenderingContext.idl
Sending        WebCore/html/canvas/CanvasRenderingContext2D.cpp
Sending        WebCore/html/canvas/CanvasRenderingContext2D.h
Sending        WebCore/html/canvas/CanvasRenderingContext2D.idl
Sending        WebCore/html/canvas/CanvasRenderingContext3D.idl
Transmitting file data ...........................
Committed revision 47752.

[Chris Marrin]

Comment on attachment 38555 [details]
One more replacement patch to add Canvas3D files to build

Landed this patch

[Chris Marrin]

Created attachment 38576 [details]
Patch with Platform specific rendering changes for Canvas3D

[Chris Marrin]

Created attachment 38667 [details]
Final patch for Canvas3D work

[Simon Fraser (smfr)]

Comment on attachment 38667 [details]
Final patch for Canvas3D work

> Index: WebCore/ChangeLog
> ===================================================================

> +        Reviewed by NOBODY (OOPS!).
> +
> +        Final patch for Canvas 3D support
> +        https://bugs.webkit.org/show_bug.cgi?id=28018
> +
> +        This hooks everything up and provides a working implementation of
> +        Canvas 3D.

You need to explain a few things here, like the style recalc hack, the fact that a
3d canvas needs a RenderLayer and a compositing layer, and why.

The whole point of changelogs (and the commit message) is to explain why the
changes were made so that someone can come back later and figure out why you
made the changes you did. Often that person is the committer!

> Index: WebCore/bindings/js/JSCanvasRenderingContext3DCustom.cpp
> ===================================================================

> -        count = args.at(2).toInt32(exec);
> +        unsigned int count = args.at(2).toInt32(exec);

You are shadowing a variable at function scope here. A different name is probably better.

> Index: WebCore/html/HTMLCanvasElement.cpp
> ===================================================================

>          if (!m_context) {
>              m_context.set(new CanvasRenderingContext3D(this));
> -            setNeedsStyleRecalc();
> +            setNeedsStyleRecalc(SyntheticStyleChange);

You need a comment here explaining why you call setNeedsStyleRecalc().

> Index: WebCore/platform/graphics/GraphicsContext3D.h
> ===================================================================
>  
>  typedef void* PlatformGraphicsContext3D;
> +#define NO_PLATFORM_GRAPHICS_CONTEXT_3D 0
>  typedef GLuint Platform3DObject;
> +#define NO_PLATFORM_3D_OBJECT 0
>  #else
>  typedef void* PlatformGraphicsContext3D;
> +#define NO_PLATFORM_GRAPHICS_CONTEXT_3D 0
>  typedef int Platform3DObject;
> +#define NO_PLATFORM_3D_OBJECT 0
>  #endif

It seems that NO_PLATFORM_GRAPHICS_CONTEXT_3D and NO_PLATFORM_3D_OBJECT should
be constants, rather than #defines, like:

typedef void* PlatformGraphicsContext3D;
const PlatformGraphicsContext3D NullPlatformGraphicsContext3D = 0; 

typedef GLuint Platform3DObject;
const Platform3DObject NullPlatform3DObject = 0; 

> Index: WebCore/platform/graphics/GraphicsLayer.h
> ===================================================================
>  typedef void* PlatformLayer;
> -typedef void* NativeLayer;
> +typedef int NativeLayer;

Why this change? It's not 64-bit friendly.

> +#if ENABLE(3D_CANVAS)
> +    virtual void setContentsTo3DCanvas(PlatformGraphicsContext3D, Platform3DObject) { }

For setContentsTo3DCanvas(), the method names needs to match the arguments. You're not passing
in a canvas 3d, so perhaps it should be setContentsToTexture() or something. Can the argument
just be a GraphicsContext3D?

> +    virtual void redraw3DCanvas() { }

I don't like "redraw". What does it really mean? "recomposite"? Can setNeedsDisplay() do
the same job?

> +#if ENABLE(3D_CANVAS)
> +    void setHas3DLayer(bool has3DLayer, PlatformGraphicsContext3D context = NO_PLATFORM_GRAPHICS_CONTEXT_3D, Platform3DObject texture = NO_PLATFORM_3D_OBJECT);
> +#endif

Why do you need both this and the above method?

> Index: WebCore/platform/graphics/mac/GraphicsLayerCA.h
> ===================================================================

> +#if ENABLE(3D_CANVAS)
> +    void updateContents3DCanvas();
> +#endif

Maybe updateContentsTexture()?

> +#if ENABLE(3D_CANVAS)
> +    PlatformGraphicsContext3D m_3DContext;
> +    Platform3DObject m_3DTexture;
> +#endif

What's the ownership model for these members? Who is responsible for freeing the
associated resources?

> Index: WebCore/platform/graphics/mac/GraphicsLayerCA.mm
> ===================================================================

> +#if ENABLE(3D_CANVAS)
> +void GraphicsLayerCA::setContentsTo3DCanvas(PlatformGraphicsContext3D context, Platform3DObject texture)
> +{
> +    if (context == m_3DContext && texture == m_3DTexture)
> +        return;
> +        
> +    m_3DContext = context;
> +    m_3DTexture = texture;
> +    
> +    noteLayerPropertyChanged(ChildrenChanged);
> +    
> +    BEGIN_BLOCK_OBJC_EXCEPTIONS
> +
> +    if (context != NO_PLATFORM_GRAPHICS_CONTEXT_3D && texture != NO_PLATFORM_3D_OBJECT) {
> +        // create the inner 3d layer
> +        m_contentsLayer = [[Canvas3DLayer alloc] initWithContext:(CGLContextObj) context texture: (GLuint) texture];

This is leaking the Canvas3DLayer. you need to use AdoptNS.

No spaces after the, and use static_cast<>.

> +#if ENABLE(3D_CANVAS)
> +void GraphicsLayerCA::redraw3DCanvas()
> +{
> +    if (m_contentsLayerPurpose == ContentsLayerFor3DCanvas)
> +        [m_contentsLayer.get() setNeedsDisplay];
> +}
> +#endif

Aha, it is just setNeedsDisplay!

> Index: WebCore/rendering/RenderHTMLCanvas.cpp
> ===================================================================

> +bool RenderHTMLCanvas::requiresLayer() const
> +{
> +    if (RenderReplaced::requiresLayer())
> +        return true;
> +    
> +#if ENABLE(3D_CANVAS)
> +    HTMLCanvasElement* canvas = reinterpret_cast<HTMLCanvasElement*>(node());

I think a static_cast<> should work there

> Index: WebCore/rendering/RenderLayerBacking.cpp
> ===================================================================

> +#if ENABLE(3D_CANVAS)    
> +        else if (renderer()->isCanvas()) {
> +            HTMLCanvasElement* canvas = reinterpret_cast<HTMLCanvasElement*>(renderer()->node());

static_cast<>

> +                if (canvas->context3D()) {
> +                    m_graphicsLayer->setContentsTo3DCanvas(canvas->context3D(), canvas->texture3D());
> +                    m_graphicsLayer->setDrawsContent(true);

Why setDrawsContent(true)? That means we'll create backing store for the primary layer,
which you don't want.

Have you tested borders, outlines etc. on a 3d canvas? You should include some layout tests
with pixel results that verify the rendering of these.

> +#if ENABLE(3D_CANVAS)    
> +    bool is3DCanvas = false;
> +    if (renderer()->isCanvas()) {
> +        HTMLCanvasElement* canvas = reinterpret_cast<HTMLCanvasElement*>(renderer()->node());
> +        is3DCanvas = canvas->is3D();
> +    }
> +    if (is3DCanvas)
> +        return true;

Seems like you could eliminate is3DCanvas and just do an earlier return here. In fact,
if you know it's a canvas, you can always return.

>  void RenderLayerBacking::rendererContentChanged()
>  {
> -    if (canUseDirectCompositing() && renderer()->isImage())
> -        updateImageContents();
> +    if (canUseDirectCompositing()) {
> +        if (renderer()->isImage())
> +            updateImageContents();
> +        else {
> +#if ENABLE(3D_CANVAS)    
> +            if (renderer()->isCanvas()) {
> +                HTMLCanvasElement* canvas = reinterpret_cast<HTMLCanvasElement*>(renderer()->node());

static_cast<>

> Index: WebCore/rendering/RenderLayerCompositor.cpp
> ===================================================================

>  bool RenderLayerCompositor::requiresCompositingLayer(const RenderLayer* layer) const
>  {
> +#if ENABLE(3D_CANVAS)    
> +    bool is3DCanvas = false;
> +    if (layer->renderer()->isCanvas()) {
> +        HTMLCanvasElement* canvas = reinterpret_cast<HTMLCanvasElement*>(layer->renderer()->node());
> +        is3DCanvas = canvas->is3D();
> +    }
> +#endif

I'd prefer this be wrapped up in its own method, requiresCompositingForCanvs().

> Index: LayoutTests/fast/dom/Window/window-properties.html
> ===================================================================
> --- LayoutTests/fast/dom/Window/window-properties.html	(revision 47753)
> +++ LayoutTests/fast/dom/Window/window-properties.html	(working copy)
> @@ -60,7 +60,9 @@ var __skip__ = {
>      "window.objCPlugin" : 1,
>      "window.objCPluginFunction" : 1,
>      "window.plainText" : 1,
> -    "window.textInputController" : 1
> +    "window.textInputController" : 1,
> +    
> +    "window.CanvasRenderingContext3D" : 1 // We ignore CanvasRenderingContext3D and test it elsewhere, since it is not in all builds

Any reason for the blank line?

r- for lack of some basic tests and the few quibbles noted above. It's mostly good though.

[Chris Marrin]

Sending        LayoutTests/ChangeLog
Adding         LayoutTests/fast/canvas/change-context-expected.txt
Adding         LayoutTests/fast/canvas/change-context.html
Sending        LayoutTests/fast/dom/Window/window-properties.html
Sending        LayoutTests/fast/dom/resources/prototype-inheritance-2.js
Sending        LayoutTests/fast/dom/resources/prototype-inheritance.js
Sending        WebCore/ChangeLog
Sending        WebCore/WebCore.xcodeproj/project.pbxproj
Sending        WebCore/bindings/js/JSCanvasRenderingContext3DCustom.cpp
Sending        WebCore/bindings/js/JSCanvasRenderingContextCustom.cpp
Sending        WebCore/dom/Element.cpp
Sending        WebCore/dom/Node.cpp
Sending        WebCore/dom/Node.h
Sending        WebCore/html/HTMLCanvasElement.cpp
Sending        WebCore/html/HTMLCanvasElement.h
Sending        WebCore/html/canvas/CanvasBuffer.cpp
Sending        WebCore/html/canvas/CanvasBuffer.h
Sending        WebCore/html/canvas/CanvasFramebuffer.cpp
Sending        WebCore/html/canvas/CanvasFramebuffer.h
Sending        WebCore/html/canvas/CanvasObject.cpp
Sending        WebCore/html/canvas/CanvasObject.h
Sending        WebCore/html/canvas/CanvasProgram.cpp
Sending        WebCore/html/canvas/CanvasProgram.h
Sending        WebCore/html/canvas/CanvasRenderbuffer.cpp
Sending        WebCore/html/canvas/CanvasRenderbuffer.h
Sending        WebCore/html/canvas/CanvasRenderingContext3D.cpp
Sending        WebCore/html/canvas/CanvasRenderingContext3D.h
Sending        WebCore/html/canvas/CanvasShader.cpp
Sending        WebCore/html/canvas/CanvasShader.h
Sending        WebCore/html/canvas/CanvasTexture.cpp
Sending        WebCore/html/canvas/CanvasTexture.h
Sending        WebCore/page/DOMWindow.idl
Sending        WebCore/page/animation/AnimationBase.cpp
Sending        WebCore/page/animation/AnimationController.cpp
Sending        WebCore/platform/graphics/GraphicsContext3D.h
Sending        WebCore/platform/graphics/GraphicsLayer.h
Sending        WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
Sending        WebCore/platform/graphics/mac/GraphicsLayerCA.h
Sending        WebCore/platform/graphics/mac/GraphicsLayerCA.mm
Sending        WebCore/rendering/RenderHTMLCanvas.cpp
Sending        WebCore/rendering/RenderHTMLCanvas.h
Sending        WebCore/rendering/RenderLayerBacking.cpp
Sending        WebCore/rendering/RenderLayerCompositor.cpp
Sending        WebCore/rendering/RenderLayerCompositor.h
Sending        WebCore/rendering/RenderObject.h
Transmitting file data .............................................
Committed revision 47843.

[Chris Marrin]

Comment on attachment 38667 [details]
Final patch for Canvas3D work

Simon hand reviewed this and it is now landed