We need some Windows drawing code for the test plugin. The Mac OS X implementation already has drawing code. Relevant files: modules/plugin/test/testplugin/nptest_platform.h modules/plugin/test/testplugin/nptest_windows.cpp modules/plugin/test/testplugin/nptest_macosx.mm

If you do this, please track the changes in bug 473577 so we can use the plugin for reftests. (They're pretty simple.)

Comment on attachment 359079 [details] [diff] [review] windows test plugin rendering v.1 Ted would you be a good reviewer for this?

I just noticed a litle left over debugging code in this in the use of pluginDrawWin instead of pluginDraw. I'll clean that up before this lands.

You should probably have Josh give it a once-over (since he's the module owner), but I'll take a look as well. One thing I noticed that you didn't handle was the alpha value (which I conveniently forgot to write a reftest for). We should support drawing with partial transparency. I'll attach a testcase (that I should turn into a reftest) in a sec.

This and the previous test attachment should render identically if the test plugin supports the alpha channel properly.

To get transparency support I had to switch from the ancient GDI to the newer GDI+. As an example plug-in that's actually better. However the plugin will fail to load on older operating systems that don't have GDI+ available. XP came with it out of the box so I don't believe this will cause any problems for tinderbox.

Comment on attachment 359335 [details] [diff] [review] windows test plugin rendering v.2 + if (pe == NULL || instanceData == NULL || + instanceData->window.type != NPWindowTypeDrawable) return 0; Personally I would put that return on the next line. +static void +GetColorsFromRGBA(PRUint32 rgba, BYTE* r, BYTE* g, BYTE* b, BYTE* a) You could make this utility method just return a Color or a SolidBrush, but it's not a big deal either way. Otherwise looks fine to me. I think using GDI+ should be fine, since it's only for tests (and for reference) anyway.

Comment on attachment 359335 [details] [diff] [review] windows test plugin rendering v.2 What Ted said :)

Note that we'll have to modify the reftest conditions here as well before checking this in: http://mxr.mozilla.org/mozilla-central/source/modules/plugin/test/reftest/reftest.list

Would you like me to push this for you?

(In reply to comment #13) > Would you like me to push this for you? I'll get it, I was planning on waiting till the weekend.

Comment on attachment 359563 [details] [diff] [review] windows test plugin rendering v.3 >-random-if(!haveTestPlugin||MOZ_WIDGET_TOOLKIT!="cocoa") != plugin-sanity.html about:blank >-fails-if(!haveTestPlugin||MOZ_WIDGET_TOOLKIT!="cocoa") == plugin-sanity.html div-sanity.html >+random-if(!haveTestPlugin||MOZ_WIDGET_TOOLKIT!="cocoa"||MOZ_WIDGET_TOOLKIT!="windows") != plugin-sanity.html about:blank >+fails-if(!haveTestPlugin||MOZ_WIDGET_TOOLKIT!="cocoa"||MOZ_WIDGET_TOOLKIT!="windows") == plugin-sanity.html div-sanity.html Shouldn't this rather be +random-if(!haveTestPlugin||(MOZ_WIDGET_TOOLKIT!="cocoa"&&MOZ_WIDGET_TOOLKIT!="windows")) != plugin-sanity.html about:blank +fails-if(!haveTestPlugin||(MOZ_WIDGET_TOOLKIT!="cocoa"&&MOZ_WIDGET_TOOLKIT!="windows")) == plugin-sanity.html div-sanity.html ?

(In reply to comment #16) > (From update of attachment 359563 [details] [diff] [review]) > >-random-if(!haveTestPlugin||MOZ_WIDGET_TOOLKIT!="cocoa") != plugin-sanity.html about:blank > >-fails-if(!haveTestPlugin||MOZ_WIDGET_TOOLKIT!="cocoa") == plugin-sanity.html div-sanity.html > >+random-if(!haveTestPlugin||MOZ_WIDGET_TOOLKIT!="cocoa"||MOZ_WIDGET_TOOLKIT!="windows") != plugin-sanity.html about:blank > >+fails-if(!haveTestPlugin||MOZ_WIDGET_TOOLKIT!="cocoa"||MOZ_WIDGET_TOOLKIT!="windows") == plugin-sanity.html div-sanity.html > > Shouldn't this rather be > > +random-if(!haveTestPlugin||(MOZ_WIDGET_TOOLKIT!="cocoa"&&MOZ_WIDGET_TOOLKIT!="windows")) > != plugin-sanity.html about:blank > +fails-if(!haveTestPlugin||(MOZ_WIDGET_TOOLKIT!="cocoa"&&MOZ_WIDGET_TOOLKIT!="windows")) > == plugin-sanity.html div-sanity.html > > ? I'll update it. Thx.