Fix registration of internal plugins broken by rev 23501

Due to the plugin_list changes in rev 23501 internal plugins registered via a call to PluginList::RegisterInternalPlugin() are no longer being added to the plugin list when LoadPlugins() is called.  Fix the problem by adding the internal plugins in LoadPlugins().

[Marshall Greenblatt, 2009-08-20 01:18:28]

Please review this change required for the Chromium Embedded Framework.

[jam, 2009-08-20 01:33:48]

Looking at http://codereview.chromium.org/164305/diff/2001/2021, it looks like
the "return" statement was accidentally taken out (my bad).  Won't putting it
back just fix the regression?

[jam, 2009-08-20 01:58:51]

btw I put in the missing line in r23794.  Can you sync and see if you still
have the problem?

On Wed, Aug 19, 2009 at 6:33 PM, <jam@chromium.org> wrote:

> Looking at http://codereview.chromium.org/164305/diff/2001/2021, it
> looks like the "return" statement was accidentally taken out (my bad).
> Won't putting it back just fix the regression?
>
>
> http://codereview.chromium.org/173107
>

[Marshall Greenblatt, 2009-08-20 13:37:36]

On 2009/08/20 01:58:51, John Abd-El-Malek wrote:
> btw I put in the missing line in r23794.  Can you sync and see if you still
> have the problem?

I still have the problem because the current LoadPlugins() logic clears and
rebuilds the plugins_ list ignoring any values in the internal_plugins_ list. 
Previously, I could call LoadPlugin() directly for my internal plugin and the
value would survive the call to LoadPlugins(). With the code as it currently
exists, any plugins that I register via a call to RegisterInternalPlugin() never
get added to the plugins_ list.

> 
> On Wed, Aug 19, 2009 at 6:33 PM, <mailto:jam@chromium.org> wrote:
> 
> > Looking at http://codereview.chromium.org/164305/diff/2001/2021, it
> > looks like the "return" statement was accidentally taken out (my bad).
> > Won't putting it back just fix the regression?
> >
> >
> > http://codereview.chromium.org/173107
> >
>

[Marshall Greenblatt, 2009-08-20 13:54:01]

I've updated the change set to reflect the logic lost by the missing 'return'
statement.

I should also mention that the modification suggested by this change set is a
bit nicer than the original implementation, because with this change it's no
longer necessary for the consumer to call LoadPlugin() explicitly after calling
RegisterInternalPlugin().

[jam, 2009-08-20 17:40:35]

On Thu, Aug 20, 2009 at 6:37 AM, <magreenblatt@gmail.com> wrote:

> On 2009/08/20 01:58:51, John Abd-El-Malek wrote:
>
>> btw I put in the missing line in r23794.  Can you sync and see if you
>>
> still
>
>> have the problem?
>>
>
> I still have the problem because the current LoadPlugins() logic clears
> and rebuilds the plugins_ list ignoring any values in the
> internal_plugins_ list.  Previously, I could call LoadPlugin() directly
> for my internal plugin and the value would survive the call to
> LoadPlugins().


I'm confused as to what exactly in
http://codereview.chromium.org/164305<http://codereview.chromium.org/164305/s...
changed
this behavior?


> With the code as it currently exists, any plugins that I
> register via a call to RegisterInternalPlugin() never get added to the
> plugins_ list.
>
>
>
>  On Wed, Aug 19, 2009 at 6:33 PM, <mailto:jam@chromium.org> wrote:
>>
>
>  > Looking at http://codereview.chromium.org/164305/diff/2001/2021, it
>> > looks like the "return" statement was accidentally taken out (my
>>
> bad).
>
>> > Won't putting it back just fix the regression?
>> >
>> >
>> > http://codereview.chromium.org/173107
>> >
>>
>
>
>
>
> http://codereview.chromium.org/173107
>

[Marshall Greenblatt, 2009-08-20 17:45:13]

On 2009/08/20 17:40:35, John Abd-El-Malek wrote:

> I'm confused as to what exactly in
>
http://codereview.chromium.org/164305%3Chttp://codereview.chromium.org/164305...>
> changed
> this behavior?

