need Windows drawing code for test plugin

RESOLVED FIXED

Status

()

Core
Plug-ins
P3
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Josh Aas, Assigned: jimm)

Tracking

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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
(Reporter)

Updated

10 years ago
OS: Mac OS X → Windows XP
(Reporter)

Updated

10 years ago
Assignee: nobody → jmathies
(Assignee)

Updated

10 years ago
Priority: -- → P3
If you do this, please track the changes in bug 473577 so we can use the plugin for reftests. (They're pretty simple.)
(Assignee)

Comment 2

10 years ago
Created attachment 359079 [details] [diff] [review]
windows test plugin rendering v.1
(Assignee)

Updated

10 years ago
Attachment #359079 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 3

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

Ted would you be a good reviewer for this?
(Assignee)

Comment 4

10 years ago
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.
Created attachment 359181 [details]
test page (uses test plugin)

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

Comment 8

10 years ago
Created attachment 359335 [details] [diff] [review]
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.
Attachment #359079 - Attachment is obsolete: true
Attachment #359079 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

10 years ago
Attachment #359335 - Flags: superreview?(joshmoz)
Attachment #359335 - Flags: review?(ted.mielczarek)
Attachment #359335 - Flags: review?(ted.mielczarek) → review+
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.
(Reporter)

Comment 10

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

What Ted said :)
Attachment #359335 - Flags: superreview?(joshmoz) → review+
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
(Assignee)

Comment 12

10 years ago
Created attachment 359563 [details] [diff] [review]
windows test plugin rendering v.3
(Assignee)

Updated

10 years ago
Attachment #359335 - Attachment is obsolete: true
Would you like me to push this for you?
(Assignee)

Comment 14

10 years ago
(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.
(Assignee)

Comment 15

10 years ago
http://hg.mozilla.org/mozilla-central/rev/f46ab3393b32
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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

?
(Assignee)

Comment 17

10 years ago
(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.
Blocks: 475723
You need to log in before you can comment on or make changes to this bug.