749257 - improve test plugin (libnptest.so)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Particularly for click-to-play, it would be nice to have a more versatile test plugin. For example, we need to be able to test handling different mime types, having multiple different plugins, etc.

Comment 1

[Benjamin Smedberg]

We typically just add whatever functions to the testplugin you need for testing. You can pretty easily add additional MIME types to the same plugin. It would be more work and we have avoided adding a new plugin mainly because getting that all installed into the test infrastructure is more complicated. Why do you need multiple plugins?

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

We need multiple plugins to test the case where, for instance, a page with both java and flash only loads the flash content.

Comment 3

[(not currently active) Ted Mielczarek]

I think some of my suggestions from bug 743429 would be the easiest way to approach this. We could pretty easily make a few copies of the test plugin and have them register for different mime types based on their filenames.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: prototype

I don't think having a plugin inspect its own filename is going to be the easiest way to do this. (Note that you can't just trivially inspect /proc/self/exe, because when calling NP_GetMIMEDescription on mac/linux, this gives firefox's name. For windows, you have to define the mime type ahead of time in the dll resource file, apparently.)

In an effort to not copy the test plugin wholesale, I wrote a patch that essentially re-links the test plugin but redefines some key symbols (mime type, name, description) and installs a second test plugin. This way, the plugins behave the exact same way and code changes are automatically kept in sync.

It would be great to have some feedback as to whether or not this is the kind of thing we want to do here.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 623241 [details] [diff] [review]
prototype

Review of attachment 623241 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with the concept here, but the execution is a bit messy.

::: dom/plugins/test/testplugin/nptest.cpp
@@ +68,5 @@
> +const char *sPluginDescription = "Plug-in for testing purposes.\xE2\x84\xA2 "  \
> +  "(\xe0\xa4\xb9\xe0\xa4\xbf\xe0\xa4\xa8\xe0\xa5\x8d\xe0\xa4\xa6\xe0\xa5\x80 " \
> +  "\xe4\xb8\xad\xe6\x96\x87 "                                                  \
> +  "\xd8\xa7\xd9\x84\xd8\xb9\xd8\xb1\xd8\xa8\xd9\x8a\xd8\xa9)";
> +const char *sSecondPluginDescription = "Second plug-in for testing purposes.";

I think the sanest way to do this would just be to use preprocessor defines. You could either:
a) Just do DEFINES += -DPLUGIN_NAME="Second Test Plug-in" ... in the Makefile, and change the code here to be #ifndef PLUGIN_NAME #define PLUGIN_NAME ...
b) Add some other define like -DUSE_ALTERNATE_PLUGIN_NAME and ifdef on that.

::: dom/plugins/test/testplugin/secondplugin/Makefile.in
@@ +117,5 @@
> +endif
> +
> +EXTRA_DSO_LDOPTS += -Wl,--defsym=sMimeDescription=sSecondMimeDesc \
> +                    -Wl,--defsym=sPluginName=sSecondPluginName \
> +                    -Wl,--defsym=sPluginDescription=sSecondPluginDescription

This is kind of horrible, and this won't work on Windows.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: prototype v2

Yeah, defsym was a bad idea.
The problem with using -DPLUGIN_NAME or something is that each source file only gets compiled once, so specifying a different PLUGIN_NAME later has no effect. My solution this time is to just have two different files that define the name/description/mimetype - one in testplugin and the other in secondplugin. When linking the final libnptest.so/libnpsecontest.so, they use different object files for those strings and thus end up with the right values.

Comment 7

[(not currently active) Ted Mielczarek]

Oh, I see. Alternately, you could just have nptest.cpp get compiled twice with different defines. Either way is fairly straightforward.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 624913 [details] [diff] [review]
prototype v2

Review of attachment 624913 [details] [diff] [review]:
-----------------------------------------------------------------

Concept seems fine to me, might want Josh or someone who's actually a plugin peer to sign off on it too.

::: dom/plugins/test/testplugin/nptest.cpp
@@ +611,5 @@
>  NP_EXPORT(NPError)
>  NP_GetValue(void* future, NPPVariable aVariable, void* aValue) {
>    switch (aVariable) {
>      case NPPVpluginNameString:
> +      *((char**)aValue) = (char *)sPluginName;

The casting here is a little weird. I'm surprised you can get away with this without a const_cast, honestly.

::: dom/plugins/test/testplugin/secondplugin/Makefile.in
@@ +32,5 @@
> +# and other provisions required by the GPL or the LGPL. If you do not delete
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****

You want to use the new MPL2 boilerplate for new files, FYI:
http://www.mozilla.org/MPL/headers/

@@ +114,5 @@
> +ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
> +CXXFLAGS        += $(MOZ_GTK2_CFLAGS)
> +CFLAGS          += $(MOZ_GTK2_CFLAGS)
> +EXTRA_DSO_LDOPTS += $(MOZ_GTK2_LIBS) $(XLDFLAGS) $(XLIBS) $(XEXT_LIBS)
> +endif

Kind of sucks to duplicate all this stuff. Maybe you could factor most of the makefile out into a testplugin.mk file and include it from both locations? You could change the relative paths to be $(topsrcdir)/dom/plugins/test/testplugin/foo if you had to.

Comment 9

[Josh Aas]

I would like to review this whenever it is ready.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

Refactored the makefile, made those casts a bit more clear, and added a test. I think this is ready for an actual review, so r? joshmoz.

Comment 11

[Josh Aas]

Please get a review from bsmedberg too. Thanks.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v2

It looks like I had to rebase this. Anyway, we're getting to the point where we actually want this for properly testing click-to-play plugins (specifically the ui), so I'm asking for review from bsmedberg.

Comment 13

[Benjamin Smedberg]

Comment on attachment 650231 [details] [diff] [review]
patch v2

In test_secondPlugin.html don't you also want to test that you can instantiate the second plugin using the other MIME type?

This is kinda unfortunate, but most our testing is ugly ;-)

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v3

Yeah, actually instantiating the plugin seems like a good idea.
Carrying over r+ for the minor change.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=a705f3abd19e

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Well, that looked good, so marking checkin-needed.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

(In reply to David Keeler from comment #14)
> Try run: https://tbpl.mozilla.org/?tree=Try&rev=a705f3abd19e

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1230521b11d3

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1230521b11d3