Exclude more plugins code for Android

Exclude more plugins code for Android

This CL fixes all plugin related link errors during component build of content_shell_apk
BUG=158821, 162667

[jam, 2012-12-17 17:54:52]

https://codereview.chromium.org/11577031/diff/1/content/browser/plugin_servic...
File content/browser/plugin_service_impl.cc (right):

https://codereview.chromium.org/11577031/diff/1/content/browser/plugin_servic...
content/browser/plugin_service_impl.cc:267: #if defined(ENABLE_PLUGINS)
what would be the point of including this file when ENABLE_PLUGINS isn't
defined, and you already ifdef out a bunch of calls to it. i.e. what i'm trying
to get at is to ask do you really need to include this file in that case?

[nilesh, 2012-12-17 18:52:18]

On 2012/12/17 17:54:52, John Abd-El-Malek wrote:
>
https://codereview.chromium.org/11577031/diff/1/content/browser/plugin_servic...
> File content/browser/plugin_service_impl.cc (right):
> 
>
https://codereview.chromium.org/11577031/diff/1/content/browser/plugin_servic...
> content/browser/plugin_service_impl.cc:267: #if defined(ENABLE_PLUGINS)
> what would be the point of including this file when ENABLE_PLUGINS isn't
> defined, and you already ifdef out a bunch of calls to it. i.e. what i'm
trying
> to get at is to ask do you really need to include this file in that case?

Removing PluginService altogether creates more ifdefs. In this CL I am not
actively removing calls to PluginService. The code in browser_process_impl is
ifdefed out mainly because it needs PepperPluginInfo::ToWebPluginInfo (which is
defined in the excluded class pepper_plugin_registry.cc)

There are 3 options:
1. Modify code in plugin_service_impl to handle the plugins disabled case. (this
CL)
2. Provide an alternate implementation (a stub) which handles the plugins
disabled case. (https://codereview.chromium.org/11507017/)
3. Remove PluginService all together: (CL
https://codereview.chromium.org/11615002/  not complete)
  I abandoned this approach as it required adding the most ifdefs.

[jam, 2012-12-17 21:15:02]

On 2012/12/17 18:52:18, nileshagrawal1 wrote:
> On 2012/12/17 17:54:52, John Abd-El-Malek wrote:
> >
>
https://codereview.chromium.org/11577031/diff/1/content/browser/plugin_servic...
> > File content/browser/plugin_service_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/11577031/diff/1/content/browser/plugin_servic...
> > content/browser/plugin_service_impl.cc:267: #if defined(ENABLE_PLUGINS)
> > what would be the point of including this file when ENABLE_PLUGINS isn't
> > defined, and you already ifdef out a bunch of calls to it. i.e. what i'm
> trying
> > to get at is to ask do you really need to include this file in that case?
> 
> Removing PluginService altogether creates more ifdefs. In this CL I am not
> actively removing calls to PluginService. The code in browser_process_impl is
> ifdefed out mainly because it needs PepperPluginInfo::ToWebPluginInfo (which
is
> defined in the excluded class pepper_plugin_registry.cc)
> 
> There are 3 options:
> 1. Modify code in plugin_service_impl to handle the plugins disabled case.
(this
> CL)
> 2. Provide an alternate implementation (a stub) which handles the plugins
> disabled case. (https://codereview.chromium.org/11507017/)
> 3. Remove PluginService all together: (CL
> https://codereview.chromium.org/11615002/  not complete)
>   I abandoned this approach as it required adding the most ifdefs.

I went through this change and that one, and it it seems that it's only
marginally more ifdefs (i.e. if one considers that plugin_service_impl.cc now
wouldn't need all the ifdefs). It's also more maintainable and easier to
understand, i.e. ENABLE_PLUGIN really means plugins are enabled, not some of the
plugin code is compiled in while other isn't. So I would say that approach is
better, since it seems that if plugins are disabled no one should be talking to
PluginService.

[nilesh, 2012-12-18 23:26:41]

On 2012/12/17 21:15:02, John Abd-El-Malek wrote:
> On 2012/12/17 18:52:18, nileshagrawal1 wrote:
> > On 2012/12/17 17:54:52, John Abd-El-Malek wrote:
> > >
> >
>
https://codereview.chromium.org/11577031/diff/1/content/browser/plugin_servic...
> > > File content/browser/plugin_service_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/11577031/diff/1/content/browser/plugin_servic...
> > > content/browser/plugin_service_impl.cc:267: #if defined(ENABLE_PLUGINS)
> > > what would be the point of including this file when ENABLE_PLUGINS isn't
> > > defined, and you already ifdef out a bunch of calls to it. i.e. what i'm
> > trying
> > > to get at is to ask do you really need to include this file in that case?
> > 
> > Removing PluginService altogether creates more ifdefs. In this CL I am not
> > actively removing calls to PluginService. The code in browser_process_impl
is
> > ifdefed out mainly because it needs PepperPluginInfo::ToWebPluginInfo (which
> is
> > defined in the excluded class pepper_plugin_registry.cc)
> > 
> > There are 3 options:
> > 1. Modify code in plugin_service_impl to handle the plugins disabled case.
> (this
> > CL)
> > 2. Provide an alternate implementation (a stub) which handles the plugins
> > disabled case. (https://codereview.chromium.org/11507017/)
> > 3. Remove PluginService all together: (CL
> > https://codereview.chromium.org/11615002/  not complete)
> >   I abandoned this approach as it required adding the most ifdefs.
> 
> I went through this change and that one, and it it seems that it's only
> marginally more ifdefs (i.e. if one considers that plugin_service_impl.cc now
> wouldn't need all the ifdefs). It's also more maintainable and easier to
> understand, i.e. ENABLE_PLUGIN really means plugins are enabled, not some of
the
> plugin code is compiled in while other isn't. So I would say that approach is
> better, since it seems that if plugins are disabled no one should be talking
to
> PluginService.

Thanks. Closing this issue in favor of https://codereview.chromium.org/11615002/