Added a test for pepper3d. It ensures that we can successfully load a pepper ...

Added a test for pepper3d. It ensures that we can successfully load a pepper 3d plugin and render.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42793

[alokp, 2010-03-22 23:18:04]

http://codereview.chromium.org/1073003/diff/8001/9008
File chrome/test/ui/npapi_test_helper.h (right):

http://codereview.chromium.org/1073003/diff/8001/9008#newcode10
chrome/test/ui/npapi_test_helper.h:10: // Helper class for NPAPI plugin UI
tests.
I partially reverted my cl: http://codereview.chromium.org/1052001/show because
I decided to add pepper tests to npapi_test_plugin itself.

[apatrick, 2010-03-23 00:50:23]

http://codereview.chromium.org/1073003/diff/8001/9002
File webkit/glue/plugins/test/plugin_client.cc (right):

http://codereview.chromium.org/1073003/diff/8001/9002#newcode117
webkit/glue/plugins/test/plugin_client.cc:117: // generic test factory.
Bravo, sir!

http://codereview.chromium.org/1073003/diff/8001/9003
File webkit/glue/plugins/test/plugin_pepper_3d_test.cc (right):

http://codereview.chromium.org/1073003/diff/8001/9003#newcode9
webkit/glue/plugins/test/plugin_pepper_3d_test.cc:9: }  // namepsace
type

http://codereview.chromium.org/1073003/diff/8001/9003#newcode26
webkit/glue/plugins/test/plugin_pepper_3d_test.cc:26: memset(&es_data_, 0,
sizeof(STUserData));
sizeof(es_data_) is preferred I think.

