crbug.com/93809
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.
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.
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.
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.
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.
Created attachment 109210 [details] proposed patch fixed style issues.
ping!
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?
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
Created attachment 110000 [details] proposed patch PTAL
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.
Created attachment 110006 [details] proposed patch resolved style issues.
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.
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.
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
(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?
Created attachment 110019 [details] proposed patch - Replaced plain char* with WebKit::WebCString - Added test description as html comment instead of html text
Comment on attachment 110019 [details] proposed patch OK great, R=me
Darin: Any further issues?
Comment on attachment 110019 [details] proposed patch Attachment 110019 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9977372
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*.
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.
Comment on attachment 110154 [details] proposed patch Clearing flags on attachment: 110154 Committed r97678: <http://trac.webkit.org/changeset/97678>
All reviewed patches have been landed. Closing bug.