The PluginList::LoadPlugins() implementation in your change replaces the
plugins_ list (plugins_ = new_plugins) where previously it appended to the
plugins_ list.

In other words, the plugins_ list would already have one object in it (my
internal plugin) before the PluginList::LoadPlugins() method was called.

[Marshall Greenblatt, 2009-08-27 14:05:33]

On 2009/08/20 17:45:13, Marshall Greenblatt wrote:
> On 2009/08/20 17:40:35, John Abd-El-Malek wrote:
> 
> > I'm confused as to what exactly in
> >
>
http://codereview.chromium.org/164305%253Chttp://codereview.chromium.org/1643...>
> > changed
> > this behavior?
> 
> The PluginList::LoadPlugins() implementation in your change replaces the
> plugins_ list (plugins_ = new_plugins) where previously it appended to the
> plugins_ list.
> 
> In other words, the plugins_ list would already have one object in it (my
> internal plugin) before the PluginList::LoadPlugins() method was called.

Did this answer your question John?

[jam, 2009-08-27 23:42:56]

sorry for the reply, I missed this message.

On Thu, Aug 27, 2009 at 7:05 AM, <magreenblatt@gmail.com> wrote:

> On 2009/08/20 17:45:13, Marshall Greenblatt wrote:
>
>> On 2009/08/20 17:40:35, John Abd-El-Malek wrote:
>>
>
>  > I'm confused as to what exactly in
>> >
>>
>
>
>
http://codereview.chromium.org/164305%253Chttp://codereview.chromium.org/1643...
> >
>
>> > changed
>> > this behavior?
>>
>
>  The PluginList::LoadPlugins() implementation in your change replaces
>>
> the
>
>> plugins_ list (plugins_ = new_plugins) where previously it appended to
>>
> the
>
>> plugins_ list.
>>
>
But the previous code also called plugins_.clear(); in the beginning of
LoadPlugins, so shouldn't be the same behavior?  LoadInternalPlugins
previously had access to plugins_ which contains everything added since the
clear, but not it has access to the plugins pointer which should have the
same stuff, no?


>  In other words, the plugins_ list would already have one object in it
>>
> (my
>
>> internal plugin) before the PluginList::LoadPlugins() method was
>>
> called.
>
> Did this answer your question John?
>
>
> http://codereview.chromium.org/173107
>

[Marshall Greenblatt, 2009-08-28 13:16:18]

> But the previous code also called plugins_.clear(); in the beginning of
> LoadPlugins, so shouldn't be the same behavior?  LoadInternalPlugins
> previously had access to plugins_ which contains everything added since the
> clear, but not it has access to the plugins pointer which should have the
> same stuff, no?

You're right, I was reporting the symptom and not the cause.  After further
examination here's what I think changed:

In the previous revision LoadPlugins(false) was called by
PluginList::Singleton(), which meant that it was executed before I called
LoadPlugin for my internal plugin.  In the new revision LoadPlugins(false) is
called by PluginList::FindPlugin(), which happens well after I called LoadPlugin
for my internal plugin, resulting in my internal plugin being dropped from the
plugins_ list.

[jam, 2009-08-28 22:00:07]

On Fri, Aug 28, 2009 at 6:16 AM, <magreenblatt@gmail.com> wrote:

> But the previous code also called plugins_.clear(); in the beginning
>>
> of
>
>> LoadPlugins, so shouldn't be the same behavior?  LoadInternalPlugins
>> previously had access to plugins_ which contains everything added
>>
> since the
>
>> clear, but not it has access to the plugins pointer which should have
>>
> the
>
>> same stuff, no?
>>
>
> You're right, I was reporting the symptom and not the cause.  After
> further examination here's what I think changed:
>
> In the previous revision LoadPlugins(false) was called by
> PluginList::Singleton(), which meant that it was executed before I
> called LoadPlugin for my internal plugin.  In the new revision
> LoadPlugins(false) is called by PluginList::FindPlugin(), which happens
> well after I called LoadPlugin for my internal plugin, resulting in my
> internal plugin being dropped from the plugins_ list.


So it seems that things were broken before anyways, if any page does
plugins.refresh(), since that would reload the plugin list and get rid of
the plugins that you called LoadPlugin on.  Correct?

