Add support for dynamically loading internal plugins

Add support for dynamically registering and loading plugins built as part of the application.  For instance, consider a medical viewing application where the complete user interface is an embedded browser window.  An internal plugin is embedded in the browser window for viewing medical scan images.  The plugin depends on the medical viewing application (cannot function independently), and so we compile both the viewing application and the plugin into a single executable.

Example C++ usage:

PluginVersionInfo info;
info.path = L"medical_viewing_plugin";
info.product_name = L"Medical Viewing Plugin";
info.file_description = L"Medical viewing application plugin";
info.file_version = L"1, 0, 0, 1";
info.mime_types =  L"application/x-medical-viewing-plugin";
info.file_extensions = L"*";
info.np_getentrypoints = my_np_getentrypoints;
info.np_initialize = my_np_initialize;
info.np_shutdown = my_np_shutdown;

// Register the plugin information
NPAPI::PluginLib::RegisterInternalPlugin(info);

// Load the plugin so that it's available to the client
NPAPI::PluginList::Singleton()->LoadPlugin(info.path);

Example HTML usage:

<embed  type="application/x-medical-viewing-plugin" width=600 height=600>

[Marshall Greenblatt, 2009-01-28 17:22:55]

Please review this patch related to the chromium-dev thread @

http://groups.google.com/group/chromium-dev/browse_thread/thread/e72a2dcd5df1...

jam: primary reviewer
avi: FYI

[Dean McNamee, 2009-01-28 17:34:55]

http://codereview.chromium.org/19625/diff/1/3
File webkit/glue/plugins/plugin_lib_win.cc (right):

http://codereview.chromium.org/19625/diff/1/3#newcode92
Line 92: static std::vector<WebPluginInfoInternal> g_dynamic_internal_plugins;
We have a rule against global static objects like this.  It will create a static
constructor that will be run on startup.  I recently fixed a few problems like
this in PluginList.  Using LazyInstance can be a bit awkward, it's better to try
to dangle it off the singleton or something if you can.

[Marshall Greenblatt, 2009-01-28 17:40:55]

That was my first thought as well, but we don't currently have a singleton for
PluginLib.  Should we create and use a singleton for PluginLib, or is there a
better approach in this particular case?

[Dean McNamee, 2009-01-28 17:42:18]

You can just use LazyInstance (see PluginList for an example).  It's a bit
awkward (my fault), but it will do.  base/lazy_instance.h

On 2009/01/28 17:40:55, Marshall Greenblatt wrote:
> That was my first thought as well, but we don't currently have a singleton for
> PluginLib.  Should we create and use a singleton for PluginLib, or is there a
> better approach in this particular case?

[jam, 2009-01-28 18:06:28]

http://codereview.chromium.org/19625/diff/1/4
File webkit/glue/plugins/plugin_lib.h (right):

http://codereview.chromium.org/19625/diff/1/4#newcode58
Line 58: static void RegisterWebPluginInfo(const WebPluginInfo& info,
you're really registering more than WebPluginInfo (since it includes function
pointers), so this should be called RegisterInternalPlugin.

http://codereview.chromium.org/19625/diff/1/3
File webkit/glue/plugins/plugin_lib_win.cc (right):

