469830 - need Windows drawing code for test plugin

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P3)

Description

[Josh Aas]

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

Comment 1

[(not currently active) Ted Mielczarek]

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

Comment 2

[Jim Mathies [:jimm]]

Attachment: windows test plugin rendering v.1

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 359079 [details] [diff] [review]
windows test plugin rendering v.1

Ted would you be a good reviewer for this?

Comment 4

[Jim Mathies [:jimm]]

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.

Comment 5

[(not currently active) Ted Mielczarek]

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.

Comment 6

[(not currently active) Ted Mielczarek]

Attachment: reference page

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: test page (uses test plugin)

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

Comment 8

[Jim Mathies [:jimm]]

Attachment: windows test plugin rendering v.2

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 9

[(not currently active) Ted Mielczarek]

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 10

[Josh Aas]

Comment on attachment 359335 [details] [diff] [review]
windows test plugin rendering v.2

What Ted said :)

Comment 11

[(not currently active) Ted Mielczarek]

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

Comment 12

[Jim Mathies [:jimm]]

Attachment: windows test plugin rendering v.3

Comment 13

[(not currently active) Ted Mielczarek]

Would you like me to push this for you?

Comment 14

[Jim Mathies [:jimm]]

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

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/f46ab3393b32

Comment 16

[Markus Stange [:mstange]]

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

?

Comment 17

[Jim Mathies [:jimm]]

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