Did you run all the tests?  Because it seems that this change will break
layout tests, which specifically don't want the default plugin to be loaded.
 i.e. now LoadPlugins does

if (webkit_glue::IsDefaultPluginEnabled())
    LoadPlugin(FilePath(kDefaultPluginLibraryName), &new_plugins);

But with your change, it'll always get added.


>
> http://codereview.chromium.org/173107
>

[Marshall Greenblatt, 2009-08-31 13:27:17]

> So it seems that things were broken before anyways, if any page does
> plugins.refresh(), since that would reload the plugin list and get rid of
> the plugins that you called LoadPlugin on.  Correct?

Yes, that's correct.

> 
> Did you run all the tests?  Because it seems that this change will break
> layout tests, which specifically don't want the default plugin to be loaded.
>  i.e. now LoadPlugins does
> 
> if (webkit_glue::IsDefaultPluginEnabled())
>     LoadPlugin(FilePath(kDefaultPluginLibraryName), &new_plugins);
> 
> But with your change, it'll always get added.

Good point.  It looks like trying to use the existing internal_plugins_ list may
be adding an undesirable level of complexity.  How would you feel if
RegisterInternalPlugin stored entries in a list separate from internal_plugins_?
Perhaps we could call it builtin_plugins_ and rename RegisterInternalPlugin to
RegisterBuiltinPlugin.

> 
> 
> >
> > http://codereview.chromium.org/173107
> >
>

[Marshall Greenblatt, 2009-09-17 00:59:50]

On 2009/08/31 13:27:17, Marshall Greenblatt wrote:
> It looks like trying to use the existing internal_plugins_ list may
> be adding an undesirable level of complexity.  How would you feel if
> RegisterInternalPlugin stored entries in a list separate from
internal_plugins_?
> Perhaps we could call it builtin_plugins_ and rename RegisterInternalPlugin to
> RegisterBuiltinPlugin.

Please review this patch that adds a new client_internal_plugins_ list populated
by the RegisterInternalPlugin() method.

[jam, 2009-09-17 01:49:38]

On 2009/09/17 00:59:50, Marshall Greenblatt wrote:
> On 2009/08/31 13:27:17, Marshall Greenblatt wrote:
> > It looks like trying to use the existing internal_plugins_ list may
> > be adding an undesirable level of complexity.  How would you feel if
> > RegisterInternalPlugin stored entries in a list separate from
> internal_plugins_?
> > Perhaps we could call it builtin_plugins_ and rename RegisterInternalPlugin
to
> > RegisterBuiltinPlugin.
> 
> Please review this patch that adds a new client_internal_plugins_ list
populated
> by the RegisterInternalPlugin() method.

I don't think adding yet another list of internal plugins, whose name will end
up confusing people because of its similarity, is what we want, especially since
it adds duplicate code.

Why not just do it like patchset 3, and have LoadInternalPlugins do the the
check for the default plugin while looping through?

[Marshall Greenblatt, 2009-09-17 02:29:34]

On 2009/09/17 01:49:38, John Abd-El-Malek wrote:
> I don't think adding yet another list of internal plugins, whose name will end
> up confusing people because of its similarity, is what we want, especially
since
> it adds duplicate code.
> 
> Why not just do it like patchset 3, and have LoadInternalPlugins do the the
> check for the default plugin while looping through?

The LoadInternalPlugins() method has gone away, but I've implemented equivolent
logic in LoadPlugins().  Let me know what you think.

[jam, 2009-09-17 19:29:18]

http://codereview.chromium.org/173107/diff/9001/9002
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/173107/diff/9001/9002#newcode163
Line 163: #if defined(OS_WIN)
1) why windows only?
2) this should be combined with lines 181-182, i.e. in this loop if the name is
kDefaultPluginLibraryName, then only call LoadPlugin if
webkit_glue::IsDefaultPluginEnabled() is true

[Marshall Greenblatt, 2009-09-17 19:46:38]

