Switch MHTMLGenerationManager to use a Callback.

Relanding:
MHTMLGenerationManager now uses a callback instead of a notification.

BUG=None
TEST=MHTML generation (via extension API should still work).
TBR=jam

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

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

chrome/browser/extensions/extension_save_page_api.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/extensions/extension_save_page_api.h"
 
+#include "base/bind.h"
 #include "base/file_util.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
@@ -105,46 +106,27 @@ void SavePageAsMHTMLFunction::TemporaryFileCreated(bool success) {
     return;
   }
 
-  registrar_.Add(
-      this, content::NOTIFICATION_MHTML_GENERATED,
-      content::Source<RenderViewHost>(tab_contents->render_view_host()));
-  // TODO(jcivelli): we should listen for navigation in the tab, tab closed,
-  //                 renderer died.
+  MHTMLGenerationManager::GenerateMHTMLCallback callback =
+      base::Bind(&SavePageAsMHTMLFunction::MHTMLGenerated, this);
+
   g_browser_process->mhtml_generation_manager()->GenerateMHTML(
-      tab_contents, mhtml_path_);
+      tab_contents, mhtml_path_, callback);
 }
 
-void SavePageAsMHTMLFunction::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  DCHECK(type == content::NOTIFICATION_MHTML_GENERATED);
-
-  const MHTMLGenerationManager::NotificationDetails* save_details =
-      content::Details<MHTMLGenerationManager::NotificationDetails>(details).
-          ptr();
-
-  if (mhtml_path_ != save_details->file_path) {
-    // This could happen if there are concurrent MHTML generations going on for
-    // the same tab.
-    LOG(WARNING) << "Received a notification that MHTML was generated but for a"
-        " different file.";
-    return;
-  }
-
-  registrar_.RemoveAll();
-
-  if (save_details->file_size <= 0) {
+void SavePageAsMHTMLFunction::MHTMLGenerated(const FilePath& file_path,
+                                             int64 mhtml_file_size) {
+  DCHECK(mhtml_path_ == file_path);
+  if (mhtml_file_size <= 0) {
     ReturnFailure(kMHTMLGenerationFailedError);
     return;
   }
 
-  if (save_details->file_size > std::numeric_limits<int>::max()) {
+  if (mhtml_file_size > std::numeric_limits<int>::max()) {
     ReturnFailure(kFileTooBigError);
     return;
   }
 
-  ReturnSuccess(save_details->file_size);
+  ReturnSuccess(mhtml_file_size);
 }
 
 void SavePageAsMHTMLFunction::ReturnFailure(const std::string& error) {

chrome/browser/extensions/extension_save_page_api.h

@@ -10,12 +10,11 @@
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/extensions/extension_function.h"
 #include "content/browser/tab_contents/tab_contents_observer.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 #include "webkit/blob/deletable_file_reference.h"
 
-class SavePageAsMHTMLFunction : public AsyncExtensionFunction,
-                                public content::NotificationObserver {
+class FilePath;
+
+class SavePageAsMHTMLFunction : public AsyncExtensionFunction {
  public:
   SavePageAsMHTMLFunction();
 
@@ -31,9 +30,6 @@ class SavePageAsMHTMLFunction : public AsyncExtensionFunction,
  private:
   virtual ~SavePageAsMHTMLFunction();
   virtual bool RunImpl() OVERRIDE;
-  virtual void Observe(int type,
-                       const content::NotificationSource& source,
-                       const content::NotificationDetails& details) OVERRIDE;
   virtual bool OnMessageReceivedFromRenderView(
       const IPC::Message& message) OVERRIDE;
 
@@ -45,6 +41,9 @@ class SavePageAsMHTMLFunction : public AsyncExtensionFunction,
   void ReturnFailure(const std::string& error);
   void ReturnSuccess(int64 file_size);
 
+  // Callback called once the MHTML generation is done.
+  void MHTMLGenerated(const FilePath& file_path, int64 mhtml_file_size);
+
   // Returns the TabContents we are associated with, NULL if it's been closed.
   TabContents* GetTabContents();
 
@@ -56,8 +55,6 @@ class SavePageAsMHTMLFunction : public AsyncExtensionFunction,
   // The file containing the MHTML.
   scoped_refptr<webkit_blob::DeletableFileReference> mhtml_file_;
 
-  content::NotificationRegistrar registrar_;
-
   DECLARE_EXTENSION_FUNCTION_NAME("experimental.savePage.saveAsMHTML")
 };
 

content/browser/download/mhtml_generation_browsertest.cc

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/bind.h"
 #include "base/file_path.h"
 #include "base/scoped_temp_dir.h"
 #include "chrome/browser/ui/browser.h"
@@ -10,7 +11,6 @@
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/browser/download/mhtml_generation_manager.h"
 #include "content/browser/tab_contents/tab_contents.h"
-#include "content/public/browser/notification_types.h"
 #include "net/test/test_server.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -18,7 +18,13 @@ namespace {
 
 class MHTMLGenerationTest : public InProcessBrowserTest {
  public:
-  MHTMLGenerationTest() {}
+  MHTMLGenerationTest() : mhtml_generated_(false), file_size_(0) {}
+
+  void MHTMLGenerated(const FilePath& path, int64 size) {
+    mhtml_generated_ = true;
+    file_size_ = size;
+    MessageLoopForUI::current()->Quit();
+  }
 
  protected:
   virtual void SetUp() {
@@ -26,7 +32,14 @@ class MHTMLGenerationTest : public InProcessBrowserTest {
     InProcessBrowserTest::SetUp();
   }
 
+  bool mhtml_generated() const { return mhtml_generated_; }
+  int64 file_size() const { return file_size_; }
+
   ScopedTempDir temp_dir_;
+
+ private:
+  bool mhtml_generated_;
+  int64 file_size_;
 };
 
 // Tests that generating a MHTML does create contents.
@@ -46,16 +59,14 @@ IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateMHTML) {
   MHTMLGenerationManager* mhtml_generation_manager =
       g_browser_process->mhtml_generation_manager();
 
-  content::Source<RenderViewHost> source(tab->render_view_host());
-  ui_test_utils::WindowedNotificationObserverWithDetails<
-      MHTMLGenerationManager::NotificationDetails> signal(
-          content::NOTIFICATION_MHTML_GENERATED, source);
-  mhtml_generation_manager->GenerateMHTML(tab, path);
-  signal.Wait();
+  mhtml_generation_manager->GenerateMHTML(tab, path,
+      base::Bind(&MHTMLGenerationTest::MHTMLGenerated, this));
+
+  // Block until the MHTML is generated.
+  ui_test_utils::RunMessageLoop();
 
-  MHTMLGenerationManager::NotificationDetails details;
-  ASSERT_TRUE(signal.GetDetailsFor(source.map_key(), &details));
-  ASSERT_GT(details.file_size, 0);
+  EXPECT_TRUE(mhtml_generated());
+  EXPECT_GT(file_size(), 0);
 
   // Make sure the actual generated file has some contents.
   int64 file_size;

content/browser/download/mhtml_generation_manager.cc

@@ -22,6 +22,9 @@ MHTMLGenerationManager::Job::Job()
       routing_id(-1) {
 }
 
+MHTMLGenerationManager::Job::~Job() {
+}
+
 MHTMLGenerationManager::MHTMLGenerationManager() {
 }
 
@@ -29,7 +32,8 @@ MHTMLGenerationManager::~MHTMLGenerationManager() {
 }
 
 void MHTMLGenerationManager::GenerateMHTML(TabContents* tab_contents,
-                                           const FilePath& file) {
+    const FilePath& file,
+    const GenerateMHTMLCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   static int id_counter = 0;
 
@@ -38,6 +42,7 @@ void MHTMLGenerationManager::GenerateMHTML(TabContents* tab_contents,
   job.file_path = file;
   job.process_id = tab_contents->GetRenderProcessHost()->GetID();
   job.routing_id = tab_contents->render_view_host()->routing_id();
+  job.callback = callback;
   id_to_job_[job_id] = job;
 
   base::ProcessHandle renderer_process =
@@ -109,18 +114,7 @@ void MHTMLGenerationManager::JobFinished(int job_id, int64 file_size) {
   }
 
   Job& job = iter->second;
-
-  RenderViewHost* rvh = RenderViewHost::FromID(job.process_id, job.routing_id);
-  if (rvh) {
-    NotificationDetails details;
-    details.file_path = job.file_path;
-    details.file_size = file_size;
-
-    content::NotificationService::current()->Notify(
-        content::NOTIFICATION_MHTML_GENERATED,
-        content::Source<RenderViewHost>(rvh),
-        content::Details<NotificationDetails>(&details));
-  }
+  job.callback.Run(job.file_path, file_size);
 
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
       base::Bind(&MHTMLGenerationManager::CloseFile, this, job.browser_file));

content/browser/download/mhtml_generation_manager.h

@@ -24,24 +24,24 @@ class CONTENT_EXPORT MHTMLGenerationManager
   MHTMLGenerationManager();
   ~MHTMLGenerationManager();
 
+  typedef base::Callback<void(const FilePath& /* path to the MHTML file */,
+      int64 /* size of the file */)> GenerateMHTMLCallback;
+
   // Instructs the render view to generate a MHTML representation of the current
   // page for |tab_contents|.
-  void GenerateMHTML(TabContents* tab_contents, const FilePath& file);
+  void GenerateMHTML(TabContents* tab_contents,
+                     const FilePath& file,
+                     const GenerateMHTMLCallback& callback);
 
   // Notification from the renderer that the MHTML generation finished.
   // |mhtml_data_size| contains the size in bytes of the generated MHTML data,
   // or -1 in case of failure.
   void MHTMLGenerated(int job_id, int64 mhtml_data_size);
 
-  // The details sent along with the MHTML_GENERATED notification.
-  struct NotificationDetails {
-    FilePath file_path;
-    int64 file_size;
-  };
-
  private:
   struct Job{
     Job();
+    ~Job();
 
     FilePath file_path;
 
@@ -53,6 +53,9 @@ class CONTENT_EXPORT MHTMLGenerationManager
     // The IDs mapping to a specific tab.
     int process_id;
     int routing_id;
+
+    // The callback to call once generation is complete.
+    GenerateMHTMLCallback callback;
   };
 
   // Called on the file thread to create |file|.

content/public/browser/notification_types.h

@@ -408,13 +408,6 @@ enum NotificationType {
   // in a Details<ChildProcessInfo>.
   NOTIFICATION_CHILD_INSTANCE_CREATED,
 
-  // Download Notifications --------------------------------------------------
-
-  // Sent when a page generation to MHTML has finished.
-  // The source is the corresponding RenderViewHost. The details is a
-  // MHTMLGenerationManager::NotificationDetails.
-  NOTIFICATION_MHTML_GENERATED,
-
   // Saved Pages -------------------------------------------------------------
 
   // Sent when a SavePackage finishes successfully. The source is the