Add a gyp flag to allow removing dependency on ppapi.

Add a gyp flag to allow removing dependency on ppapi.

 - Introduces a new macro ENABLE_PLUGINS
 - Create a new PluginDelegateHelper interface which is used by RenderViewImpl 


This flag will be used to remove all plugins related code from the Android build.

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

[Jay Civelli, 2012-11-29 18:57:43]

LGTM with nits.

https://codereview.chromium.org/11414180/diff/4002/content/browser/plugin_ser...
File content/browser/plugin_service_impl.cc (right):

https://codereview.chromium.org/11414180/diff/4002/content/browser/plugin_ser...
content/browser/plugin_service_impl.cc:271: #if defined(ENABLE_PPAPI)
Maybe you could simply do:
#if !defined(ENABLE_PPAPI)
return NULL;
#endif

https://codereview.chromium.org/11414180/diff/4002/content/renderer/media/med...
File content/renderer/media/media_stream_dispatcher.cc (right):

https://codereview.chromium.org/11414180/diff/4002/content/renderer/media/med...
content/renderer/media/media_stream_dispatcher.cc:8: #include
"base/message_loop_proxy.h"
Why do you need this?

https://codereview.chromium.org/11414180/diff/4002/content/renderer/plugin_de...
File content/renderer/plugin_delegate_helper.h (right):

https://codereview.chromium.org/11414180/diff/4002/content/renderer/plugin_de...
content/renderer/plugin_delegate_helper.h:52: const webkit::WebPluginInfo&
webplugin_info,
Indent should be 4 spaces.

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

https://codereview.chromium.org/11414180/diff/4002/content/renderer/render_vi...
content/renderer/render_view_impl.cc:4383: if (pepper_webplugin) return
pepper_webplugin;
Return statement should be on its own line in C++.

[nilesh, 2012-11-29 22:54:03]

https://codereview.chromium.org/11414180/diff/4002/content/browser/plugin_ser...
File content/browser/plugin_service_impl.cc (right):

https://codereview.chromium.org/11414180/diff/4002/content/browser/plugin_ser...
content/browser/plugin_service_impl.cc:271: #if defined(ENABLE_PPAPI)
On 2012/11/29 18:57:43, Jay Civelli wrote:
> Maybe you could simply do:
> #if !defined(ENABLE_PPAPI)
> return NULL;
> #endif

I need to compile out references to PpapiPluginProcessHost as it is excluded
from the build.

https://codereview.chromium.org/11414180/diff/4002/content/renderer/media/med...
File content/renderer/media/media_stream_dispatcher.cc (right):

https://codereview.chromium.org/11414180/diff/4002/content/renderer/media/med...
content/renderer/media/media_stream_dispatcher.cc:8: #include
"base/message_loop_proxy.h"
It is used below. I got a build error due to this.
I think there should be an explicit include.

https://codereview.chromium.org/11414180/diff/4002/content/renderer/plugin_de...
File content/renderer/plugin_delegate_helper.h (right):

https://codereview.chromium.org/11414180/diff/4002/content/renderer/plugin_de...
content/renderer/plugin_delegate_helper.h:52: const webkit::WebPluginInfo&
webplugin_info,
On 2012/11/29 18:57:43, Jay Civelli wrote:
> Indent should be 4 spaces.

Done.

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

https://codereview.chromium.org/11414180/diff/4002/content/renderer/render_vi...
content/renderer/render_view_impl.cc:4383: if (pepper_webplugin) return
pepper_webplugin;
On 2012/11/29 18:57:43, Jay Civelli wrote:
> Return statement should be on its own line in C++.

Done.

[nilesh, 2012-11-29 23:25:55]

ping.
Brett, Can you please take a look.
Thanks.

[brettw, 2012-11-30 23:18:12]

https://codereview.chromium.org/11414180/diff/4002/content/browser/plugin_dat...
File content/browser/plugin_data_remover_impl.cc (right):