On 2009/09/17 19:29:18, John Abd-El-Malek wrote:
> http://codereview.chromium.org/173107/diff/9001/9002
> File webkit/glue/plugins/plugin_list.cc (right):
> 
> http://codereview.chromium.org/173107/diff/9001/9002#newcode163
> Line 163: #if defined(OS_WIN)
> 1) why windows only?

The default plugin is only added to the internal plugins list for OS_WIN (line
119).

> 2) this should be combined with lines 181-182, i.e. in this loop if the name
is
> kDefaultPluginLibraryName, then only call LoadPlugin if
> webkit_glue::IsDefaultPluginEnabled() is true

Two reasons:

a. I think internal plugins should be loaded before "discovered" plugins. For
instance, it's possible that an application would provide its own internal
plugin for a common plugin type and the internal plugin should be given have
preference.

b. The default plugin always needs to be last.  Therefore, unless
PluginList::RegisterInternalPlugin() is going to enforce this requirement the
logic will still need to loop over all plugins in the internal plugins list and
then come back to the default plugin.

[Marshall Greenblatt, 2009-09-17 19:48:12]

On 2009/09/17 19:46:38, Marshall Greenblatt wrote:
> Two reasons:

Two reasons for my implementation approach, that is :-)

> 
> a. I think internal plugins should be loaded before "discovered" plugins. For
> instance, it's possible that an application would provide its own internal
> plugin for a common plugin type and the internal plugin should be given have
> preference.
> 
> b. The default plugin always needs to be last.  Therefore, unless
> PluginList::RegisterInternalPlugin() is going to enforce this requirement the
> logic will still need to loop over all plugins in the internal plugins list
and
> then come back to the default plugin.

[jam, 2009-09-17 21:30:42]

On 2009/09/17 19:46:38, Marshall Greenblatt wrote:
> On 2009/09/17 19:29:18, John Abd-El-Malek wrote:
> > http://codereview.chromium.org/173107/diff/9001/9002
> > File webkit/glue/plugins/plugin_list.cc (right):
> > 
> > http://codereview.chromium.org/173107/diff/9001/9002#newcode163
> > Line 163: #if defined(OS_WIN)
> > 1) why windows only?
> 
> The default plugin is only added to the internal plugins list for OS_WIN (line
> 119).

sure, but if it gets enabled for other platforms, it would be nice if only one
place needs to be updated.  it seems that this check causes no harm by not
having an ifdef for OS_WIN.
> 
> > 2) this should be combined with lines 181-182, i.e. in this loop if the name
> is
> > kDefaultPluginLibraryName, then only call LoadPlugin if
> > webkit_glue::IsDefaultPluginEnabled() is true
> 
> Two reasons:
> 
> a. I think internal plugins should be loaded before "discovered" plugins. For
> instance, it's possible that an application would provide its own internal
> plugin for a common plugin type and the internal plugin should be given have
> preference.
> 
> b. The default plugin always needs to be last.  Therefore, unless
> PluginList::RegisterInternalPlugin() is going to enforce this requirement the
> logic will still need to loop over all plugins in the internal plugins list
and
> then come back to the default plugin.

ok i'm convinced :)

[Marshall Greenblatt, 2009-09-17 23:09:09]

Please review the updated patch set that removes the OS_WIN check and adds some
additional source code comments.

[jam, 2009-09-18 03:23:14]

lgtm, thanks for persevering :)

[Marshall Greenblatt, 2009-09-18 13:42:51]

On 2009/09/18 03:23:14, John Abd-El-Malek wrote:
> lgtm, thanks for persevering :)

Not a problem.  After you land this change you'll probably want to talk with
gregoryd about his proposed changes for NaCL:
http://codereview.chromium.org/207025

He's adding a member variable to plugin_list that I think will no longer be
necessary.

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

On 2009/09/18 13:42:51, Marshall Greenblatt wrote:
> On 2009/09/18 03:23:14, John Abd-El-Malek wrote:
> > lgtm, thanks for persevering :)
> 
> Not a problem.  After you land this change you'll probably want to talk with
> gregoryd about his proposed changes for NaCL:
> http://codereview.chromium.org/207025
> 
> He's adding a member variable to plugin_list that I think will no longer be
> necessary.

good catch, thanks for letting me know, it'll simply that patch :)