Bug 69027 – [Chromium] Add layout tests for WebPlugin compositor path

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Alok Priyadarshi]

crbug.com/93809

[Alok Priyadarshi]

Created attachment 109073 [details]
proposed patch

As suggested I created a fake pepper plugin emulating the pepper-3d compositor path. The fake plugin just renders a translucent green texture that gets composited with two other translucent red and blue boxes.

[WebKit Review Bot]

Attachment 109073 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Tools/DumpRenderTree/chromium/TestPepperPlugin.cpp:98:  One space before end of line comments  [whitespace/comments] [5]
Tools/DumpRenderTree/chromium/TestPepperPlugin.h:79:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Comment on attachment 109073 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109073&action=review

> Tools/DumpRenderTree/chromium/TestPepperPlugin.cpp:43
> +const WebString TestPepperPlugin::kMimeType =

since this plugin is not actually using pepper, I think it probably shouldn't refer to pepper.

this is just a WebPlugin implementation that can be used for testing plugin support.

[Alok Priyadarshi]

Created attachment 109204 [details]
proposed patch

Used "webplugin" instead of "pepper". Added comment about how it can be extended in future to test various aspects of testing WebPlugin. Made the scene rendered into the plugin more interesting.

[WebKit Review Bot]

Attachment 109204 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Tools/DumpRenderTree/chromium/TestWebPlugin.h:74:  This { should be at the end of the previous line  [whitespace/braces] [4]
Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:192:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alok Priyadarshi]

Created attachment 109210 [details]
proposed patch

fixed style issues.

[Alok Priyadarshi]

ping!

[James Robinson]

Comment on attachment 109210 [details]
proposed patch

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?

[Alok Priyadarshi]

Comment on attachment 109210 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109210&action=review

The plugin now draws a scene that can be customized using plugin parameters. I used the parameters to write three separate tests.

I think "repaint" is already being tested. Remember that plugin is not drawing in response to WebPlugin::paint(). Instead it is painting in response to resize event and asking the browser to repaint the backing texture.

>> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:16
>> + * this software without specific prior written permission.
> 
> wrong license header - we use a 2-clause for new files

done

>> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:42
>> +// GLenum values copied from gl2.h.
> 
> why don't you use the ones from GraphicsContext3D.h ?

Are we allowed to directly include anything in WebCore? I did not want to add unnecessary dependency. These enum values will never change. Ideally WebGraphicsContext3D.h should define these.

>> 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'

I am using the same type as expected by WebGraphicsContext3D::shaderSource(). Removed abbreviation.

>> 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

I am using the same type as expected by WebGraphicsContext3D::shaderSource().

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

done.

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

done.

>> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:100
>> +// static
> 
> we don't typically use this sort of comment in webkit

removed

>> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:102
>> +        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?

done

>> 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

done

>> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:150
>> +        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 ( ?

done

>> 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 ?)

done

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

done

>> Tools/DumpRenderTree/chromium/TestWebPlugin.h:43
>> +// This behavior can be sutomized in future using WebPluginParams.
> 
> typo (i think?) "sutomized"

Added new comment.

>> Tools/DumpRenderTree/chromium/TestWebPlugin.h:46
>> +    static const WebKit::WebString kMimeType;
> 
> could this be a function instead?

done

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

done

>> Tools/DumpRenderTree/chromium/TestWebPlugin.h:57
>> +            const WebKit::WebRect& frameRect,
> 
> weird line wrapping here too

done

>> LayoutTests/platform/chromium/compositing/plugins/webplugin-alpha.html:34
>> +  <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?

done

[Alok Priyadarshi]

Created attachment 110000 [details]
proposed patch

PTAL

[WebKit Review Bot]

Attachment 110000 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Tools/DumpRenderTree/chromium/TestWebPlugin.h:103:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/DumpRenderTree/chromium/TestWebPlugin.h:104:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/DumpRenderTree/chromium/TestWebPlugin.h:105:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:59:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:59:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alok Priyadarshi]

