Clean up WebNavigationObserver by taking out password specific callbacks, and...

Clean up WebNavigationObserver by taking out password specific callbacks, and make PasswordManager just dispatch the IPC message directly to achieve this.  I've also combined WebNavigationObserver with IPC message filtering (I hadn't seen the former when I added it).  This allows a little more cleanup in AutoFillManager.
Review URL: http://codereview.chromium.org/6368011

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

chrome/browser/autocomplete_history_manager.h

@@ -9,8 +9,8 @@
 #include <vector>
 
 #include "chrome/browser/prefs/pref_member.h"
+#include "chrome/browser/tab_contents/web_navigation_observer.h"
 #include "chrome/browser/webdata/web_data_service.h"
-#include "ipc/ipc_channel.h"
 
 namespace webkit_glue {
 struct FormData;
@@ -21,13 +21,13 @@ class TabContents;
 
 // Per-tab Autocomplete history manager. Handles receiving form data from the
 // renderer and the storing and retrieving of form data through WebDataService.
-class AutocompleteHistoryManager : public IPC::Channel::Listener,
+class AutocompleteHistoryManager : public WebNavigationObserver,
                                    public WebDataServiceConsumer {
  public:
   explicit AutocompleteHistoryManager(TabContents* tab_contents);
   virtual ~AutocompleteHistoryManager();
 
-  // IPC::Channel::Listener implementation.
+  // WebNavigationObserver implementation.
   virtual bool OnMessageReceived(const IPC::Message& message);
 
   // WebDataServiceConsumer implementation.

chrome/browser/autofill/autofill_manager.cc

@@ -171,6 +171,12 @@ void AutoFillManager::RegisterUserPrefs(PrefService* prefs) {
                           kAutoFillNegativeUploadRateDefaultValue);
 }
 
+void AutoFillManager::DidNavigateMainFramePostCommit(
+    const NavigationController::LoadCommittedDetails& details,
+    const ViewHostMsg_FrameNavigate_Params& params) {
+  Reset();
+}
+
 bool AutoFillManager::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(AutoFillManager, message)

chrome/browser/autofill/autofill_manager.h

@@ -16,7 +16,7 @@
 #include "chrome/browser/autofill/autofill_dialog.h"
 #include "chrome/browser/autofill/autofill_download.h"
 #include "chrome/browser/autofill/personal_data_manager.h"
-#include "ipc/ipc_channel.h"
+#include "chrome/browser/tab_contents/web_navigation_observer.h"
 
 class AutoFillCCInfoBarDelegate;
 class AutoFillProfile;
@@ -25,7 +25,6 @@ class CreditCard;
 class FormStructure;
 class PrefService;
 class RenderViewHost;
-class TabContents;
 
 namespace webkit_glue {
 struct FormData;
@@ -34,7 +33,7 @@ class FormField;
 
 // Manages saving and restoring the user's personal information entered into web
 // forms.
-class AutoFillManager : public IPC::Channel::Listener,
+class AutoFillManager : public WebNavigationObserver,
                         public AutoFillDownloadManager::Observer {
  public:
   explicit AutoFillManager(TabContents* tab_contents);
@@ -49,7 +48,10 @@ class AutoFillManager : public IPC::Channel::Listener,
   // Returns the TabContents hosting this AutoFillManager.
   TabContents* tab_contents() const { return tab_contents_; }
 
-  // IPC::Channel::Listener implementation.
+  // WebNavigationObserver implementation.
+  virtual void DidNavigateMainFramePostCommit(
+      const NavigationController::LoadCommittedDetails& details,
+      const ViewHostMsg_FrameNavigate_Params& params);
   virtual bool OnMessageReceived(const IPC::Message& message);
 
   // Called by the AutoFillCCInfoBarDelegate when the user interacts with the

chrome/browser/password_manager/password_manager.cc

@@ -16,6 +16,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/notification_registrar.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/common/render_messages.h"
 #include "chrome/common/render_messages_params.h"
 #include "grit/generated_resources.h"
 
@@ -158,7 +159,18 @@ void PasswordManager::DidNavigateAnyFramePostCommit(
     ProvisionallySavePassword(params.password_form);
 }
 
-void PasswordManager::PasswordFormsFound(
+bool PasswordManager::OnMessageReceived(const IPC::Message& message) {
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(PasswordManager, message)
+    IPC_MESSAGE_HANDLER(ViewHostMsg_PasswordFormsFound, OnPasswordFormsFound)
+    IPC_MESSAGE_HANDLER(ViewHostMsg_PasswordFormsVisible,
+                        OnPasswordFormsVisible)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
+}
+
+void PasswordManager::OnPasswordFormsFound(
     const std::vector<PasswordForm>& forms) {
   if (!delegate_->GetProfileForPasswordManager())
     return;
@@ -179,7 +191,7 @@ void PasswordManager::PasswordFormsFound(
   }
 }
 
-void PasswordManager::PasswordFormsVisible(
+void PasswordManager::OnPasswordFormsVisible(
     const std::vector<PasswordForm>& visible_forms) {
   if (!provisional_save_manager_.get())
     return;

chrome/browser/password_manager/password_manager.h

@@ -53,9 +53,11 @@ class PasswordManager : public LoginModel,
   virtual void DidNavigateAnyFramePostCommit(
       const NavigationController::LoadCommittedDetails& details,
       const ViewHostMsg_FrameNavigate_Params& params);
-  virtual void PasswordFormsFound(
+  virtual bool OnMessageReceived(const IPC::Message& message);
+
+  void OnPasswordFormsFound(
       const std::vector<webkit_glue::PasswordForm>& forms);
-  virtual void PasswordFormsVisible(
+  void OnPasswordFormsVisible(
       const std::vector<webkit_glue::PasswordForm>& visible_forms);
 
  private:

chrome/browser/password_manager/password_manager_unittest.cc

@@ -137,8 +137,8 @@ TEST_F(PasswordManagerTest, FormSubmitEmptyStore) {
   std::vector<PasswordForm> observed;
   PasswordForm form(MakeSimpleForm());
   observed.push_back(form);
-  manager()->PasswordFormsFound(observed);  // The initial load.
-  manager()->PasswordFormsVisible(observed);  // The initial layout.
+  manager()->OnPasswordFormsFound(observed);  // The initial load.
+  manager()->OnPasswordFormsVisible(observed);  // The initial layout.
 
   // And the form submit contract is to call ProvisionallySavePassword.
   manager()->ProvisionallySavePassword(form);
@@ -172,8 +172,8 @@ TEST_F(PasswordManagerTest, FormSubmitNoGoodMatch) {
   std::vector<PasswordForm> observed;
   PasswordForm form(MakeSimpleForm());
   observed.push_back(form);
-  manager()->PasswordFormsFound(observed);  // The initial load.
-  manager()->PasswordFormsVisible(observed);  // The initial layout.
+  manager()->OnPasswordFormsFound(observed);  // The initial load.
+  manager()->OnPasswordFormsVisible(observed);  // The initial layout.
   manager()->ProvisionallySavePassword(form);
 
   // We still expect an add, since we didn't have a good match.
@@ -196,8 +196,8 @@ TEST_F(PasswordManagerTest, FormSeenThenLeftPage) {
   std::vector<PasswordForm> observed;
   PasswordForm form(MakeSimpleForm());
   observed.push_back(form);
-  manager()->PasswordFormsFound(observed);  // The initial load.
-  manager()->PasswordFormsVisible(observed);  // The initial layout.
+  manager()->OnPasswordFormsFound(observed);  // The initial load.
+  manager()->OnPasswordFormsVisible(observed);  // The initial layout.
 
   manager()->DidNavigate();
 
@@ -213,14 +213,14 @@ TEST_F(PasswordManagerTest, FormSubmitFailedLogin) {
   std::vector<PasswordForm> observed;
   PasswordForm form(MakeSimpleForm());
   observed.push_back(form);
-  manager()->PasswordFormsFound(observed);  // The initial load.
-  manager()->PasswordFormsVisible(observed);  // The initial layout.
+  manager()->OnPasswordFormsFound(observed);  // The initial load.
+  manager()->OnPasswordFormsVisible(observed);  // The initial layout.
 
   manager()->ProvisionallySavePassword(form);
 
   // The form reappears, and is visible in the layout:
-  manager()->PasswordFormsFound(observed);
-  manager()->PasswordFormsVisible(observed);
+  manager()->OnPasswordFormsFound(observed);
+  manager()->OnPasswordFormsVisible(observed);
 
   // No expected calls to the PasswordStore...
   manager()->DidStopLoading();
@@ -236,13 +236,13 @@ TEST_F(PasswordManagerTest, FormSubmitInvisibleLogin) {
   std::vector<PasswordForm> observed;
   PasswordForm form(MakeSimpleForm());
   observed.push_back(form);
-  manager()->PasswordFormsFound(observed);  // The initial load.
-  manager()->PasswordFormsVisible(observed);  // The initial layout.
+  manager()->OnPasswordFormsFound(observed);  // The initial load.
+  manager()->OnPasswordFormsVisible(observed);  // The initial layout.
 
   manager()->ProvisionallySavePassword(form);
 
   // The form reappears, but is not visible in the layout:
-  manager()->PasswordFormsFound(observed);
+  manager()->OnPasswordFormsFound(observed);
   // No call to PasswordFormsVisible.
 
   // Expect info bar to appear:
@@ -269,7 +269,7 @@ TEST_F(PasswordManagerTest, InitiallyInvisibleForm) {
   std::vector<PasswordForm> observed;
   PasswordForm form(MakeSimpleForm());
   observed.push_back(form);
-  manager()->PasswordFormsFound(observed);  // The initial load.
+  manager()->OnPasswordFormsFound(observed);  // The initial load.
   // PasswordFormsVisible is not called.
 
   manager()->DidStopLoading();

chrome/browser/renderer_host/render_view_host.cc

@@ -784,9 +784,6 @@ bool RenderViewHost::OnMessageReceived(const IPC::Message& msg) {
                                     OnMsgRunBeforeUnloadConfirm)
     IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewHostMsg_ShowModalHTMLDialog,
                                     OnMsgShowModalHTMLDialog)
-    IPC_MESSAGE_HANDLER(ViewHostMsg_PasswordFormsFound, OnMsgPasswordFormsFound)
-    IPC_MESSAGE_HANDLER(ViewHostMsg_PasswordFormsVisible,
-                        OnMsgPasswordFormsVisible)
     IPC_MESSAGE_HANDLER(ViewHostMsg_StartDragging, OnMsgStartDragging)
     IPC_MESSAGE_HANDLER(ViewHostMsg_UpdateDragCursor, OnUpdateDragCursor)
     IPC_MESSAGE_HANDLER(ViewHostMsg_TakeFocus, OnTakeFocus)
@@ -1361,16 +1358,6 @@ void RenderViewHost::PrintNodeUnderContextMenu() {
   Send(new ViewMsg_PrintNodeUnderContextMenu(routing_id()));
 }
 
-void RenderViewHost::OnMsgPasswordFormsFound(
-    const std::vector<PasswordForm>& forms) {
-  delegate_->PasswordFormsFound(forms);
-}
-
-void RenderViewHost::OnMsgPasswordFormsVisible(
-    const std::vector<PasswordForm>& visible_forms) {
-  delegate_->PasswordFormsVisible(visible_forms);
-}
-
 void RenderViewHost::OnMsgStartDragging(
     const WebDropData& drop_data,
     WebDragOperationsMask drag_operations_mask,
@@ -1729,18 +1716,6 @@ void RenderViewHost::PerformCustomContextMenuAction(unsigned action) {
   Send(new ViewMsg_CustomContextMenuAction(routing_id(), action));
 }
 
-void RenderViewHost::TranslatePage(int page_id,
-                                   const std::string& translate_script,
-                                   const std::string& source_lang,
-                                   const std::string& target_lang) {
-  Send(new ViewMsg_TranslatePage(routing_id(), page_id, translate_script,
-                                 source_lang, target_lang));
-}
-
-void RenderViewHost::RevertTranslation(int page_id) {
-  Send(new ViewMsg_RevertTranslation(routing_id(), page_id));
-}
-
 void RenderViewHost::SendContentSettings(const GURL& url,
                                          const ContentSettings& settings) {
   Send(new ViewMsg_SetContentSettingsForCurrentURL(url, settings));

chrome/browser/renderer_host/render_view_host.h

@@ -474,18 +474,6 @@ class RenderViewHost : public RenderWidgetHost {
   // Tells the render view that a custom context action has been selected.
   void PerformCustomContextMenuAction(unsigned action);
 
-  // Tells the renderer to translate the current page from one language to
-  // another.  If the current page id is not |page_id|, the request is ignored.
-  // |translate_script| is the script that should be injected in the page to
-  // perform the translation.
-  void TranslatePage(int page_id,
-                     const std::string& translate_script,
-                     const std::string& source_lang,
-                     const std::string& target_lang);
-
-  // Reverts the text of current page to its original (non-translated) contents.
-  void RevertTranslation(int page_id);
-
   // Informs renderer of updated content settings.
   void SendContentSettings(const GURL& url,
                            const ContentSettings& settings);
@@ -621,10 +609,6 @@ class RenderViewHost : public RenderWidgetHost {
   void OnMsgShowModalHTMLDialog(const GURL& url, int width, int height,
                                 const std::string& json_arguments,
                                 IPC::Message* reply_msg);
-  void OnMsgPasswordFormsFound(
-      const std::vector<webkit_glue::PasswordForm>& forms);
-  void OnMsgPasswordFormsVisible(
-      const std::vector<webkit_glue::PasswordForm>& visible_forms);
   void OnMsgStartDragging(const WebDropData& drop_data,
                           WebKit::WebDragOperationsMask operations_allowed,
                           const SkBitmap& image,

chrome/browser/renderer_host/render_view_host_delegate.h

@@ -690,14 +690,6 @@ class RenderViewHostDelegate : public IPC::Channel::Listener {
                                    const std::string& json_arguments,
                                    IPC::Message* reply_msg) {}
 
-  // Password forms have been detected in the page.
-  virtual void PasswordFormsFound(
-      const std::vector<webkit_glue::PasswordForm>& forms) {}
-
-  // On initial layout, password forms are known to be visible on the page.
-  virtual void PasswordFormsVisible(
-      const std::vector<webkit_glue::PasswordForm>& visible_forms) {}
-
   // Notification that the page has an OpenSearch description document.
   virtual void PageHasOSDD(RenderViewHost* render_view_host,
                            int32 page_id, const GURL& doc_url,

chrome/browser/tab_contents/tab_contents.cc

@@ -449,9 +449,9 @@ TabContents::TabContents(Profile* profile,
     omnibox_search_hint_.reset(new OmniboxSearchHint(this));
 
   autofill_manager_.reset(new AutoFillManager(this));
-  message_filters_.push_back(autofill_manager_.get());
+  AddNavigationObserver(autofill_manager_.get());
   autocomplete_history_manager_.reset(new AutocompleteHistoryManager(this));
-  message_filters_.push_back(autocomplete_history_manager_.get());
+  AddNavigationObserver(autocomplete_history_manager_.get());
 }
 
 TabContents::~TabContents() {
@@ -571,10 +571,12 @@ void TabContents::RegisterUserPrefs(PrefService* prefs) {
 }
 
 bool TabContents::OnMessageReceived(const IPC::Message& message) {
-  for (size_t i = 0; i < message_filters_.size(); ++i) {
-    if (message_filters_[i]->OnMessageReceived(message))
+  ObserverListBase<WebNavigationObserver>::Iterator it(
+      web_navigation_observers_);
+  WebNavigationObserver* observer;
+  while ((observer = it.GetNext()) != NULL)
+    if (observer->OnMessageReceived(message))
       return true;
-  }
 
   bool handled = true;
   bool message_is_ok = true;
@@ -1986,9 +1988,6 @@ void TabContents::DidNavigateMainFramePostCommit(
   // Notify observers about navigation.
   FOR_EACH_OBSERVER(WebNavigationObserver, web_navigation_observers_,
                     DidNavigateMainFramePostCommit(details, params));
-
-  // Clear the cache of forms in AutoFill.
-  autofill_manager_->Reset();
 }
 
 void TabContents::DidNavigateAnyFramePostCommit(
@@ -2933,18 +2932,6 @@ void TabContents::ShowModalHTMLDialog(const GURL& url, int width, int height,
   }
 }
 
-void TabContents::PasswordFormsFound(
-    const std::vector<webkit_glue::PasswordForm>& forms) {
-  FOR_EACH_OBSERVER(WebNavigationObserver, web_navigation_observers_,
-                    PasswordFormsFound(forms));
-}
-
-void TabContents::PasswordFormsVisible(
-    const std::vector<webkit_glue::PasswordForm>& visible_forms) {
-  FOR_EACH_OBSERVER(WebNavigationObserver, web_navigation_observers_,
-                    PasswordFormsVisible(visible_forms));
-}
-
 // Checks to see if we should generate a keyword based on the OSDD, and if
 // necessary uses TemplateURLFetcher to download the OSDD and create a keyword.
 void TabContents::PageHasOSDD(

chrome/browser/tab_contents/tab_contents.h

@@ -55,10 +55,6 @@ namespace printing {
 class PrintViewManager;
 }
 
-namespace webkit_glue {
-struct PasswordForm;
-}
-
 class AutocompleteHistoryManager;
 class AutoFillManager;
 class BlockedContentContainer;
@@ -767,6 +763,7 @@ class TabContents : public PageNavigator,
   FRIEND_TEST_ALL_PREFIXES(TabContentsTest, NoJSMessageOnInterstitials);
   FRIEND_TEST_ALL_PREFIXES(TabContentsTest, UpdateTitle);
   FRIEND_TEST_ALL_PREFIXES(TabContentsTest, CrossSiteCantPreemptAfterUnload);
+  FRIEND_TEST_ALL_PREFIXES(FormStructureBrowserTest, HTMLFiles);
 
   // Temporary until the view/contents separation is complete.
   friend class TabContentsView;
@@ -1014,10 +1011,6 @@ class TabContents : public PageNavigator,
   virtual void ShowModalHTMLDialog(const GURL& url, int width, int height,
                                    const std::string& json_arguments,
                                    IPC::Message* reply_msg);
-  virtual void PasswordFormsFound(
-      const std::vector<webkit_glue::PasswordForm>& forms);
-  virtual void PasswordFormsVisible(
-      const std::vector<webkit_glue::PasswordForm>& visible_forms);
   virtual void PageHasOSDD(RenderViewHost* render_view_host,
                            int32 page_id,
                            const GURL& url,
@@ -1349,9 +1342,6 @@ class TabContents : public PageNavigator,
   // (full-page plugins for now only) permissions.
   int content_restrictions_;
 
-  // All the IPC message filters for this render view.
-  std::vector<IPC::Channel::Listener*> message_filters_;
-
   DISALLOW_COPY_AND_ASSIGN(TabContents);
 };
 

chrome/browser/tab_contents/web_navigation_observer.h

@@ -6,20 +6,14 @@
 #define CHROME_BROWSER_TAB_CONTENTS_WEB_NAVIGATION_OBSERVER_H_
 
 #include "chrome/browser/tab_contents/navigation_controller.h"
+#include "ipc/ipc_channel.h"
 
 struct ViewHostMsg_FrameNavigate_Params;
 
 // An observer API implemented by classes which are interested in various page
-// load events from TabContents.
-
-// TODO(pink): Is it worth having a bitfield where certain clients can only
-// register for certain events? Is the extra function call worth the added pain
-// on the caller to build the bitfield?
-
-class WebNavigationObserver {
+// load events from TabContents.  They also get a chance to filter IPC messages.
+class WebNavigationObserver : public IPC::Channel::Listener {
  public:
-  // For removing PasswordManager deps...
-
   virtual void NavigateToPendingEntry() { }
 
   virtual void DidNavigateMainFramePostCommit(
@@ -32,13 +26,8 @@ class WebNavigationObserver {
   virtual void DidStartLoading() { }
   virtual void DidStopLoading() { }
 
-  // TODO(beng): These should move from here once PasswordManager is able to
-  //             establish its own private communication protocol to the
-  //             renderer.
-  virtual void PasswordFormsFound(
-      const std::vector<webkit_glue::PasswordForm>& forms) { }
-  virtual void PasswordFormsVisible(
-      const std::vector<webkit_glue::PasswordForm>& visible_forms) { }
+  // IPC::Channel::Listener implementation.
+  virtual bool OnMessageReceived(const IPC::Message& message) { return false; }
 
 #if 0
   // For unifying with delegate...

chrome/browser/translate/translate_manager.cc

@@ -33,6 +33,7 @@
 #include "chrome/common/notification_source.h"
 #include "chrome/common/notification_type.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/common/render_messages.h"
 #include "chrome/common/translate_errors.h"
 #include "chrome/common/url_constants.h"
 #include "grit/browser_resources.h"
@@ -473,7 +474,8 @@ void TranslateManager::RevertTranslation(TabContents* tab_contents) {
     NOTREACHED();
     return;
   }
-  tab_contents->render_view_host()->RevertTranslation(entry->page_id());
+  tab_contents->render_view_host()->Send(new ViewMsg_RevertTranslation(
+      tab_contents->render_view_host()->routing_id(), entry->page_id()));
   tab_contents->language_state().set_current_language(
       tab_contents->language_state().original_language());
 }
@@ -510,8 +512,10 @@ void TranslateManager::DoTranslatePage(TabContents* tab,
   }
 
   tab->language_state().set_translation_pending(true);
-  tab->render_view_host()->TranslatePage(entry->page_id(), translate_script,
-                                         source_lang, target_lang);
+
+  tab->render_view_host()->Send(new ViewMsg_TranslatePage(
+      tab->render_view_host()->routing_id(), entry->page_id(), translate_script,
+      source_lang, target_lang));
 
   // Ideally we'd have a better way to uniquely identify form control elements,
   // but we don't have that yet.  So before start translation, we clear the

chrome/browser/ui/login/login_prompt.cc

@@ -398,7 +398,7 @@ class LoginDialogTask : public Task {
     PasswordManager* password_manager = (*wrapper)->GetPasswordManager();
     std::vector<PasswordForm> v;
     MakeInputForPasswordManager(&v);
-    password_manager->PasswordFormsFound(v);
+    password_manager->OnPasswordFormsFound(v);
     handler_->SetPasswordManager(password_manager);
 
     string16 host_and_port_hack16 = WideToUTF16Hack(auth_info_->host_and_port);