473577 - make test plugin usable for reftests

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[(not currently active) Ted Mielczarek]

Josh wrote a test plugin in bug 386676 for use in plugin mochitests etc. This is great, since we severely lack coverage there. We also lack coverage of plugins in our reftests. We should extend the test plugin to be able to render simple color patterns so we could use it in reftest. As a start, something like "fill completely with green" would make it usable for a lot of reftests.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: very basic mac implementation

Here's a not-very-useful Mac implementation. It adds a drawMode field to the plugin data, and adds DM_SOLID_COLOR as an option, with the color specified in another field of the plugin data. I don't really know enough about plugins to add any useful way to set this (via scripting or URL params or embed params) but I assume it would be straightforward to do so. Having the ability to do:
<embed type="application/x-test" bgcolor="FF00FF00"></embed>

or something like that would make this quite usable for reftests.

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: let test plugin draw solid colors, add reftests

Ok, this lets you specify some parameters in the <embed> (or presumably <param>s under the <object>, if the docs are to be believed), so you can do like:
<embed type="application/x-test" width="400" height="400" drawmode="solid" color="FF00FF00"></embed>
to get a solid green plugin that's 400x400 px. The color format is AARRGGBB. You can leave off the AA and it will default to 0xFF.

I wrote some basic sanity checking reftests, and they pass, which is cool. dbaron: is there a way to include these in the main reftest.list only if plugins are enabled? Alternately, is there a way to make them all fails-if(plugins are disabled)?

Comment 3

[Josh Aas]

Comment on attachment 357162 [details] [diff] [review]
let test plugin draw solid colors, add reftests

>+PRUint32
>+parseHexColor(char* color)

Please move this into nptest_utils

>+  for (int i=0; i<argc; i++) {

Style is "i = 0", "i < argc".

What is the point of testing so many basic colors in exactly the same way?

We can figure out how to make a nice system for parsing arguments into the plugin later.

Comment 4

[(not currently active) Ted Mielczarek]

(In reply to comment #3)
> What is the point of testing so many basic colors in exactly the same way?

Mostly it's to test that my hex color parsing code works. :) Never hurts to have sanity checks.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #2)
> dbaron: is there a way to include these in the main reftest.list only if
> plugins are enabled? Alternately, is there a way to make them all
> fails-if(plugins are disabled)?

fails-if(MOZ_PLUGINS!="1"), I'd think.  That does add another dependency on the build, though...

Comment 6

[(not currently active) Ted Mielczarek]

We could probably fake that in JS by looking for something that's only present when plugins are compiled, right? Actually, we could be even more specific, and see if we have the actual test plugin available, and expose that to the reftest manifest parser.

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: fix nits, simplify reftests

Ok, fixed review comments, and combined the multi-color reftests into one single reftest testing 4 different colors. Ideally I'd have a few more tests in there to thoroughly test the hex color parsing code, but this is a good start.

Comment 8

[Josh Aas]

Comment on attachment 357344 [details] [diff] [review]
fix nits, simplify reftests

>+      byte[0] = color[len-2];
>+      byte[1] = color[len-1];
...
>+      byte[1] = color[len-1];

Few more cases of not having a space around operators.

Looks good to me, thanks!

Comment 9

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/86c69da673eb

Comment 10

[Michael Ventnor]

Ted, will you check in bug 469831 too?

Comment 11

[(not currently active) Ted Mielczarek]

Sure. You really don't have commit access yet? For shame!

Comment 12

[Michael Ventnor]

Yeah, everyone's saying I should get it. I'd like to become more experienced in hg first, though: http://quotes.burntelectrons.org/3853 :)

Comment 13

[Mark Banner (:standard8)]

I just checked in a bustage fix for TB trunk: http://hg.mozilla.org/mozilla-central/rev/184eadf4e185 (add missing prtypes.h header).

Comment 14

[(not currently active) Ted Mielczarek]

(In reply to comment #13)

Thanks, sorry I missed that!