Created attachment 110006 [details]
proposed patch

resolved style issues.

[James Robinson]

Comment on attachment 110000 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110000&action=review

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:281
> +unsigned TestWebPlugin::loadShader(unsigned type, const WGC3Dchar* source)

my point wasn't that const char* was worse than const WGC3Dchar*, but that we should use c++ string types instead of c-style strings when possible.  So in this instance, pass the strings around as WTF::String (or maybe WebString, I'm not sure which is more commonly used in Tools/DumpRenderTree/chromium) until the point where you call into the API requiring a c-style string.

> LayoutTests/platform/chromium/compositing/plugins/webplugin-reflection.html:14
> +<p>You should see a blue triangle on green background and its reflection.</p>

Don't put the text in the actual test - this will make the pixel results different on every platform!  Instead, put the descriptive text in an HTML comment so that people looking at a potential test failure can see what the intention is without having platform-specific results.

[James Robinson]

Comment on attachment 110006 [details]
proposed patch

My comments from the previous patch still apply.  This test will make the bots to red as is because there's text in the pixel results but there aren't platform-specific expectations.

[Alok Priyadarshi]

Comment on attachment 110000 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110000&action=review

>> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:281
>> +unsigned TestWebPlugin::loadShader(unsigned type, const WGC3Dchar* source)
> 
> my point wasn't that const char* was worse than const WGC3Dchar*, but that we should use c++ string types instead of c-style strings when possible.  So in this instance, pass the strings around as WTF::String (or maybe WebString, I'm not sure which is more commonly used in Tools/DumpRenderTree/chromium) until the point where you call into the API requiring a c-style string.

I could certainly use WebKit string classes but I do not understand why it is important. These shader strings:
- can only be ASCII
- are not stored anywhere
- only used in private functions

>> LayoutTests/platform/chromium/compositing/plugins/webplugin-reflection.html:14
>> +<p>You should see a blue triangle on green background and its reflection.</p>
> 
> Don't put the text in the actual test - this will make the pixel results different on every platform!  Instead, put the descriptive text in an HTML comment so that people looking at a potential test failure can see what the intention is without having platform-specific results.

will do

[James Robinson]

(In reply to comment #15)
> (From update of attachment 110000 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110000&action=review
> 
> >> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:281
> >> +unsigned TestWebPlugin::loadShader(unsigned type, const WGC3Dchar* source)
> > 
> > my point wasn't that const char* was worse than const WGC3Dchar*, but that we should use c++ string types instead of c-style strings when possible.  So in this instance, pass the strings around as WTF::String (or maybe WebString, I'm not sure which is more commonly used in Tools/DumpRenderTree/chromium) until the point where you call into the API requiring a c-style string.
> 
> I could certainly use WebKit string classes but I do not understand why it is important. These shader strings:
> - can only be ASCII
> - are not stored anywhere
> - only used in private functions
> 

These things are all true today.  Will they always be? Is there any drawback to using a more future-proof type?

[Alok Priyadarshi]

Created attachment 110019 [details]
proposed patch

- Replaced plain char* with WebKit::WebCString
- Added test description as html comment instead of html text

[James Robinson]

Comment on attachment 110019 [details]
proposed patch

OK great, R=me

[Alok Priyadarshi]

Darin: Any further issues?

[WebKit Review Bot]

Comment on attachment 110019 [details]
proposed patch

Attachment 110019 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9977372

[Alok Priyadarshi]

Created attachment 110154 [details]
proposed patch

Fixed compile errors on linux. Used WTF::CString instead of WebKit::WebCString. Apparently gcc cannot convert a char* -> std::string -> WebCString. WebString also does not have constructor that takes char*.

[Adam Barth]

Comment on attachment 110019 [details]
proposed patch

Cleared James Robinson's review+ from obsolete attachment 110019 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Review Bot]

Comment on attachment 110154 [details]
proposed patch

Clearing flags on attachment: 110154

Committed r97678: <http://trac.webkit.org/changeset/97678>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.