Clean up PluginDataRemover.

chrome/browser/plugin_data_remover.cc

Index: chrome/browser/plugin_data_remover.cc
diff --git a/chrome/browser/plugin_data_remover.cc b/chrome/browser/plugin_data_remover.cc
index 4415c5f3d6d8755ea962180410c6f424ff245b20..bf80e894deda77c9c75ded547b8a2ffc9f932cb4 100644
--- a/chrome/browser/plugin_data_remover.cc
+++ b/chrome/browser/plugin_data_remover.cc
@@ -5,46 +5,56 @@
 #include "chrome/browser/plugin_data_remover.h"
 
 #include "base/message_loop_proxy.h"
+#include "base/metrics/histogram.h"
+#include "base/version.h"
 #include "chrome/browser/browser_thread.h"
 #include "chrome/browser/plugin_service.h"
 #include "chrome/common/plugin_messages.h"
 #include "ipc/ipc_channel.h"

[jam, 2010/12/14 17:34:19]

nit: i dont think this include is needed

[Bernhard Bauer, 2010/12/14 20:38:37]

Done.

+#include "webkit/glue/plugins/plugin_group.h"
+#include "webkit/glue/plugins/plugin_list.h"
 
 #if defined(OS_POSIX)
 #include "ipc/ipc_channel_posix.h"
 #endif
 
 namespace {
-const std::string flash_mime_type = "application/x-shockwave-flash";
-const int64 timeout_ms = 10000;
-}
+const char* g_flash_mime_type = "application/x-shockwave-flash";
+// TODO(bauerb): Update minimum required Flash version as soon as there is one
+// implementing the API.
+const char* g_min_flash_version = "100";
+const int64 g_timeout_ms = 10000;
+}  // namespace
 
 PluginDataRemover::PluginDataRemover()
-    : channel_(NULL),
-      method_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
-}
+    : is_removing_(false),
+      channel_(NULL) { }
 
 PluginDataRemover::~PluginDataRemover() {
-  DCHECK(!done_task_.get());
-  if (!BrowserThread::DeleteSoon(BrowserThread::IO, FROM_HERE, channel_))
-    delete channel_;
+  DCHECK(!is_removing_);
+  if (channel_)
+    BrowserThread::DeleteSoon(BrowserThread::IO, FROM_HERE, channel_);
 }
 
 void PluginDataRemover::StartRemoving(base::Time begin_time, Task* done_task) {
   DCHECK(!done_task_.get());
+  DCHECK(!is_removing_);
+  remove_start_time_ = base::Time::Now();
   begin_time_ = begin_time;
 
   message_loop_ = base::MessageLoopProxy::CreateForCurrentThread();
   done_task_.reset(done_task);
+  is_removing_ = true;
 
+  AddRef();

[jam, 2010/12/14 17:34:19]

I don't understand why you keep a self reference between this function and
OnChannelOpened?  i.e. why not keep it until the process is done?  and also, if
there's a timeout, Release() is never called (i.e. what if the process never
launched or connected).

[Bernhard Bauer, 2010/12/14 20:38:37]

Even if the process never launches, eventually OnError is called (when the
PluginProcessHost is deleted). For that reason, I need to make sure the client
is not deleted until either OnChannelOpened or OnError is called, and calling
Release() in each of these methods is the easiest way to make sure that happens.

   PluginService::GetInstance()->OpenChannelToPlugin(
-      GURL(), flash_mime_type, this);
+      GURL(), g_flash_mime_type, this);
 
   BrowserThread::PostDelayedTask(
       BrowserThread::IO,
       FROM_HERE,
-      method_factory_.NewRunnableMethod(&PluginDataRemover::OnTimeout),
-      timeout_ms);
+      NewRunnableMethod(this, &PluginDataRemover::OnTimeout),
+      g_timeout_ms);
 }
 
 int PluginDataRemover::ID() {
@@ -60,7 +70,17 @@ void PluginDataRemover::SetPluginInfo(const WebPluginInfo& info) {
 }
 
 void PluginDataRemover::OnChannelOpened(const IPC::ChannelHandle& handle) {
+  ConnectToChannel(handle);
+  Release();
+}
+
+void PluginDataRemover::ConnectToChannel(const IPC::ChannelHandle& handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  // If we timed out, don't bother connecting.
+  if (!is_removing_)
+    return;
+
   DCHECK(!channel_);
 #if defined(OS_POSIX)
   // If we received a ChannelHandle, register it now.
@@ -69,14 +89,14 @@ void PluginDataRemover::OnChannelOpened(const IPC::ChannelHandle& handle) {
 #endif
   channel_ = new IPC::Channel(handle.name, IPC::Channel::MODE_CLIENT, this);
   if (!channel_->Connect()) {
-    NOTREACHED() << "Couldn't connect to plugin";
+    LOG(DFATAL) << "Couldn't connect to plugin";
     SignalDone();
     return;
   }
 
   if (!channel_->Send(
           new PluginMsg_ClearSiteData(0, std::string(), begin_time_))) {
-    NOTREACHED() << "Couldn't send ClearSiteData message";
+    LOG(DFATAL) << "Couldn't send ClearSiteData message";
     SignalDone();
   }
 }
@@ -84,10 +104,14 @@ void PluginDataRemover::OnChannelOpened(const IPC::ChannelHandle& handle) {
 void PluginDataRemover::OnError() {
   NOTREACHED() << "Couldn't open plugin channel";
   SignalDone();
+  Release();
 }
 
 void PluginDataRemover::OnClearSiteDataResult(bool success) {
-  DCHECK(success) << "ClearSiteData returned error";
+  if (!success)
+    LOG(DFATAL) << "ClearSiteData returned error";
+  UMA_HISTOGRAM_TIMES("ClearPluginData.time",
+                      base::Time::Now() - remove_start_time_);
   SignalDone();
 }
 
@@ -105,14 +129,38 @@ void PluginDataRemover::OnMessageReceived(const IPC::Message& msg) {
 }
 
 void PluginDataRemover::OnChannelError() {
-  NOTREACHED() << "Channel error";
+  LOG(DFATAL) << "Channel error";
   SignalDone();
 }
 
 void PluginDataRemover::SignalDone() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  if (!done_task_.get())
+  if (!is_removing_)
     return;
-  message_loop_->PostTask(FROM_HERE, done_task_.release());
-  message_loop_ = NULL;
+  is_removing_ = false;
+  if (done_task_.get()) {
+    message_loop_->PostTask(FROM_HERE, done_task_.release());
+    message_loop_ = NULL;
+  }
+}
+
+// static
+bool PluginDataRemover::IsSupported() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  bool allow_wildcard = false;
+  WebPluginInfo plugin;
+  std::string mime_type;
+  if (!NPAPI::PluginList::Singleton()->GetPluginInfo(GURL(),
+                                                     g_flash_mime_type,
+                                                     allow_wildcard,
+                                                     &plugin,
+                                                     &mime_type))
+    return false;
+  scoped_ptr<Version> version(
+      PluginGroup::CreateVersionFromString(plugin.version));
+  scoped_ptr<Version> min_version(
+      Version::GetVersionFromString(g_min_flash_version));
+  return plugin.enabled &&
+         version.get()
+         && min_version->CompareTo(*version) == -1;

[jam, 2010/12/14 17:34:19]

nit: style is to have the && at the end of hte previous line

[Bernhard Bauer, 2010/12/14 20:38:37]

Done.

 }