https://codereview.chromium.org/11414180/diff/4002/content/browser/plugin_dat...
content/browser/plugin_data_remover_impl.cc:23: #if defined(ENABLE_PPAPI)
#ifdefed includes should go below all the others, with a blank line between
them.

https://codereview.chromium.org/11414180/diff/2005/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11414180/diff/2005/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:2364: '../ppapi/ppapi_internal.gyp:ppapi_ipc',  # For
PpapiMsg_LoadPlugin
Can you just delete the comment here? It's out of date (we need lots more than
that now).

https://codereview.chromium.org/11414180/diff/2005/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/11414180/diff/2005/chrome/chrome_browser_ui.g...
chrome/chrome_browser_ui.gypi:2217: '../ppapi/ppapi_internal.gyp:ppapi_ipc',  #
For PpapiMsg_LoadPlugin
Ditto

https://codereview.chromium.org/11414180/diff/2005/content/browser/plugin_dat...
File content/browser/plugin_data_remover_impl.cc (right):

https://codereview.chromium.org/11414180/diff/2005/content/browser/plugin_dat...
content/browser/plugin_data_remover_impl.cc:23: #if defined(ENABLE_PPAPI)
Put ifdefed includes below the others.

https://codereview.chromium.org/11414180/diff/2005/content/browser/plugin_dat...
content/browser/plugin_data_remover_impl.cc:207: IPC::Message*
CreatePpapiClearSiteDataMsg(uint64 max_age) {
I'd find this much easier to follow if there were two versions of this function,
one for ENABLE_PPAPI and one for not. That way inside the function there would
only be one level of ifdef (for OS_WIN), which should be easier to follow, even
if it's ~2 more lines.

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

https://codereview.chromium.org/11414180/diff/2005/content/browser/renderer_h...
content/browser/renderer_host/render_message_filter.cc:388: #if
defined(ENABLE_PPAPI)
Is this necessary? You still keep the .cc functions and the messages are
compiled in no matter what. I'd be inclined to not ifdef this if it's not
necessary.

https://codereview.chromium.org/11414180/diff/2005/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.h (right):

https://codereview.chromium.org/11414180/diff/2005/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.h:112: 
When implementing a base class, don't use blank lines so the functions are all
grouped in one block (so you can tell that they're all the same base class).

https://codereview.chromium.org/11414180/diff/2005/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.h:439: CONTENT_EXPORT
scoped_refptr<webkit::ppapi::PluginModule>
Is this CONTENT_EXPORT needed? Now that this is a private I would not expect to
need to export it from the library.

https://codereview.chromium.org/11414180/diff/2005/content/renderer/plugin_de...
File content/renderer/plugin_delegate_helper.cc (right):

https://codereview.chromium.org/11414180/diff/2005/content/renderer/plugin_de...
content/renderer/plugin_delegate_helper.cc:8: #include
"webkit/plugins/ppapi/plugin_module.h"
Why is this include necessary?

https://codereview.chromium.org/11414180/diff/2005/content/renderer/plugin_de...
content/renderer/plugin_delegate_helper.cc:50: }  // namespace content
Need blank before this.

https://codereview.chromium.org/11414180/diff/2005/content/renderer/plugin_de...
File content/renderer/plugin_delegate_helper.h (right):

https://codereview.chromium.org/11414180/diff/2005/content/renderer/plugin_de...
content/renderer/plugin_delegate_helper.h:46: class PluginDelegateHelper {
The name for this somehow seems wrong. It's not a helper for a plugin delegate.

Maybe something like RenderViewPepperHelper since from the render view's
perspective it's not a delegate, but the "thing that manages Pepper plugins for
the RenderView". Feel free to suggest a better name.

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

https://codereview.chromium.org/11414180/diff/2005/content/renderer/render_vi...
content/renderer/render_view_impl.cc:608: #if defined(ENABLE_PPAPI)
I think this should just go in the constructor rather than the initializer list.
#ifdefs in the initializer list get hard to follow.

[nilesh, 2012-12-01 00:41:34]

Addressed comments.
PTAL.

https://codereview.chromium.org/11414180/diff/2005/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/11414180/diff/2005/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:2364: '../ppapi/ppapi_internal.gyp:ppapi_ipc',  # For
PpapiMsg_LoadPlugin
On 2012/11/30 23:18:12, brettw wrote:
> Can you just delete the comment here? It's out of date (we need lots more than
> that now).

Done.

https://codereview.chromium.org/11414180/diff/2005/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/11414180/diff/2005/chrome/chrome_browser_ui.g...
chrome/chrome_browser_ui.gypi:2217: '../ppapi/ppapi_internal.gyp:ppapi_ipc',  #
For PpapiMsg_LoadPlugin
On 2012/11/30 23:18:12, brettw wrote:
> Ditto

Done.

https://codereview.chromium.org/11414180/diff/2005/content/browser/plugin_dat...
File content/browser/plugin_data_remover_impl.cc (right):

https://codereview.chromium.org/11414180/diff/2005/content/browser/plugin_dat...
content/browser/plugin_data_remover_impl.cc:23: #if defined(ENABLE_PPAPI)
On 2012/11/30 23:18:12, brettw wrote:
> Put ifdefed includes below the others.

Done.

https://codereview.chromium.org/11414180/diff/2005/content/browser/plugin_dat...
content/browser/plugin_data_remover_impl.cc:207: IPC::Message*
CreatePpapiClearSiteDataMsg(uint64 max_age) {
On 2012/11/30 23:18:12, brettw wrote:
> I'd find this much easier to follow if there were two versions of this
function,
> one for ENABLE_PPAPI and one for not. That way inside the function there would
> only be one level of ifdef (for OS_WIN), which should be easier to follow,
even
> if it's ~2 more lines.

Done.

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

https://codereview.chromium.org/11414180/diff/2005/content/browser/renderer_h...
content/browser/renderer_host/render_message_filter.cc:388: #if
defined(ENABLE_PPAPI)
On 2012/11/30 23:18:12, brettw wrote:
> Is this necessary? You still keep the .cc functions and the messages are
> compiled in no matter what. I'd be inclined to not ifdef this if it's not
> necessary.

These methods are compiled in, but are removed by the linker if these references
are ifdefed out.
These methods contain reference to ppapi which are not compiled in. If we remove
ifdef here, then we will need to add ifdefs in these methods.

https://codereview.chromium.org/11414180/diff/2005/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.h (right):

https://codereview.chromium.org/11414180/diff/2005/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.h:112: 
On 2012/11/30 23:18:12, brettw wrote:
> When implementing a base class, don't use blank lines so the functions are all
> grouped in one block (so you can tell that they're all the same base class).

Done.

https://codereview.chromium.org/11414180/diff/2005/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.h:439: CONTENT_EXPORT
scoped_refptr<webkit::ppapi::PluginModule>
On 2012/11/30 23:18:12, brettw wrote:
> Is this CONTENT_EXPORT needed? Now that this is a private I would not expect
to
> need to export it from the library.

Done.

https://codereview.chromium.org/11414180/diff/2005/content/renderer/plugin_de...
File content/renderer/plugin_delegate_helper.cc (right):

https://codereview.chromium.org/11414180/diff/2005/content/renderer/plugin_de...
content/renderer/plugin_delegate_helper.cc:8: #include
"webkit/plugins/ppapi/plugin_module.h"
On 2012/11/30 23:18:12, brettw wrote:
> Why is this include necessary?

Removed.

https://codereview.chromium.org/11414180/diff/2005/content/renderer/plugin_de...
content/renderer/plugin_delegate_helper.cc:50: }  // namespace content
On 2012/11/30 23:18:12, brettw wrote:
> Need blank before this.

Done.

https://codereview.chromium.org/11414180/diff/2005/content/renderer/plugin_de...
File content/renderer/plugin_delegate_helper.h (right):

https://codereview.chromium.org/11414180/diff/2005/content/renderer/plugin_de...
content/renderer/plugin_delegate_helper.h:46: class PluginDelegateHelper {
On 2012/11/30 23:18:12, brettw wrote:
> The name for this somehow seems wrong. It's not a helper for a plugin
delegate.
> 
> Maybe something like RenderViewPepperHelper since from the render view's
> perspective it's not a delegate, but the "thing that manages Pepper plugins
for
> the RenderView". Feel free to suggest a better name.

I agree RenderViewPepperHelper is a better name. I will rename this after all
comments are resolved.

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

https://codereview.chromium.org/11414180/diff/2005/content/renderer/render_vi...
content/renderer/render_view_impl.cc:608: #if defined(ENABLE_PPAPI)
On 2012/11/30 23:18:12, brettw wrote:
> I think this should just go in the constructor rather than the initializer
list.
> #ifdefs in the initializer list get hard to follow.

Done.

[nilesh, 2012-12-01 00:45:58]

Adding palmer for content/common/all_messages.h

[nilesh, 2012-12-01 01:20:49]

Renamed PluginDelegateHelper to RenderViewPepperHelper.

[palmer, 2012-12-03 21:31:16]

Although you only wanted my IPC security review, I've decided to make your life
a bit harder. :) I'm just really helpful like that.

From a security perspective, we are fine with disabling PPAPI on these platforms
where we don't have Flash or Chrome's PDF reader; but now we are in a bit of a
weird place where PPAPI (the good plugin API) is gone, but NPAPI (the bad plugin
API) is still present. I see in some places (e.g. chrome/chrome_browser.gypi) we
build NPAPI only when os != iOS, but it makes more sense to me for this to be an
"enable_plugins" flag rather than enable_ppapi. That is, it would enable/disable
both PPAPI and NPAPI in one go. Might as well get rid of any other
plugin-related code while you're at it.

We don't want to allow a situation in which NPAPI can creep back ("But it's the
only plugin API enabled on Android. We have to use it!", for example). Plus,
disabling all plugin functionality would make builds even faster and the browser
even smaller and faster...

[brettw, 2012-12-03 21:44:12]

So maybe we should be using ENABLE_PLUGINS instead? That makes more sense to me,
and while it adds more files, it makes some places cleaner (like
plugin_data_remover_impl would just be removed from the build instead of filled
with ifdefs).

[nilesh, 2012-12-03 21:58:47]

I am happy to explore removing npapi too. I guess most of the code we want to
remove is in webkit/plugins/npapi/ ?
The third_party/npapi/npapi.gyp is not a real target (just .h files)

I would really like to do it in steps.
Even with this CL we are not removing code from webkit/plugins/ppapi/.. That was
going to be my next step.

I am happy to rename this to ENABLE_PLUGINS, but it will need multiple CLs to
remove all of plugins.

[brettw, 2012-12-03 22:02:47]

I think that plan sounds reasonable (renaming the flag ENABLE_PLUGINS now, do
the rest in an extra step). Maybe we can neuter one extra place to make sure
NPAPI plugins don't run so in case you don't come back to this then NPAPI
plugins won't work and we avoid Justin's scenario?

NPAPI stuff is in webkit/plugins/npapi and content/plugin.

[nilesh, 2012-12-03 23:39:14]

On 2012/12/03 22:02:47, brettw wrote:
> I think that plan sounds reasonable (renaming the flag ENABLE_PLUGINS now, do
> the rest in an extra step). Maybe we can neuter one extra place to make sure
> NPAPI plugins don't run so in case you don't come back to this then NPAPI
> plugins won't work and we avoid Justin's scenario?
> 
> NPAPI stuff is in webkit/plugins/npapi and content/plugin.

I have renamed enable_ppapi to enable_plugins
Additionally, I have ifdefed out PluginMain and WorkerMain in
content/app/content_main_runner.cc

Please take another look.

[palmer, 2012-12-03 23:50:51]

LGTM from an IPC security review perspective; whether or not the NPAPI change
incurs an IPC review, I'd like to see the next patch too (since I also wear the
Chrome-on-mobile security guy hat). Thanks!

https://codereview.chromium.org/11414180/diff/12012/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/11414180/diff/12012/build/common.gypi#newcode379
build/common.gypi:379: # Enable ppapi by default.
Maybe improve the comment:

# Enable PPAPI and NPAPI by default.
# TODO(nileshagrawal): Make this flag
# enable/disable NPAPI as well as PPAPI; see crbug.com/162667.

I.e. include a specific reference to the bug so that we know what the full
meaning of this flag is intended to be, someday.

[nilesh, 2012-12-04 00:21:48]

Thanks Chris. 
Will add you on CLs related to this.

https://codereview.chromium.org/11414180/diff/12012/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/11414180/diff/12012/build/common.gypi#newcode379
build/common.gypi:379: # Enable ppapi by default.
On 2012/12/03 23:50:51, Chris P. wrote:
> Maybe improve the comment:
> 
> # Enable PPAPI and NPAPI by default.
> # TODO(nileshagrawal): Make this flag
> # enable/disable NPAPI as well as PPAPI; see crbug.com/162667.
> 
> I.e. include a specific reference to the bug so that we know what the full
> meaning of this flag is intended to be, someday.

Done.

[nilesh, 2012-12-04 17:47:40]

Hi Brett, Does this look ok to you?

[brettw, 2012-12-04 23:13:52]

lgtm

[commit-bot: I haz the power, 2012-12-04 23:37:03]

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

[commit-bot: I haz the power, 2012-12-04 23:37:12]

Failed to apply patch for content/browser/plugin_data_remover_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/plugin_data_remover_impl.cc
  Hunk #4 FAILED at 252.
  1 out of 4 hunks FAILED -- saving rejects to file
content/browser/plugin_data_remover_impl.cc.rej

Patch:       content/browser/plugin_data_remover_impl.cc
Index: content/browser/plugin_data_remover_impl.cc
diff --git a/content/browser/plugin_data_remover_impl.cc
b/content/browser/plugin_data_remover_impl.cc
index
08b9aff12db2b2618f9146f8e7eb036176923743..679b9acf8515e8ce41c493b873cec9e42d379085
100644
--- a/content/browser/plugin_data_remover_impl.cc
+++ b/content/browser/plugin_data_remover_impl.cc
@@ -20,10 +20,13 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/pepper_plugin_info.h"
-#include "ppapi/proxy/ppapi_messages.h"
 #include "webkit/plugins/npapi/plugin_utils.h"
 #include "webkit/plugins/plugin_constants.h"
 
+#if defined(ENABLE_PLUGINS)
+#include "ppapi/proxy/ppapi_messages.h"
+#endif
+
 namespace content {
 
 namespace {
@@ -178,8 +181,10 @@ class PluginDataRemoverImpl::Context
     IPC_BEGIN_MESSAGE_MAP(Context, message)
       IPC_MESSAGE_HANDLER(PluginHostMsg_ClearSiteDataResult,
                           OnClearSiteDataResult)
+#if defined(ENABLE_PLUGINS)
       IPC_MESSAGE_HANDLER(PpapiHostMsg_ClearSiteDataResult,
                           OnPpapiClearSiteDataResult)
+#endif
       IPC_MESSAGE_UNHANDLED_ERROR()
     IPC_END_MESSAGE_MAP()
 
@@ -200,6 +205,31 @@ class PluginDataRemoverImpl::Context
   friend class base::DeleteHelper<Context>;
   virtual ~Context() {}
 
+#if defined(ENABLE_PLUGINS)
+  IPC::Message* CreatePpapiClearSiteDataMsg(uint64 max_age) {
+    FilePath profile_path =
+        PepperFileMessageFilter::GetDataDirName(browser_context_path_);
+    // TODO(vtl): This "duplicates" logic in webkit/plugins/ppapi/file_path.cc
+    // (which prepends the plugin name to the relative part of the path
+    // instead, with the absolute, profile-dependent part being enforced by
+    // the browser).
+#if defined(OS_WIN)
+    FilePath plugin_data_path =
+        profile_path.Append(FilePath(UTF8ToUTF16(plugin_name_)));
+#else
+    FilePath plugin_data_path = profile_path.Append(FilePath(plugin_name_));
+#endif  // defined(OS_WIN)
+    return new PpapiMsg_ClearSiteData(0u, plugin_data_path, std::string(),
+                                      kClearAllData, max_age);
+  }
+#else
+  IPC::Message* CreatePpapiClearSiteDataMsg(uint64 max_age) {
+    NOTREACHED() << "CreatePpapiClearSiteDataMsg called with "
+        << "ENABLE_PLUGINS undefined.";
+    return NULL;
+  }
+#endif  // defined(ENABLE_PLUGINS)
+
   // Connects the client side of a newly opened plug-in channel.
   void ConnectToChannel(const IPC::ChannelHandle& handle, bool is_ppapi) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
@@ -222,20 +252,7 @@ class PluginDataRemoverImpl::Context
 
     IPC::Message* msg;
     if (is_ppapi) {
-      FilePath profile_path =
-          PepperFileMessageFilter::GetDataDirName(browser_context_path_);
-      // TODO(vtl): This "duplicates" logic in
webkit/plugins/ppapi/file_path.cc
-      // (which prepends the plugin name to the relative part of the path
-      // instead, with the absolute, profile-dependent part being enforced by
-      // the browser).
-#if defined(OS_WIN)
-      FilePath plugin_data_path =
-          profile_path.Append(FilePath(UTF8ToUTF16(plugin_name_)));
-#else
-      FilePath plugin_data_path = profile_path.Append(FilePath(plugin_name_));
-#endif
-      msg = new PpapiMsg_ClearSiteData(0u, plugin_data_path, std::string(),
-                                       kClearAllData, max_age);
+      msg = CreatePpapiClearSiteDataMsg(max_age);
     } else {
       msg = new PluginMsg_ClearSiteData(std::string(), kClearAllData, max_age);
     }

[commit-bot: I haz the power, 2012-12-04 23:42:16]

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

[commit-bot: I haz the power, 2012-12-04 23:42:23]

Failed to apply patch for content/browser/plugin_data_remover_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/plugin_data_remover_impl.cc
  Hunk #4 FAILED at 252.
  1 out of 4 hunks FAILED -- saving rejects to file
content/browser/plugin_data_remover_impl.cc.rej

Patch:       content/browser/plugin_data_remover_impl.cc
Index: content/browser/plugin_data_remover_impl.cc
diff --git a/content/browser/plugin_data_remover_impl.cc
b/content/browser/plugin_data_remover_impl.cc
index
08b9aff12db2b2618f9146f8e7eb036176923743..679b9acf8515e8ce41c493b873cec9e42d379085
100644
--- a/content/browser/plugin_data_remover_impl.cc
+++ b/content/browser/plugin_data_remover_impl.cc
@@ -20,10 +20,13 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/pepper_plugin_info.h"
-#include "ppapi/proxy/ppapi_messages.h"
 #include "webkit/plugins/npapi/plugin_utils.h"
 #include "webkit/plugins/plugin_constants.h"
 
+#if defined(ENABLE_PLUGINS)
+#include "ppapi/proxy/ppapi_messages.h"
+#endif
+
 namespace content {
 
 namespace {
@@ -178,8 +181,10 @@ class PluginDataRemoverImpl::Context
     IPC_BEGIN_MESSAGE_MAP(Context, message)
       IPC_MESSAGE_HANDLER(PluginHostMsg_ClearSiteDataResult,
                           OnClearSiteDataResult)
+#if defined(ENABLE_PLUGINS)
       IPC_MESSAGE_HANDLER(PpapiHostMsg_ClearSiteDataResult,
                           OnPpapiClearSiteDataResult)
+#endif
       IPC_MESSAGE_UNHANDLED_ERROR()
     IPC_END_MESSAGE_MAP()
 
@@ -200,6 +205,31 @@ class PluginDataRemoverImpl::Context
   friend class base::DeleteHelper<Context>;
   virtual ~Context() {}
 
+#if defined(ENABLE_PLUGINS)
+  IPC::Message* CreatePpapiClearSiteDataMsg(uint64 max_age) {
+    FilePath profile_path =
+        PepperFileMessageFilter::GetDataDirName(browser_context_path_);
+    // TODO(vtl): This "duplicates" logic in webkit/plugins/ppapi/file_path.cc
+    // (which prepends the plugin name to the relative part of the path
+    // instead, with the absolute, profile-dependent part being enforced by
+    // the browser).
+#if defined(OS_WIN)
+    FilePath plugin_data_path =
+        profile_path.Append(FilePath(UTF8ToUTF16(plugin_name_)));
+#else
+    FilePath plugin_data_path = profile_path.Append(FilePath(plugin_name_));
+#endif  // defined(OS_WIN)
+    return new PpapiMsg_ClearSiteData(0u, plugin_data_path, std::string(),
+                                      kClearAllData, max_age);
+  }
+#else
+  IPC::Message* CreatePpapiClearSiteDataMsg(uint64 max_age) {
+    NOTREACHED() << "CreatePpapiClearSiteDataMsg called with "
+        << "ENABLE_PLUGINS undefined.";
+    return NULL;
+  }
+#endif  // defined(ENABLE_PLUGINS)
+
   // Connects the client side of a newly opened plug-in channel.
   void ConnectToChannel(const IPC::ChannelHandle& handle, bool is_ppapi) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
@@ -222,20 +252,7 @@ class PluginDataRemoverImpl::Context
 
     IPC::Message* msg;
     if (is_ppapi) {
-      FilePath profile_path =
-          PepperFileMessageFilter::GetDataDirName(browser_context_path_);
-      // TODO(vtl): This "duplicates" logic in
webkit/plugins/ppapi/file_path.cc
-      // (which prepends the plugin name to the relative part of the path
-      // instead, with the absolute, profile-dependent part being enforced by
-      // the browser).
-#if defined(OS_WIN)
-      FilePath plugin_data_path =
-          profile_path.Append(FilePath(UTF8ToUTF16(plugin_name_)));
-#else
-      FilePath plugin_data_path = profile_path.Append(FilePath(plugin_name_));
-#endif
-      msg = new PpapiMsg_ClearSiteData(0u, plugin_data_path, std::string(),
-                                       kClearAllData, max_age);
+      msg = CreatePpapiClearSiteDataMsg(max_age);
     } else {
       msg = new PluginMsg_ClearSiteData(std::string(), kClearAllData, max_age);
     }

[commit-bot: I haz the power, 2012-12-05 00:45:19]

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

[commit-bot: I haz the power, 2012-12-05 05:24:57]

Change committed as 171162

[qinmin, 2013-01-02 23:45:05]

On 2012/12/05 05:24:57, I haz the power (commit-bot) wrote:
> Change committed as 171162

This change breaks the old style youtube embeds on android. We still need the
plugin support, can we split ppapi plugin from regular plugins?
https://code.google.com/p/chromium/issues/detail?id=167971

[nilesh, 2013-01-03 00:19:55]

On 2013/01/02 23:45:05, qinmin wrote:
> On 2012/12/05 05:24:57, I haz the power (commit-bot) wrote:
> > Change committed as 171162
> 
> This change breaks the old style youtube embeds on android. We still need the
> plugin support, can we split ppapi plugin from regular plugins?
> https://code.google.com/p/chromium/issues/detail?id=167971


As discussed offline, this CL is not the issue as it mainly deals with pepper
plugins. The offending CL is a more recent change:
https://chromiumcodereview.appspot.com/11615002