[webkit-reviews] review denied: [Bug 35626] Implement WebGLContextLostEvent and WebGLContextRestoredEvent : [Attachment 72572] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Nov 2 12:36:26 PDT 2010


Kenneth Russell <kbr at google.com> has denied Adrienne Walker <enne at google.com>'s
request for review:
Bug 35626: Implement WebGLContextLostEvent and WebGLContextRestoredEvent
https://bugs.webkit.org/show_bug.cgi?id=35626

Attachment 72572: Patch
https://bugs.webkit.org/attachment.cgi?id=72572&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=72572&action=review

Thanks for working through how to hook this up. It generally looks good but
there are a few minor issues. Also, could you consider hooking up the
webglcontextcreationerror event in WebGLRenderingContext::create in the case
where the GraphicsContext3D creation fails? Also, please file a follow-on bug
to actually call the loseContext / restoreContext entry points.

> LayoutTests/fast/canvas/webgl/resources/webgl-test-utils.js:287
> +  fillTexture(gl, tex, width, height, color);

Split this change into a separate patch.

> WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:242
> -	   return v8::Undefined();
> +	   return v8::Null();
>      }
>      if (!succeed)
> -	   return v8::Undefined();
> +	   return v8::Null();

These changes look unrelated and if so should be split into a separate patch.

> WebCore/html/canvas/WebGLEvent.cpp:16
> + *	  * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *	  * 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.
> + *	  * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.

This is actually not the correct license header. See e.g.
WebKit/chromium/tests/PODIntervalTreeTest.cpp for the correct one.

> WebCore/html/canvas/WebGLEvent.cpp:36
> +WebGLEvent::WebGLEvent()

According to the spec this should be named WebGLContextEvent. Please rename it.


> WebCore/html/canvas/WebGLEvent.h:16
> + *	  * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *	  * 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.
> + *	  * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.

Wrong license header.

> WebCore/html/canvas/WebGLEvent.h:38
> +class WebGLEvent : public Event {

WebGLContextEvent.

> WebCore/html/canvas/WebGLEvent.h:54
> +protected:

For the time being, at least, these can be private.

> WebCore/html/canvas/WebGLEvent.idl:17
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License

> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.

Wrong license header.

> WebCore/html/canvas/WebGLEvent.idl:23
> +    ] WebGLEvent : Event {

WebGLContextEvent.

> WebCore/html/canvas/WebGLRenderingContext.idl:555
> +	   [StrictTypeChecking, ConvertNullStringTo=Null] DOMString
getProgramInfoLog(in WebGLProgram program) raises(DOMException);

These ConvertNullStringTo=Null changes look unrelated and if so should be split
off into a separate patch.


More information about the webkit-reviews mailing list