[webkit-reviews] review granted: [Bug 28018] Need to implement Canvas3d/WebGL for 3D rendering : [Attachment 38460] Patch with remaining new files

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 28018: Need to implement Canvas3d/WebGL for 3D rendering
https://bugs.webkit.org/show_bug.cgi?id=28018

Attachment 38460: Patch with remaining new files
https://bugs.webkit.org/attachment.cgi?id=38460&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> 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(context
Id))));
> +    else if (contextId == "webkit-3d")
> +	   return toJS(exec,
WTF::getPtr(reinterpret_cast<CanvasRenderingContext3D*>(imp->getContext(context
Id))));

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