http://codereview.chromium.org/19625/diff/1/3#newcode81
Line 81: struct WebPluginInfoInternal
I don't think we want to add yet another structure to hold plugin information
(since we already have WebPluginInfo and PluginVersionInfo.  What you really
want to do is register an PluginVersionInfo, not a WebPluginInfo+FPs.  It would
then be held in a vector along with the other internal plugins.  This stuff
should be consolidated into plugin_lib.cc, and the platform specific code just
has the option to add more internal plugins (i.e. now windows code would add
activex + gears).  makes sense?

http://codereview.chromium.org/19625/diff/1/2
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/19625/diff/1/2#newcode81
Line 81: void UnloadPlugin(const FilePath& filename);
Why do you need this?  Also, the name is misleading.  Since PluginLib is
refcounted, just removing it from plugin_ gives no guarantee that it'll be
unloaded.

[Marshall Greenblatt, 2009-01-28 18:27:33]

PluginVersionInfo uses wchar_t* instead of std::wstring, which makes it unclear
as to who is responsible for allocating/freeing the strings.  Can I change the
wchar_t* to std::wstring?

[jam, 2009-01-28 18:32:50]

On 2009/01/28 18:27:33, Marshall Greenblatt wrote:
> PluginVersionInfo uses wchar_t* instead of std::wstring, which makes it
unclear
> as to who is responsible for allocating/freeing the strings.  Can I change the
> wchar_t* to std::wstring?

We actually made it wchar_t* on purpose to avoid global static objects (since
otherwise the internal plugins would create static std::strings).  I think here
you can add a comment that the passed in strings should be valid for the
duration of PluginList..

[Marshall Greenblatt, 2009-01-28 18:41:25]

> We actually made it wchar_t* on purpose to avoid global static objects (since
> otherwise the internal plugins would create static std::strings).  I think
here
> you can add a comment that the passed in strings should be valid for the
> duration of PluginList..

If we are eliminating g_internal_plugins and using the new
RegisterInternalPlugin() method to register the plugins currently included in
g_internal_plugins, then we will be eliminating the global creation of the
static std::wstring instances as well.  Therefore, we could go back to using
std::wstring in PluginVersionInfo.  Does that make sense?

[Marshall Greenblatt, 2009-01-28 19:28:05]

Please review the updated patch.

1. Use LazyInstance for the dynamic vector of PluginVersionInfo in
plugin_lib_win.cc.
2. Eliminate the global g_internal_plugins array in plugin_lib_win.cc and use
RegisterInternalPlugin() to add them in the dynamic vector instead.
3. Change the strings in PluginVersionInfo from wchar_t* to std::wstring since
we're no longer using them as static globals.

[jam, 2009-01-28 19:34:22]

On 2009/01/28 19:28:05, Marshall Greenblatt wrote:
> Please review the updated patch.
> 
> 1. Use LazyInstance for the dynamic vector of PluginVersionInfo in
> plugin_lib_win.cc.
> 2. Eliminate the global g_internal_plugins array in plugin_lib_win.cc and use
> RegisterInternalPlugin() to add them in the dynamic vector instead.
> 3. Change the strings in PluginVersionInfo from wchar_t* to std::wstring since
> we're no longer using them as static globals.

The code still has std::strings getting constructed.
When you do

const PluginVersionInfo builtin_plugins[] = {
       {
         kActiveXShimFileName,


This will cause an std::wstring to be created when the program loads, which is
what we're trying to avoid.

[Marshall Greenblatt, 2009-01-28 19:55:06]

On 2009/01/28 19:34:22, John Abd-El-Malek wrote:
> 
> The code still has std::strings getting constructed.
> When you do
> 
> const PluginVersionInfo builtin_plugins[] = {
>        {
>          kActiveXShimFileName,
> 
> 
> This will cause an std::wstring to be created when the program loads, which is
> what we're trying to avoid.

Are you sure? My understanding is that the std::wstring constructor won't be
called until the structure initialization line is reached during method
execution (at least with MSVS2005).  Consider the following trivial example:

class MyClass
{
public:
  MyClass(const wchar_t* str)
  {
    wprintf(L"In constructor for %s\n", str);
  }
};

struct MyStruct
{
  MyClass foo;
};

int _tmain(int argc, _TCHAR* argv[])
{
  wprintf(L"Main start\n");
  const MyStruct ms = { L"Foo" };
  wprintf(L"Main end\n");

  getchar();
  return 0;
}

The output you see will be:

Main start
In constructor for Foo
Main end

[Marshall Greenblatt, 2009-01-28 20:07:12]

On Wed, Jan 28, 2009 at 2:28 PM, John Abd-El-Malek <jam@chromium.org> wrote:

    On Wed, Jan 28, 2009 at 10:41 AM,  <magreenblatt@gmail.com> wrote:
    >
    >> We actually made it wchar_t* on purpose to avoid global static objects
    >
    > (since
    >>
    >> otherwise the internal plugins would create static std::strings).  I
    >
    > think here
    >>
    >> you can add a comment that the passed in strings should be valid for
    >
    > the
    >>
    >> duration of PluginList..
    >
    > If we are eliminating g_internal_plugins and using the new
    > RegisterInternalPlugin() method to register the plugins currently
    > included in g_internal_plugins, then we will be eliminating the global
    > creation of the static std::wstring instances as well.  Therefore, we
    > could go back to using std::wstring in PluginVersionInfo.  Does that
    > make sense?

    not really, because we'll still have const PluginVersion Info = { blah
    } in the code, which will cause static std::strings to be created...

    I think it might just be quicker for me to send you a changelist with
    this stuff instead of trying to get you to code up what's in my head.
    Let me send it to you and we can see if it's sufficient.
    >
    > http://codereview.chromium.org/19625
    >

Go ahead and do your implementation.  My personal interest is more with the
functionality than the particular approach :-).