[webkit-reviews] review denied: [Bug 69027] Add layout tests for WebPlugin compositor path : [Attachment 109210] proposed patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Alok Priyadarshi
<alokp at chromium.org>'s request for review:
Bug 69027: Add layout tests for WebPlugin compositor path
https://bugs.webkit.org/show_bug.cgi?id=69027

Attachment 109210: proposed patch
https://bugs.webkit.org/attachment.cgi?id=109210&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109210&action=review


Good start.  I'd really like to see a repaint test as well to make sure that
the paths for a WebPlugin requesting a new frame and the compositor picking up
the correct texture contents are exercised.

> Tools/DumpRenderTree/chromium/TestWebPlugin.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.

wrong license header - we use a 2-clause for new files

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:42
> +// GLenum values copied from gl2.h.

why don't you use the ones from GraphicsContext3D.h ?

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:58
> +static unsigned loadShader(unsigned type, const char* src,
WebGraphicsContext3D* context)

please use an actual string type here (such as WTF::String) instead of a
c-style string. also, in webkit we typically don't abbreviate words like
'source'

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:75
> +static unsigned loadProgram(const char* vSrc, const char* fSrc,
WebGraphicsContext3D* context)

same here - please use a real string type and don't abbreviate the parameter
names

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:77
> +    unsigned vShader = loadShader(GL_VERTEX_SHADER, vSrc, context);

vShader -> vertexShader

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:78
> +    unsigned fShader = loadShader(GL_FRAGMENT_SHADER, fSrc, context);

fShader -> fragmentShader

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:100
> +// static

we don't typically use this sort of comment in webkit

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:102
> +const WebString TestWebPlugin::kMimeType =
> +	   WebString::fromUTF8("application/x-webkit-test-webplugin");

i know this is just test code but static initializers are still bad. can you
just expose a static const getter for this instead of constructing an instance
of WebString statically?

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:110
> +    memset(&m_scene, 0, sizeof(m_scene));

gross! give the struct a constructor and initialize the fields if you want to
zero-initialize it

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:150
> +void TestWebPlugin::updateGeometry(
> +	   const WebRect& frameRect,

this line wrapping is weird. can you put frameRect on the first line and line
up the the rest with the opening ( ?

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:187
> +    float vVertices[] = { 0.0f,  0.8f, 0.0f, 

please don't abbreviate. i'm not sure what this is supposed to mean
(vertexVertices ?)

> Tools/DumpRenderTree/chromium/TestWebPlugin.h:8
> + *	  * Redistributions of source code must retain the above copyright

wrong header here too

> Tools/DumpRenderTree/chromium/TestWebPlugin.h:43
> +// This behavior can be sutomized in future using WebPluginParams.

typo (i think?) "sutomized"

> Tools/DumpRenderTree/chromium/TestWebPlugin.h:46
> +    static const WebKit::WebString kMimeType;

could this be a function instead?

> Tools/DumpRenderTree/chromium/TestWebPlugin.h:49
> +    ~TestWebPlugin();

virtual

> Tools/DumpRenderTree/chromium/TestWebPlugin.h:57
> +    virtual void updateGeometry(
> +	       const WebKit::WebRect& frameRect,

weird line wrapping here too

> LayoutTests/platform/chromium/compositing/plugins/webplugin-alpha.html:34
> +<body>
> +  <div id="box1"></div>
> +  <embed id="plugin" type="application/x-webkit-test-webplugin">
> +  <div id="box2"></div>

could you describe what we're supposed to see here in an HTML comment, so that
if the output changes people can tell if the test is passing or not?

could you also construct a test case where it's easier to tell if the test is
in fact passing or not? perhaps have a red square with a green plugin on top so
if the plugin fails to display the red square is visible?