Exclude PluginService for builds with enable_plugins==0

Exclude PluginService for builds with enable_plugins==0
    
- Saves 140k in the final shared library (libchromeview.so)
- Fixes all plugin related link errors during component build of
content_shell_apk

BUG=158821, 162667
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174087

[nilesh, 2012-12-18 19:11:58]

Exclude PluginService for builds with enable_plugins==0
    
- Saves 120k in the final shared library (libchromeview.so)
- Fixes all plugin related link errors during component build of
content_shell_apk

BUG=158821, 162667

[nilesh, 2012-12-18 19:13:24]

Hi John, Please take a look.

[jam, 2012-12-19 01:55:15]

thanks, just a bunch of comments to exclude more files

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/chrome_plug...
File chrome/browser/chrome_plugin_service_filter.cc (right):

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/chrome_plug...
chrome/browser/chrome_plugin_service_filter.cc:153: #if defined(ENABLE_PLUGINS)
it seems that this whole file is unnecessary if ENABLE_PLUGINS isn't defined

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/plugins/plu...
File chrome/browser/plugins/plugin_prefs.cc (right):

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_prefs.cc:578: #if defined(ENABLE_PLUGINS)
ditto

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/ui/webui/ve...
File chrome/browser/ui/webui/version_handler.cc (right):

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_handler.cc:129: void
VersionHandler::OnGotPlugins(
seems that this function should be wrapped as well? are you just relying on the
linker not including this function?

https://codereview.chromium.org/11615002/diff/5001/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/11615002/diff/5001/chrome/chrome_tests_unit.g...
chrome/chrome_tests_unit.gypi:1832:
'browser/extensions/extension_service_unittest.cc', # Needs PluginService
this seems too wide? i.e. if someone disables plugins it doesn't mean that they
want to disable extensions. if there are tests in that file that depend on
extensions, then these should be wrapped with ENABLE_PLUGINS

https://codereview.chromium.org/11615002/diff/5001/content/browser/plugin_pro...
File content/browser/plugin_process_host.cc (right):

https://codereview.chromium.org/11615002/diff/5001/content/browser/plugin_pro...
content/browser/plugin_process_host.cc:313: #if defined(ENABLE_PLUGINS)
why is this file included if no plugins?

https://codereview.chromium.org/11615002/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/11615002/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_message_filter.cc:626: void
RenderMessageFilter::OnGetPlugins(
how about not sending this IPC altogether?

https://codereview.chromium.org/11615002/diff/5001/content/public/common/cont...
File content/public/common/content_client.cc (right):

https://codereview.chromium.org/11615002/diff/5001/content/public/common/cont...
content/public/common/content_client.cc:41: #if !defined(ENABLE_PLUGINS)
nit: it's easier to read this if it's written the other way around

https://codereview.chromium.org/11615002/diff/5001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11615002/diff/5001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:2513: #if !defined(ENABLE_PLUGINS)
nit: can you flip the order here so that it's easier to read?

[stuartmorgan, 2012-12-19 12:53:57]

Thanks for tackling this! It'll definitely be helpful for iOS too. A couple of
drive-by comments:

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:832: PluginFinder::GetInstance()->Init();
Are you sure the ordering here wasn't important?

https://codereview.chromium.org/11615002/diff/5001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11615002/diff/5001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:1886: return NULL;
Shouldn't these functions both have a NOTREACHED() as well?

[nilesh, 2012-12-19 21:07:44]

Addressed comments. Please take another look.

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:832: PluginFinder::GetInstance()->Init();
On 2012/12/19 12:53:58, stuartmorgan wrote:
> Are you sure the ordering here wasn't important?

Actually I am not sure. I am moving it back as it was just a cosmetic change.

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/chrome_plug...
File chrome/browser/chrome_plugin_service_filter.cc (right):

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/chrome_plug...
chrome/browser/chrome_plugin_service_filter.cc:153: #if defined(ENABLE_PLUGINS)
On 2012/12/19 01:55:15, John Abd-El-Malek wrote:
> it seems that this whole file is unnecessary if ENABLE_PLUGINS isn't defined

Done. Had to ifdef calls to ChromePluginServiceFilter::RegisterResourceContext
in profile_impl.cc, but I think this is cleaner now.

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/plugins/plu...
File chrome/browser/plugins/plugin_prefs.cc (right):

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_prefs.cc:578: #if defined(ENABLE_PLUGINS)
On 2012/12/19 01:55:15, John Abd-El-Malek wrote:
> ditto

Removed this file too.

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/ui/webui/ve...
File chrome/browser/ui/webui/version_handler.cc (right):

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_handler.cc:129: void
VersionHandler::OnGotPlugins(
Yes. I am relying on the linker not including this code in the final SO.

Let me know if you feel strongly about this. There is no way to find and
maintain all such instances, unless we have coverage via a component build.
Infact (atleast in the chrome layer), we actively rely on link time removal to
avoid adding too many ifdefs. 

OTOH, in the content layer, where we are trying to get a component build
working, we need to remove unknown references at compile time.

https://codereview.chromium.org/11615002/diff/5001/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/11615002/diff/5001/chrome/chrome_tests_unit.g...
chrome/chrome_tests_unit.gypi:1832:
'browser/extensions/extension_service_unittest.cc', # Needs PluginService
On 2012/12/19 01:55:15, John Abd-El-Malek wrote:
> this seems too wide? i.e. if someone disables plugins it doesn't mean that
they
> want to disable extensions. if there are tests in that file that depend on
> extensions, then these should be wrapped with ENABLE_PLUGINS

Done.

https://codereview.chromium.org/11615002/diff/5001/content/browser/plugin_pro...
File content/browser/plugin_process_host.cc (right):

https://codereview.chromium.org/11615002/diff/5001/content/browser/plugin_pro...
content/browser/plugin_process_host.cc:313: #if defined(ENABLE_PLUGINS)
On 2012/12/19 01:55:15, John Abd-El-Malek wrote:
> why is this file included if no plugins?

Removed.

https://codereview.chromium.org/11615002/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/11615002/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_message_filter.cc:626: void
RenderMessageFilter::OnGetPlugins(
On 2012/12/19 01:55:15, John Abd-El-Malek wrote:
> how about not sending this IPC altogether?

Done. Added code in content/renderer/renderer_webkitplatformsupport_impl.cc to
skip the IPC.

https://codereview.chromium.org/11615002/diff/5001/content/public/common/cont...
File content/public/common/content_client.cc (right):

https://codereview.chromium.org/11615002/diff/5001/content/public/common/cont...
content/public/common/content_client.cc:41: #if !defined(ENABLE_PLUGINS)
On 2012/12/19 01:55:15, John Abd-El-Malek wrote:
> nit: it's easier to read this if it's written the other way around

Done.

https://codereview.chromium.org/11615002/diff/5001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11615002/diff/5001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:1886: return NULL;
On 2012/12/19 12:53:58, stuartmorgan wrote:
> Shouldn't these functions both have a NOTREACHED() as well?

Added NOTREACHED here. 
RenderViewImpl::createPlugin is implementing Webkit::WebFrameClient which is
called and is expected to return NULL.

https://codereview.chromium.org/11615002/diff/5001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:2513: #if !defined(ENABLE_PLUGINS)
On 2012/12/19 01:55:15, John Abd-El-Malek wrote:
> nit: can you flip the order here so that it's easier to read?

Done.

[nilesh, 2012-12-19 21:13:25]

I apologize for the rebase in the latest patch.
It affects the patch set delta for:
extensions_service.cc
buffered_resource_handler.cc
render_process_host_impl.cc
render_view_impl.cc
and gyp files

[nilesh, 2012-12-19 21:19:09]

Adding owners for metrics and extensions.
yoz: extensions
isherman: metrics

Please take a look.

[jam, 2012-12-19 21:23:10]

(looking through patch now, some preliminary comments)

On 2012/12/19 21:19:09, nileshagrawal1 wrote:
> Adding owners for metrics and extensions.
> yoz: extensions
> isherman: metrics
> 
> Please take a look.

I'm an owner in chrome and content, so i'll review the whole change to save you
(and the other owners) time, esp since you're just ifdefing out code

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/ui/webui/ve...
File chrome/browser/ui/webui/version_handler.cc (right):

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_handler.cc:129: void
VersionHandler::OnGotPlugins(
On 2012/12/19 21:07:44, nileshagrawal1 wrote:
> Yes. I am relying on the linker not including this code in the final SO.

yes I think it would be good to remove that as well. i.e. someone can compile
with different link settings (debug vs release) that don't remove functions that
aren't called. i.e. have you sent this to debug bots?

> 
> Let me know if you feel strongly about this. There is no way to find and
> maintain all such instances, unless we have coverage via a component build.
> Infact (atleast in the chrome layer), we actively rely on link time removal to
> avoid adding too many ifdefs. 
> 
> OTOH, in the content layer, where we are trying to get a component build
> working, we need to remove unknown references at compile time.

[jam, 2012-12-19 21:32:31]

(ok i see the changes to extensions and metrics isn't trivial, readding them)

https://codereview.chromium.org/11615002/diff/22004/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11615002/diff/22004/chrome/chrome_browser.gyp...
chrome/chrome_browser.gypi:2376: 'browser/plugins/plugin_observer.cc',
nit: maybe in a followup change since this is already getting big. but seems
like all of chrome/browser/plugins/ should be removed?

[jam, 2012-12-19 21:35:02]

lgtm with the one nit in VersionHandler::OnGotPlugins

would be great if all the plugin code in chrome/browser/plugins is removed in a
followup (or if you wish, in this one)

[nilesh, 2012-12-19 21:47:25]

Thanks for the review.
I will handle removing all of chrome/browser/plugins in a subsequent CL.

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/ui/webui/ve...
File chrome/browser/ui/webui/version_handler.cc (right):

https://codereview.chromium.org/11615002/diff/5001/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_handler.cc:129: void
VersionHandler::OnGotPlugins(
ifdefed the method.

I have tried android (debug/release) and default ios bots. No other platform has
plugins disabled.

https://codereview.chromium.org/11615002/diff/22004/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11615002/diff/22004/chrome/chrome_browser.gyp...
chrome/chrome_browser.gypi:2376: 'browser/plugins/plugin_observer.cc',
On 2012/12/19 21:32:31, John Abd-El-Malek wrote:
> nit: maybe in a followup change since this is already getting big. but seems
> like all of chrome/browser/plugins/ should be removed? 

Yes. I can tackle this in a followup CL. I will keep the bug open
(crbug.com/162667)

[Ilya Sherman, 2012-12-19 22:45:57]

Hmm, this change is relatively invasive.  What's the advantage to #ifdef'ing out
all the callsites, rather than simply stubbing out the PluginService methods?

https://codereview.chromium.org/11615002/diff/24005/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/11615002/diff/24005/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:671: WritePluginList(plugin_list,
write_as_xml);
I'm not sure that it's safe to skip writing the plugin list entirely for the XML
uploads -- the UMA server might reject uploads that don't have the outer
"plugins" element.  I would prefer if you #ifdef'd out more narrowly within the
WritePluginList method.  Ditto for WritePluginStabilityElements.

[jam, 2012-12-19 23:03:33]

On Wed, Dec 19, 2012 at 2:45 PM, <isherman@chromium.org> wrote:

> Hmm, this change is relatively invasive.  What's the advantage to
> #ifdef'ing out
> all the callsites, rather than simply stubbing out the PluginService
> methods?
>

compiling in some the plugin code, and having to ifdef out random parts is
hard to maintain. it's much easier if no_plugins= no plugin code


>
>
> https://codereview.chromium.**org/11615002/diff/24005/**
>
chrome/browser/metrics/**metrics_log.cc<https://codereview.chromium.org/11615002/diff/24005/chrome/browser/metrics/metrics_log.cc>
> File chrome/browser/metrics/**metrics_log.cc (right):
>
> https://codereview.chromium.**org/11615002/diff/24005/**
>
chrome/browser/metrics/**metrics_log.cc#newcode671<https://codereview.chromium.org/11615002/diff/24005/chrome/browser/metrics/metrics_log.cc#newcode671>
> chrome/browser/metrics/**metrics_log.cc:671: WritePluginList(plugin_list,
> write_as_xml);
> I'm not sure that it's safe to skip writing the plugin list entirely for
> the XML uploads -- the UMA server might reject uploads that don't have
> the outer "plugins" element.  I would prefer if you #ifdef'd out more
> narrowly within the WritePluginList method.  Ditto for
> WritePluginStabilityElements.
>
>
https://codereview.chromium.**org/11615002/<https://codereview.chromium.org/1...
>

[nilesh, 2012-12-19 23:18:29]

Hi Ilya, Please take another look.

https://codereview.chromium.org/11615002/diff/24005/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/11615002/diff/24005/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:671: WritePluginList(plugin_list,
write_as_xml);
On 2012/12/19 22:45:57, Ilya Sherman wrote:
> I'm not sure that it's safe to skip writing the plugin list entirely for the
XML
> uploads -- the UMA server might reject uploads that don't have the outer
> "plugins" element.  I would prefer if you #ifdef'd out more narrowly within
the
> WritePluginList method.  Ditto for WritePluginStabilityElements.

Done.

However, there is a codepath which allows for the outer "plugins" element to be
skipped (in WritePluginStabilityElements if prefs::kStabilityPluginStats
is NULL), but I am not sure if it is is ever executed or is relevant.

[Ilya Sherman, 2012-12-19 23:45:44]

metrics/ LGTM, thanks

https://codereview.chromium.org/11615002/diff/24005/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/11615002/diff/24005/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:671: WritePluginList(plugin_list,
write_as_xml);
On 2012/12/19 23:18:29, nileshagrawal1 wrote:
> On 2012/12/19 22:45:57, Ilya Sherman wrote:
> > I'm not sure that it's safe to skip writing the plugin list entirely for the
> XML
> > uploads -- the UMA server might reject uploads that don't have the outer
> > "plugins" element.  I would prefer if you #ifdef'd out more narrowly within
> the
> > WritePluginList method.  Ditto for WritePluginStabilityElements.
> 
> Done.
> 
> However, there is a codepath which allows for the outer "plugins" element to
be
> skipped (in WritePluginStabilityElements if prefs::kStabilityPluginStats
> is NULL), but I am not sure if it is is ever executed or is relevant.

Yeah, I'm not sure how paranoid to be; but since I don't know the XML server
code intimately, I'd rather be safe than sorry.  I'm looking forward to being
able to remove the XML uploading code entirely soon :)

[Yoyo Zhou, 2012-12-19 23:52:36]

LGTM for extensions

[commit-bot: I haz the power, 2012-12-20 00:12:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/11615002/24009

[commit-bot: I haz the power, 2012-12-20 02:56:59]

Change committed as 174087