Readability review for bauerb.

PluginDataRemover opens a connection to the Flash plugin process, tells it to call the NPP_ClearSiteData function and waits for it to finish. This is used in Chrome to clear Flash LSO data from the Clear Browsing Data dialog and at shutdown.

BUG=none
TEST=none


Review URL: http://codereview.chromium.org/6665034

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@80351 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 53 insertions, 33 deletions

diff --git a/chrome/browser/plugin_data_remover.cc b/chrome/browser/plugin_data_remover.cc
index ebbacc4..1afb5ca 100644
--- a/chrome/browser/plugin_data_remover.cc
+++ b/chrome/browser/plugin_data_remover.cc
@@ -21,18 +21,21 @@
 #endif
 
 namespace {
+
 const char* kFlashMimeType = "application/x-shockwave-flash";
 // The minimum Flash Player version that implements NPP_ClearSiteData.
 const char* kMinFlashVersion = "10.3";
 const int64 kRemovalTimeoutMs = 10000;
 const uint64 kClearAllData = 0;
+
 }  // namespace
 
 PluginDataRemover::PluginDataRemover()
     : mime_type_(kFlashMimeType),
       is_removing_(false),
       event_(new base::WaitableEvent(true, false)),
-      channel_(NULL) { }
+      channel_(NULL) {
+}
 
 PluginDataRemover::~PluginDataRemover() {
   DCHECK(!is_removing_);
@@ -40,14 +43,15 @@ PluginDataRemover::~PluginDataRemover() {
     BrowserThread::DeleteSoon(BrowserThread::IO, FROM_HERE, channel_);
 }
 
-base::WaitableEvent* PluginDataRemover::StartRemoving(
-    const base::Time& begin_time) {
+base::WaitableEvent* PluginDataRemover::StartRemoving(base::Time begin_time) {
   DCHECK(!is_removing_);
   remove_start_time_ = base::Time::Now();
   begin_time_ = begin_time;
 
   is_removing_ = true;
 
+  // Balanced in OnChannelOpened or OnError. Exactly one them will eventually be
+  // called, so we need to keep this object around until then.
   AddRef();
   PluginService::GetInstance()->OpenChannelToNpapiPlugin(
       0, 0, GURL(), mime_type_, this);
@@ -74,7 +78,7 @@ void PluginDataRemover::Wait() {
 }
 
 int PluginDataRemover::ID() {
-  // Generate an ID for the browser process.
+  // Generate a unique identifier for this PluginProcessHostClient.
   return ChildProcessInfo::GenerateChildProcessUniqueId();
 }
 
@@ -88,6 +92,7 @@ void PluginDataRemover::SetPluginInfo(
 
 void PluginDataRemover::OnChannelOpened(const IPC::ChannelHandle& handle) {
   ConnectToChannel(handle);
+  // Balancing the AddRef call in StartRemoving.
   Release();
 }
 
@@ -106,10 +111,9 @@ void PluginDataRemover::ConnectToChannel(const IPC::ChannelHandle& handle) {
     return;
   }
 
-  if (!channel_->Send(
-          new PluginMsg_ClearSiteData(std::string(),
-                                      kClearAllData,
-                                      begin_time_))) {
+  if (!channel_->Send(new PluginMsg_ClearSiteData(std::string(),
+                                                  kClearAllData,
+                                                  begin_time_))) {
     NOTREACHED() << "Couldn't send ClearSiteData message";
     SignalDone();
     return;
@@ -119,6 +123,7 @@ void PluginDataRemover::ConnectToChannel(const IPC::ChannelHandle& handle) {
 void PluginDataRemover::OnError() {
   LOG(DFATAL) << "Couldn't open plugin channel";
   SignalDone();
+  // Balancing the AddRef call in StartRemoving.
   Release();
 }
 
diff --git a/chrome/browser/plugin_data_remover.h b/chrome/browser/plugin_data_remover.h
index b61218b..696df1c 100644
--- a/chrome/browser/plugin_data_remover.h
+++ b/chrome/browser/plugin_data_remover.h
@@ -23,17 +23,20 @@ class PluginDataRemover : public base::RefCountedThreadSafe<PluginDataRemover>,
  public:
   PluginDataRemover();
 
-  // Used in tests to call a different plugin.
+  // The plug-in whose data should be removed (usually Flash) is specified via
+  // its MIME type. This method sets a different MIME type in order to call a
+  // different plug-in (for example in tests).
   void set_mime_type(const std::string& mime_type) { mime_type_ = mime_type; }
 
   // Starts removing plug-in data stored since |begin_time|.
-  base::WaitableEvent* StartRemoving(const base::Time& begin_time);
+  base::WaitableEvent* StartRemoving(base::Time begin_time);
 
   // Returns whether there is a plug-in installed that supports removing
   // LSO data. Because this method possibly has to load the plug-in list, it
   // should only be called on the FILE thread.
   static bool IsSupported();
 
+  // Indicates whether we are still in the process of removing plug-in data.
   bool is_removing() const { return is_removing_; }
 
   // Wait until removing has finished. When the browser is still running (i.e.
@@ -41,14 +44,14 @@ class PluginDataRemover : public base::RefCountedThreadSafe<PluginDataRemover>,
   // with the WaitableEvent returned by StartRemoving.
   void Wait();
 
-  // PluginProcessHost::Client methods
+  // PluginProcessHost::Client methods.
   virtual int ID();
   virtual bool OffTheRecord();
   virtual void SetPluginInfo(const webkit::npapi::WebPluginInfo& info);
   virtual void OnChannelOpened(const IPC::ChannelHandle& handle);
   virtual void OnError();
 
-  // IPC::Channel::Listener methods
+  // IPC::Channel::Listener methods.
   virtual bool OnMessageReceived(const IPC::Message& message);
   virtual void OnChannelError();
 
@@ -57,9 +60,15 @@ class PluginDataRemover : public base::RefCountedThreadSafe<PluginDataRemover>,
   friend class PluginDataRemoverTest;
   ~PluginDataRemover();
 
+  // Signals that we are finished with removing data (successful or not). This
+  // method is safe to call multiple times.
   void SignalDone();
+  // Connects the client side of a newly opened plug-in channel.
   void ConnectToChannel(const IPC::ChannelHandle& handle);
+  // Handles the PluginHostMsg_ClearSiteDataResult message.
   void OnClearSiteDataResult(bool success);
+  // Called when a timeout happens in order not to block the client
+  // indefinitely.
   void OnTimeout();
 
   std::string mime_type_;
@@ -70,8 +79,11 @@ class PluginDataRemover : public base::RefCountedThreadSafe<PluginDataRemover>,
   base::Time begin_time_;
   scoped_ptr<base::WaitableEvent> event_;
   // We own the channel, but it's used on the IO thread, so it needs to be
-  // deleted there as well.
+  // deleted there. It's NULL until we have opened a connection to the plug-in
+  // process.
   IPC::Channel* channel_;
+
+  DISALLOW_COPY_AND_ASSIGN(PluginDataRemover);
 };
 
 #endif  // CHROME_BROWSER_PLUGIN_DATA_REMOVER_H_
diff --git a/chrome/browser/plugin_data_remover_browsertest.cc b/chrome/browser/plugin_data_remover_browsertest.cc
index 3fbaf75..23b67d6 100644
--- a/chrome/browser/plugin_data_remover_browsertest.cc
+++ b/chrome/browser/plugin_data_remover_browsertest.cc
@@ -49,8 +49,6 @@ class PluginDataRemoverTest : public InProcessBrowserTest,
     command_line->AppendSwitchPath(switches::kExtraPluginDir,
                                    browser_directory.AppendASCII("plugins"));
 #endif
-
-//    command_line->AppendSwitch(switches::kPluginStartupDialog);
   }
 
  private:
diff --git a/chrome/browser/plugin_exceptions_table_model.cc b/chrome/browser/plugin_exceptions_table_model.cc
index de679f0..cf37846 100644
--- a/chrome/browser/plugin_exceptions_table_model.cc
+++ b/chrome/browser/plugin_exceptions_table_model.cc
@@ -23,7 +23,8 @@ PluginExceptionsTableModel::PluginExceptionsTableModel(
                  NotificationService::AllSources());
 }
 
-PluginExceptionsTableModel::~PluginExceptionsTableModel() {}
+PluginExceptionsTableModel::~PluginExceptionsTableModel() {
+}
 
 bool PluginExceptionsTableModel::CanRemoveRows(const Rows& rows) const {
   return !rows.empty();
@@ -32,11 +33,12 @@ bool PluginExceptionsTableModel::CanRemoveRows(const Rows& rows) const {
 void PluginExceptionsTableModel::RemoveRows(const Rows& rows) {
   AutoReset<bool> tmp(&updates_disabled_, true);
   bool reload_all = false;
-  // Iterate in reverse over the rows to get the indexes right.
+  // Iterate over the rows starting with the highest ones so we can delete
+  // entries from |settings_| without the other indices shifting.
   for (Rows::const_reverse_iterator it = rows.rbegin();
        it != rows.rend(); ++it) {
     DCHECK_LT(*it, settings_.size());
-    SettingsEntry& entry = settings_[*it];
+    SettingsEntry entry = settings_[*it];
     HostContentSettingsMap* map = entry.is_otr ? otr_map_ : map_;
     map->SetContentSetting(entry.pattern,
                            CONTENT_SETTINGS_TYPE_PLUGINS,
@@ -50,7 +52,8 @@ void PluginExceptionsTableModel::RemoveRows(const Rows& rows) {
       if (row_counts_[entry.plugin_id] == 0)  {
         reload_all = true;
       } else {
-        observer_->OnItemsRemoved(*it, 1);
+        if (observer_)
+          observer_->OnItemsRemoved(*it, 1);
       }
     }
   }
@@ -156,7 +159,8 @@ void PluginExceptionsTableModel::LoadSettings() {
     }
     string16 title = plugins[i].GetGroupName();
     for (HostContentSettingsMap::SettingsForOneType::iterator setting_it =
-             settings.begin(); setting_it != settings.end(); ++setting_it) {
+             settings.begin();
+         setting_it != settings.end(); ++setting_it) {
       SettingsEntry entry = {
         setting_it->first,
         group_id,
@@ -193,4 +197,3 @@ void PluginExceptionsTableModel::ReloadSettings() {
   if (observer_)
     observer_->OnModelChanged();
 }
-
diff --git a/chrome/browser/plugin_exceptions_table_model.h b/chrome/browser/plugin_exceptions_table_model.h
index 8614258..55fc646 100644
--- a/chrome/browser/plugin_exceptions_table_model.h
+++ b/chrome/browser/plugin_exceptions_table_model.h
@@ -6,7 +6,6 @@
 #define CHROME_BROWSER_PLUGIN_EXCEPTIONS_TABLE_MODEL_H_
 #pragma once
 
-#include <deque>
 #include <string>
 #include <vector>
 
@@ -68,13 +67,16 @@ class PluginExceptionsTableModel : public RemoveRowsTableModel,
   scoped_refptr<HostContentSettingsMap> map_;
   scoped_refptr<HostContentSettingsMap> otr_map_;
 
-  std::deque<SettingsEntry> settings_;
-  std::deque<int> row_counts_;
-  std::deque<std::string> resources_;
+  std::vector<SettingsEntry> settings_;
+  std::vector<int> row_counts_;
+  std::vector<std::string> resources_;
   TableModel::Groups groups_;
 
   NotificationRegistrar registrar_;
   bool updates_disabled_;
+
+  // Weak, can be NULL. Our owner should manage its lifetime, see
+  // TableModel::SetObserver().
   ui::TableModelObserver* observer_;
 
   DISALLOW_COPY_AND_ASSIGN(PluginExceptionsTableModel);
diff --git a/chrome/browser/plugin_exceptions_table_model_unittest.cc b/chrome/browser/plugin_exceptions_table_model_unittest.cc
index ac14c86b..95342b1 100644
--- a/chrome/browser/plugin_exceptions_table_model_unittest.cc
+++ b/chrome/browser/plugin_exceptions_table_model_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -24,7 +24,7 @@ using ::testing::Invoke;
 
 class MockTableModelObserver : public ui::TableModelObserver {
  public:
-   explicit MockTableModelObserver(ui::TableModel* model)
+  explicit MockTableModelObserver(ui::TableModel* model)
       : model_(model) {
     ON_CALL(*this, OnItemsRemoved(_, _))
         .WillByDefault(
@@ -108,12 +108,12 @@ class PluginExceptionsTableModelTest : public testing::Test {
   }
 
  protected:
-  void CheckInvariants() {
-    typedef std::deque<PluginExceptionsTableModel::SettingsEntry> Entries;
-    Entries& settings = table_model_->settings_;
-    std::deque<int>& row_counts = table_model_->row_counts_;
-    std::deque<std::string>& resources = table_model_->resources_;
-    ui::TableModel::Groups& groups = table_model_->groups_;
+  void CheckInvariants() const {
+    typedef std::vector<PluginExceptionsTableModel::SettingsEntry> Entries;
+    const Entries& settings = table_model_->settings_;
+    const std::vector<int>& row_counts = table_model_->row_counts_;
+    const std::vector<std::string>& resources = table_model_->resources_;
+    const ui::TableModel::Groups& groups = table_model_->groups_;
 
     EXPECT_EQ(groups.size(), row_counts.size());
     EXPECT_EQ(groups.size(), resources.size());