http://codereview.chromium.org/1073003/diff/8001/9003#newcode44
webkit/glue/plugins/test/plugin_pepper_3d_test.cc:44: NPError
Pepper3DTest::SetWindow(NPWindow* pNPWindow) {
"pNPWindow" is not Google style.

http://codereview.chromium.org/1073003/diff/8001/9003#newcode109
webkit/glue/plugins/test/plugin_pepper_3d_test.cc:109: SetError("Could not
destroy PGL context");
Should these call SignalTestComplete?

http://codereview.chromium.org/1073003/diff/8001/9003#newcode159
webkit/glue/plugins/test/plugin_pepper_3d_test.cc:159: GLint x =
static_cast<GLint>(0.25f * es_context_.width);
Couldn't these be done entirely with integer arithmetic to avoid the casts?

http://codereview.chromium.org/1073003/diff/8001/9004
File webkit/glue/plugins/test/plugin_pepper_3d_test.h (right):

http://codereview.chromium.org/1073003/diff/8001/9004#newcode24
webkit/glue/plugins/test/plugin_pepper_3d_test.h:24: virtual NPError
SetWindow(NPWindow* pNPWindow);
"pNPWindow" is not Google style. How about just "window"?

http://codereview.chromium.org/1073003/diff/8001/9004#newcode37
webkit/glue/plugins/test/plugin_pepper_3d_test.h:37: void TestPixel(int x, int
y, GLubyte expected_color[3]);
const GLubyte?

http://codereview.chromium.org/1073003/diff/8001/9004#newcode47
webkit/glue/plugins/test/plugin_pepper_3d_test.h:47: friend void
RepaintCallback(NPP, NPDeviceContext3D*);
Could this be a static function rather than a friend free function?

[greggman, 2010-03-23 02:13:53]

LGTM from my part after Al's comments.

[David Springer, 2010-03-23 05:19:13]

I did not notice any mac-specific files in this CL, such as an Info.plist. 
Would you like help making these tests work on Mac as well?

[brettw, 2010-03-23 05:21:37]

Darin & Dimitri: at least one of you guys should comment on whether we should
add this stuff to the regular NPAPI test plugin. The other option is to write a
new one for pepper-specific stuff.

[brettw, 2010-03-23 16:18:04]

I talked with Dimitri and it sounds like we shouldn't be adding this to the
existing webkit test plugin.

The existing plugin is really for running WebKit's tests. We hope to push all
that code upstream to the WebKit repo. This test is really a a test for our
renderer, and should live in our repo.

The old way of adding it to the pepper test plugin was okay but that plugin is
pretty nasty. If you want to add it back there that would be OK with me. Long
term, I think we need a new plugin for running automated tests like yours. This
new plugin should probably be forked off of pepper_test_plugin, which is
somewhat cleaned up over the webkit test plugin. If you want you can clone the
project and add your test there, or we can move your test later in a subsequent
patch, whatever makes it easier for you.

[alokp, 2010-03-23 16:38:09]

My reasoning for adding it to npapi-test-plugin was that pepper is an
extension to npapi and once it becomes standard, will be implemented
in webkit. Also ui_tests infrastructure along with PluginClient and
PluginTest classes make it really convenient to write tests. I see
there is a test-object in pepper-test-plugin but I do not understand
how it is supposed to work and how the plugin gets hooked to gtest.

On Tue, Mar 23, 2010 at 10:18 AM,  <brettw@chromium.org> wrote:
> I talked with Dimitri and it sounds like we shouldn't be adding this to the
> existing webkit test plugin.
>
> The existing plugin is really for running WebKit's tests. We hope to push
> all
> that code upstream to the WebKit repo. This test is really a a test for our
> renderer, and should live in our repo.
>
> The old way of adding it to the pepper test plugin was okay but that plugin
> is
> pretty nasty. If you want to add it back there that would be OK with me.
> Long
> term, I think we need a new plugin for running automated tests like yours.
> This
> new plugin should probably be forked off of pepper_test_plugin, which is
> somewhat cleaned up over the webkit test plugin. If you want you can clone
> the
> project and add your test there, or we can move your test later in a
> subsequent
> patch, whatever makes it easier for you.
>
> http://codereview.chromium.org/1073003
>

[alokp, 2010-03-23 16:59:11]

Dave: Provided it is ok to add these tests to npapi_test_plugin, my plan is to
submit it as disabled. This is windows only right now. I will make it work on
other platforms in subsequent CLs. I can enable it only after it works on all
platforms and the buildbots have OpenGL support.

http://codereview.chromium.org/1073003/diff/8001/9003
File webkit/glue/plugins/test/plugin_pepper_3d_test.cc (right):

http://codereview.chromium.org/1073003/diff/8001/9003#newcode9
webkit/glue/plugins/test/plugin_pepper_3d_test.cc:9: }  // namepsace
?

http://codereview.chromium.org/1073003/diff/8001/9003#newcode26
webkit/glue/plugins/test/plugin_pepper_3d_test.cc:26: memset(&es_data_, 0,
sizeof(STUserData));
On 2010/03/23 00:50:23, apatrick wrote:
> sizeof(es_data_) is preferred I think.

Done.

http://codereview.chromium.org/1073003/diff/8001/9003#newcode44
webkit/glue/plugins/test/plugin_pepper_3d_test.cc:44: NPError
Pepper3DTest::SetWindow(NPWindow* pNPWindow) {
On 2010/03/23 00:50:23, apatrick wrote:
> "pNPWindow" is not Google style.

Done.

http://codereview.chromium.org/1073003/diff/8001/9003#newcode109
webkit/glue/plugins/test/plugin_pepper_3d_test.cc:109: SetError("Could not
destroy PGL context");
I am not sure if SignalTestComplete aborts immediately. I am currently calling
SignalTestComplete only for fatal errors. Other errors will just get appended.

http://codereview.chromium.org/1073003/diff/8001/9003#newcode159
webkit/glue/plugins/test/plugin_pepper_3d_test.cc:159: GLint x =
static_cast<GLint>(0.25f * es_context_.width);
On 2010/03/23 00:50:23, apatrick wrote:
> Couldn't these be done entirely with integer arithmetic to avoid the casts?

Done.

http://codereview.chromium.org/1073003/diff/8001/9004
File webkit/glue/plugins/test/plugin_pepper_3d_test.h (right):

http://codereview.chromium.org/1073003/diff/8001/9004#newcode24
webkit/glue/plugins/test/plugin_pepper_3d_test.h:24: virtual NPError
SetWindow(NPWindow* pNPWindow);
I agree but I was just following the base class style. Done.

http://codereview.chromium.org/1073003/diff/8001/9004#newcode37
webkit/glue/plugins/test/plugin_pepper_3d_test.h:37: void TestPixel(int x, int
y, GLubyte expected_color[3]);
On 2010/03/23 00:50:23, apatrick wrote:
> const GLubyte?

Done.

http://codereview.chromium.org/1073003/diff/8001/9004#newcode47
webkit/glue/plugins/test/plugin_pepper_3d_test.h:47: friend void
RepaintCallback(NPP, NPDeviceContext3D*);
On 2010/03/23 00:50:23, apatrick wrote:
> Could this be a static function rather than a friend free function?

Done.

[apatrick, 2010-03-23 17:12:41]

LGTM for style. I defer the webkit integration issues to others.

http://codereview.chromium.org/1073003/diff/8001/9003
File webkit/glue/plugins/test/plugin_pepper_3d_test.cc (right):

http://codereview.chromium.org/1073003/diff/8001/9003#newcode9
webkit/glue/plugins/test/plugin_pepper_3d_test.cc:9: }  // namepsace
At least I was ironic. I made a typo trying to say "typo". It should be
"namespace" rather than "namepsace".

[alokp, 2010-03-23 17:16:29]

http://codereview.chromium.org/1073003/diff/8001/9003
File webkit/glue/plugins/test/plugin_pepper_3d_test.cc (right):

http://codereview.chromium.org/1073003/diff/8001/9003#newcode9
webkit/glue/plugins/test/plugin_pepper_3d_test.cc:9: }  // namepsace
On 2010/03/23 17:12:41, apatrick wrote:
> At least I was ironic. I made a typo trying to say "typo". It should be
> "namespace" rather than "namepsace".

Done.

[David Springer, 2010-03-23 19:17:35]

On 2010/03/23 17:16:29, alokp wrote:
> http://codereview.chromium.org/1073003/diff/8001/9003
> File webkit/glue/plugins/test/plugin_pepper_3d_test.cc (right):
> 
> http://codereview.chromium.org/1073003/diff/8001/9003#newcode9
> webkit/glue/plugins/test/plugin_pepper_3d_test.cc:9: }  // namepsace
> On 2010/03/23 17:12:41, apatrick wrote:
> > At least I was ironic. I made a typo trying to say "typo". It should be
> > "namespace" rather than "namepsace".
> 
> Done.

