Last Comment Bug 749257 - improve test plugin (libnptest.so)
: improve test plugin (libnptest.so)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: David Keeler [:keeler] (use needinfo?)
:
Mentors:
Depends on:
Blocks: 743429
  Show dependency treegraph
 
Reported: 2012-04-26 10:21 PDT by David Keeler [:keeler] (use needinfo?)
Modified: 2012-08-15 09:49 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
prototype (11.56 KB, patch)
2012-05-11 11:58 PDT, David Keeler [:keeler] (use needinfo?)
ted: feedback-
Details | Diff | Review
prototype v2 (11.69 KB, patch)
2012-05-17 14:59 PDT, David Keeler [:keeler] (use needinfo?)
ted: feedback+
Details | Diff | Review
patch (14.50 KB, patch)
2012-05-22 14:55 PDT, David Keeler [:keeler] (use needinfo?)
jaas: review+
Details | Diff | Review
patch v2 (15.25 KB, patch)
2012-08-08 11:51 PDT, David Keeler [:keeler] (use needinfo?)
benjamin: review+
Details | Diff | Review
patch v3 (15.53 KB, patch)
2012-08-13 10:47 PDT, David Keeler [:keeler] (use needinfo?)
dkeeler: review+
Details | Diff | Review

Description David Keeler [:keeler] (use needinfo?) 2012-04-26 10:21:43 PDT
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 [:bsmedberg] 2012-04-26 10:32:54 PDT
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 David Keeler [:keeler] (use needinfo?) 2012-04-26 10:36:18 PDT
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 Ted Mielczarek [:ted.mielczarek] 2012-04-26 11:14:04 PDT
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 David Keeler [:keeler] (use needinfo?) 2012-05-11 11:58:59 PDT
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.
Comment 5 Ted Mielczarek [:ted.mielczarek] 2012-05-17 11:57:54 PDT
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 David Keeler [:keeler] (use needinfo?) 2012-05-17 14:59:56 PDT
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.
Comment 7 Ted Mielczarek [:ted.mielczarek] 2012-05-17 16:52:14 PDT
Oh, I see. Alternately, you could just have nptest.cpp get compiled twice with different defines. Either way is fairly straightforward.
Comment 8 Ted Mielczarek [:ted.mielczarek] 2012-05-18 10:03:45 PDT
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 2012-05-18 10:14:20 PDT
I would like to review this whenever it is ready.
Comment 10 David Keeler [:keeler] (use needinfo?) 2012-05-22 14:55:07 PDT
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.
Comment 11 Josh Aas 2012-06-08 12:27:13 PDT
Please get a review from bsmedberg too. Thanks.
Comment 12 David Keeler [:keeler] (use needinfo?) 2012-08-08 11:51:37 PDT
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.
Comment 13 Benjamin Smedberg [:bsmedberg] 2012-08-13 10:17:01 PDT
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 David Keeler [:keeler] (use needinfo?) 2012-08-13 10:47:28 PDT
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
Comment 15 David Keeler [:keeler] (use needinfo?) 2012-08-14 10:17:55 PDT
Well, that looked good, so marking checkin-needed.
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-08-14 17:52:31 PDT
(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] 2012-08-15 09:49:20 PDT
https://hg.mozilla.org/mozilla-central/rev/1230521b11d3

Note You need to log in before you can comment on or make changes to this bug.