improve test plugin (libnptest.so)

RESOLVED FIXED in mozilla17

Status

()

Core
Plug-ins
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

unspecified
mozilla17
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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

Updated

5 years ago
Blocks: 743429
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?
We need multiple plugins to test the case where, for instance, a page with both java and flash only loads the flash content.
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.
Created attachment 623241 [details] [diff] [review]
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.
Attachment #623241 - Flags: feedback?(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.
Attachment #623241 - Flags: feedback?(ted.mielczarek) → feedback-
Created attachment 624913 [details] [diff] [review]
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.
Attachment #623241 - Attachment is obsolete: true
Attachment #624913 - Flags: feedback?(ted.mielczarek)
Oh, I see. Alternately, you could just have nptest.cpp get compiled twice with different defines. Either way is fairly straightforward.
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.
Attachment #624913 - Flags: feedback?(ted.mielczarek) → feedback+

Comment 9

5 years ago
I would like to review this whenever it is ready.
Created attachment 626198 [details] [diff] [review]
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.
Attachment #624913 - Attachment is obsolete: true
Attachment #626198 - Flags: review?(joshmoz)

Updated

5 years ago
Attachment #626198 - Flags: review?(joshmoz) → review+

Comment 11

5 years ago
Please get a review from bsmedberg too. Thanks.
Created attachment 650231 [details] [diff] [review]
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.
Assignee: nobody → dkeeler
Attachment #626198 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #650231 - Flags: review?(benjamin)
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 ;-)
Attachment #650231 - Flags: review?(benjamin) → review+
Created attachment 651462 [details] [diff] [review]
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
Attachment #650231 - Attachment is obsolete: true
Attachment #651462 - Flags: review+
Well, that looked good, so marking checkin-needed.
Keywords: checkin-needed
(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
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1230521b11d3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.