LGTM, given that there is a plan in place to have these tests work on Mac.  Is
there a bug tracking this?

[alokp, 2010-03-23 20:49:09]

Not that I know of.

[brettw, 2010-03-24 16:27:19]

On Tue, Mar 23, 2010 at 9:37 AM, Alok Priyadarshi <alokp@chromium.org> wrote:
> My reasoning for adding it to npapi-test-plugin was that pepper is an
> extension to npapi and once it becomes standard, will be implemented
> in webkit. Also ui_tests infrastructure along with PluginClient and
> PluginTest classes make it really convenient to write tests. I see
> there is a test-object in pepper-test-plugin but I do not understand
> how it is supposed to work and how the plugin gets hooked to gtest.

There hasn't actually been a lot of discussion with the WebKit folks
about the pepper extensions yet, and we'll very likely upstream our
plugin changes long before WebKit takes pepper. When that happens,
having them together will be painful since WebKit won't have the right
headers to compile the plugin. This will force the upstreamer to split
the changes apart for us, or it will just break our tests, neither of
which we want.

I'll look into how the test infrastructure works on this today to help
you create a new plugin.

Brett

[alokp, 2010-03-24 17:03:51]

Hi Brett,
If you already do not know how TestObject works, then please don't bother. I
will try to figure it out myself. But it will have to be tomorrow. Boulder
office is closed today due to heavy snow and my laptop cannot handle the huge
chrome codebase. I will be working on ANGLE and my 20% project today.

-Alok

[brettw, 2010-03-24 17:11:12]

On Wed, Mar 24, 2010 at 10:03 AM,  <alokp@chromium.org> wrote:
> Hi Brett,
> If you already do not know how TestObject works, then please don't bother. I
> will try to figure it out myself. But it will have to be tomorrow. Boulder
> office is closed today due to heavy snow and my laptop cannot handle the
> huge
> chrome codebase. I will be working on ANGLE and my 20% project today.

I see. No I don't know how test object works. Please let me know if
you need help! I want to make sure we can get good test coverage by
the end of the week.

Brett

[alokp, 2010-03-25 20:23:16]

I have separated out pepper-specific stuff from npapi. I created a
npapi_test_common library to abstract out the infrastructure for testing npapi
plugins and used it for both npapi_test_plugin and pepper_test_plugin.

One downside for this cl is that the pepper_test_plugin is detached from
all.gyp. I am assuming it will be deprecated as soon as we have tests for 2d,
events, and audio. For now if people want to use that plugin they will have to
generate the build project themselves.

[brettw, 2010-03-25 21:59:57]

LGTM

http://codereview.chromium.org/1073003/diff/57002/83013
File webkit/glue/plugins/test/plugin_test_factory.h (right):

http://codereview.chromium.org/1073003/diff/57002/83013#newcode20
webkit/glue/plugins/test/plugin_test_factory.h:20: } // namespace NPAPIClient
Two spaces before end-of-line comments.

http://codereview.chromium.org/1073003/diff/57002/83014
File webkit/glue/plugins/test/plugin_windowless_test.cc (right):

http://codereview.chromium.org/1073003/diff/57002/83014#newcode21
webkit/glue/plugins/test/plugin_windowless_test.cc:21: : PluginTest(id,
host_functions) {
Can you fix this indenting (I know it's not your fault) to 4 spaces?

http://codereview.chromium.org/1073003/diff/57002/83009
File webkit/tools/pepper_test_plugin/pepper_3d_test.cc (right):

http://codereview.chromium.org/1073003/diff/57002/83009#newcode198
webkit/tools/pepper_test_plugin/pepper_3d_test.cc:198: } // namespace
NPAPIClient
Need 2 spaces before //

[alokp, 2010-03-25 22:35:40]

I had to add a couple more files to the CL because a few npapi tests were
failing. If you look into NPP_SetWindow in plugin_client.cc, I removed a guard
against (pNPWindow->window == NULL) so that pepper plugins which are windowless
receive SetWindow call. This broke a couple of tests that were not handling NULL
window properly.

http://codereview.chromium.org/1073003/diff/57002/83013
File webkit/glue/plugins/test/plugin_test_factory.h (right):

http://codereview.chromium.org/1073003/diff/57002/83013#newcode20
webkit/glue/plugins/test/plugin_test_factory.h:20: } // namespace NPAPIClient
On 2010/03/25 21:59:57, brettw wrote:
> Two spaces before end-of-line comments.

Done.

http://codereview.chromium.org/1073003/diff/57002/83014
File webkit/glue/plugins/test/plugin_windowless_test.cc (right):

http://codereview.chromium.org/1073003/diff/57002/83014#newcode21
webkit/glue/plugins/test/plugin_windowless_test.cc:21: : PluginTest(id,
host_functions) {
On 2010/03/25 21:59:57, brettw wrote:
> Can you fix this indenting (I know it's not your fault) to 4 spaces?

Done.

http://codereview.chromium.org/1073003/diff/57002/83009
File webkit/tools/pepper_test_plugin/pepper_3d_test.cc (right):

http://codereview.chromium.org/1073003/diff/57002/83009#newcode198
webkit/tools/pepper_test_plugin/pepper_3d_test.cc:198: } // namespace
NPAPIClient
On 2010/03/25 21:59:57, brettw wrote:
> Need 2 spaces before //

Done.

[brettw, 2010-03-25 22:38:02]

On Thu, Mar 25, 2010 at 3:35 PM,  <alokp@chromium.org> wrote:
> I had to add a couple more files to the CL because a few npapi tests were
> failing. If you look into NPP_SetWindow in plugin_client.cc, I removed a
> guard
> against (pNPWindow->window == NULL) so that pepper plugins which are
> windowless
> receive SetWindow call. This broke a couple of tests that were not handling
> NULL
> window properly.

Okay, sounds good, thanks.

Brett