Delete the provisional load commit notification since it duplicates the nav entry committed notification.

I had to add some more stuff to the nav entry committed structure which now looks suspiciously like the provisional load details structure. I'll see how I can improve this in a future pass.

I used the new NotificationRegistrar to automatically unregister for notifications in the SSL manager, which reduces some code.
Review URL: http://codereview.chromium.org/3095

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

5 files changed, 60 insertions, 55 deletions

diff --git a/chrome/browser/navigation_controller.cc b/chrome/browser/navigation_controller.cc
index 58781be..31ff778 100644
--- a/chrome/browser/navigation_controller.cc
+++ b/chrome/browser/navigation_controller.cc
@@ -559,23 +559,10 @@ bool NavigationController::RendererDidNavigate(
   details->entry = GetActiveEntry();
   details->is_in_page = IsURLInPageNavigation(params.url);
   details->is_main_frame = PageTransition::IsMainFrame(params.transition);
+  details->is_interstitial = is_interstitial;
+  details->serialized_security_info = params.security_info;
   NotifyNavigationEntryCommitted(details);
 
-  // Broadcast the NOTIFY_FRAME_PROVISIONAL_LOAD_COMMITTED notification for use
-  // by the SSL manager.
-  //
-  // TODO(brettw) bug 1352803: this information should be combined with
-  // NOTIFY_NAV_ENTRY_COMMITTED so this one can be deleted.
-  ProvisionalLoadDetails provisional_details(details->is_main_frame,
-                                             is_interstitial,
-                                             details->is_in_page,
-                                             params.url,
-                                             params.security_info);
-  NotificationService::current()->
-      Notify(NOTIFY_FRAME_PROVISIONAL_LOAD_COMMITTED,
-             Source<NavigationController>(this),
-             Details<ProvisionalLoadDetails>(&provisional_details));
-
   // It is now a safe time to schedule collection for any tab contents of a
   // different type, because a navigation is necessary to get back to them.
   ScheduleTabContentsCollectionForInactiveTabs();
diff --git a/chrome/browser/navigation_controller.h b/chrome/browser/navigation_controller.h
index f0e6096..0e1b5919 100644
--- a/chrome/browser/navigation_controller.h
+++ b/chrome/browser/navigation_controller.h
@@ -41,6 +41,8 @@ class NavigationController {
   };
 
   // Provides the details for a NOTIFY_NAV_ENTRY_COMMITTED notification.
+  // TODO(brettw) this mostly duplicates ProvisionalLoadDetails, it would be
+  // nice to unify these somehow.
   struct LoadCommittedDetails {
     // By default, the entry will be filled according to a new main frame
     // navigation.
@@ -48,7 +50,8 @@ class NavigationController {
         : entry(NULL),
           is_auto(false),
           is_in_page(false),
-          is_main_frame(true) {
+          is_main_frame(true),
+          is_interstitial(false) {
     }
 
     // The committed entry. This will be the active entry in the controller.
@@ -72,6 +75,16 @@ class NavigationController {
     // sub-frame.
     bool is_main_frame;
 
+    // True when this navigation is for an interstitial page. Many consumers
+    // won't care about interstitial loads.
+    bool is_interstitial;
+
+    // When the committed load is a web page from the renderer, this string
+    // specifies the security state if the page is secure.
+    // See ViewHostMsg_FrameNavigate_Params.security_info, where it comes from.
+    // Use SSLManager::DeserializeSecurityInfo to decode it.
+    std::string serialized_security_info;
+
     // Returns whether the user probably felt like they navigated somewhere new.
     // We often need this logic for showing or hiding something, and this
     // returns true only for main frame loads that the user initiated, that go
diff --git a/chrome/browser/provisional_load_details.h b/chrome/browser/provisional_load_details.h
index 3094d13..2c342c1 100644
--- a/chrome/browser/provisional_load_details.h
+++ b/chrome/browser/provisional_load_details.h
@@ -14,6 +14,9 @@
 // and NOTIFY_FAIL_PROVISIONAL_LOAD_WITH_ERROR notifications
 // (see notification_types.h).
 
+// TODO(brettw) this mostly duplicates
+// NavigationController::LoadCommittedDetails, it would be nice to unify these
+// somehow.
 class ProvisionalLoadDetails {
  public:
   ProvisionalLoadDetails(bool main_frame,
diff --git a/chrome/browser/ssl_manager.cc b/chrome/browser/ssl_manager.cc
index ffb0645..0d03c3e 100644
--- a/chrome/browser/ssl_manager.cc
+++ b/chrome/browser/ssl_manager.cc
@@ -133,35 +133,21 @@ SSLManager::SSLManager(NavigationController* controller, Delegate* delegate)
     delegate_ = SSLPolicy::GetDefaultPolicy();
 
   // Subscribe to various notifications.
-  NotificationService* service = NotificationService::current();
-  service->AddObserver(this, NOTIFY_FRAME_PROVISIONAL_LOAD_COMMITTED,
-                       Source<NavigationController>(controller_));
-  service->AddObserver(this, NOTIFY_FAIL_PROVISIONAL_LOAD_WITH_ERROR,
-                       Source<NavigationController>(controller_));
-  service->AddObserver(this, NOTIFY_RESOURCE_RESPONSE_STARTED,
-                       Source<NavigationController>(controller_));
-  service->AddObserver(this, NOTIFY_RESOURCE_RECEIVED_REDIRECT,
-                       Source<NavigationController>(controller_));
-  service->AddObserver(this, NOTIFY_LOAD_FROM_MEMORY_CACHE,
-                       Source<NavigationController>(controller_));
+  registrar_.Add(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                 Source<NavigationController>(controller_));
+  registrar_.Add(this, NOTIFY_FAIL_PROVISIONAL_LOAD_WITH_ERROR,
+                 Source<NavigationController>(controller_));
+  registrar_.Add(this, NOTIFY_RESOURCE_RESPONSE_STARTED,
+                 Source<NavigationController>(controller_));
+  registrar_.Add(this, NOTIFY_RESOURCE_RECEIVED_REDIRECT,
+                 Source<NavigationController>(controller_));
+  registrar_.Add(this, NOTIFY_LOAD_FROM_MEMORY_CACHE,
+                 Source<NavigationController>(controller_));
 }
 
 SSLManager::~SSLManager() {
   // Close any of our info bars that are still left.
   FOR_EACH_OBSERVER(SSLInfoBar, visible_info_bars_, Close());
-
-  // Unsubscribe from various notifications.
-  NotificationService* service = NotificationService::current();
-  service->RemoveObserver(this, NOTIFY_FRAME_PROVISIONAL_LOAD_COMMITTED,
-                          Source<NavigationController>(controller_));
-  service->RemoveObserver(this, NOTIFY_FAIL_PROVISIONAL_LOAD_WITH_ERROR,
-                          Source<NavigationController>(controller_));
-  service->RemoveObserver(this, NOTIFY_RESOURCE_RESPONSE_STARTED,
-                          Source<NavigationController>(controller_));
-  service->RemoveObserver(this, NOTIFY_RESOURCE_RECEIVED_REDIRECT,
-                          Source<NavigationController>(controller_));
-  service->RemoveObserver(this, NOTIFY_LOAD_FROM_MEMORY_CACHE,
-                          Source<NavigationController>(controller_));
 }
 
 // Delegate API method.
@@ -552,8 +538,8 @@ void SSLManager::Observe(NotificationType type,
 
   // Dispatch by type.
   switch (type) {
-    case NOTIFY_FRAME_PROVISIONAL_LOAD_COMMITTED:
-      DidCommitProvisionalLoad(Details<ProvisionalLoadDetails>(details).ptr());
+    case NOTIFY_NAV_ENTRY_COMMITTED:
+      DidCommitProvisionalLoad(details);
       break;
     case NOTIFY_FAIL_PROVISIONAL_LOAD_WITH_ERROR:
       DidFailProvisionalLoadWithError(
@@ -607,14 +593,23 @@ void SSLManager::DidLoadFromMemoryCache(LoadFromMemoryCacheDetails* details) {
                                details->ssl_cert_status());
 }
 
-void SSLManager::DidCommitProvisionalLoad(ProvisionalLoadDetails* details) {
+void SSLManager::DidCommitProvisionalLoad(
+    const NotificationDetails& in_details) {
+  NavigationController::LoadCommittedDetails* details =
+      Details<NavigationController::LoadCommittedDetails>(in_details).ptr();
+
   // Ignore in-page navigations, they should not change the security style or
   // the info-bars.
-  if (details->in_page_navigation())
+  if (details->is_in_page)
     return;
 
+  // Decode the security details.
+  int ssl_cert_id, ssl_cert_status, ssl_security_bits;
+  DeserializeSecurityInfo(details->serialized_security_info,
+                          &ssl_cert_id, &ssl_cert_status, &ssl_security_bits);
+
   bool changed = false;
-  if (details->main_frame()) {
+  if (details->is_main_frame) {
     // We are navigating to a new page, it's time to close the info bars.  They
     // will automagically disappear from the visible_info_bars_ list (when
     // OnInfoBarClose is invoked).
@@ -631,13 +626,13 @@ void SSLManager::DidCommitProvisionalLoad(ProvisionalLoadDetails* details) {
       // page. Reset the SSL information and add the new data we have.
       entry->ssl() = NavigationEntry::SSLStatus();
       InitializeEntryIfNeeded(entry);  // For security_style.
-      entry->ssl().set_cert_id(details->ssl_cert_id());
-      entry->ssl().set_cert_status(details->ssl_cert_status());
-      entry->ssl().set_security_bits(details->ssl_security_bits());
+      entry->ssl().set_cert_id(ssl_cert_id);
+      entry->ssl().set_cert_status(ssl_cert_status);
+      entry->ssl().set_security_bits(ssl_security_bits);
       changed = true;
     }
 
-    if (details->interstitial_page()) {
+    if (details->is_interstitial) {
       // We should not have any errors when loading an interstitial page, and as
       // a consequence no messages.
       DCHECK(pending_messages_.empty());
@@ -648,9 +643,9 @@ void SSLManager::DidCommitProvisionalLoad(ProvisionalLoadDetails* details) {
   // An HTTPS response may not have a certificate for some reason.  When that
   // happens, use the unauthenticated (HTTP) rather than the authentication
   // broken security style so that we can detect this error condition.
-  if (net::IsCertStatusError(details->ssl_cert_status()))
+  if (net::IsCertStatusError(ssl_cert_status))
     changed |= SetMaxSecurityStyle(SECURITY_STYLE_AUTHENTICATION_BROKEN);
-  else if (details->url().SchemeIsSecure() && !details->ssl_cert_id())
+  else if (details->entry->url().SchemeIsSecure() && !ssl_cert_id)
     changed |= SetMaxSecurityStyle(SECURITY_STYLE_UNAUTHENTICATED);
 
   if (changed) {
diff --git a/chrome/browser/ssl_manager.h b/chrome/browser/ssl_manager.h
index 5d088a7..f0409b7 100644
--- a/chrome/browser/ssl_manager.h
+++ b/chrome/browser/ssl_manager.h
@@ -11,10 +11,11 @@
 #include "base/basictypes.h"
 #include "base/observer_list.h"
 #include "base/ref_counted.h"
-#include "chrome/browser/views/info_bar_message_view.h"
 #include "chrome/browser/provisional_load_details.h"
 #include "chrome/browser/resource_dispatcher_host.h"
 #include "chrome/browser/security_style.h"
+#include "chrome/browser/views/info_bar_message_view.h"
+#include "chrome/common/notification_registrar.h"
 #include "chrome/common/notification_service.h"
 #include "chrome/common/render_messages.h"
 #include "googleurl/src/gurl.h"
@@ -25,10 +26,10 @@
 #include "webkit/glue/resource_type.h"
 
 class InfoBarItemView;
-class NavigationController;
 class NavigationEntry;
 class LoadFromMemoryCacheDetails;
 class LoadNotificationDetails;
+class NavigationController;
 class PrefService;
 class ResourceRedirectDetails;
 class ResourceRequestDetails;
@@ -422,9 +423,12 @@ class SSLManager : public NotificationObserver {
      Task* action;
    };
 
-  // Entry points for notifications to which we subscribe.
+  // Entry points for notifications to which we subscribe. Note that
+  // DidCommitProvisionalLoad uses the abstract NotificationDetails type since
+  // the type we need is in NavigationController which would create a circular
+  // header file dependency.
   void DidLoadFromMemoryCache(LoadFromMemoryCacheDetails* details);
-  void DidCommitProvisionalLoad(ProvisionalLoadDetails* details);
+  void DidCommitProvisionalLoad(const NotificationDetails& details);
   void DidFailProvisionalLoadWithError(ProvisionalLoadDetails* details);
   void DidStartResourceResponse(ResourceRequestDetails* details);
   void DidReceiveResourceRedirect(ResourceRedirectDetails* details);
@@ -449,6 +453,9 @@ class SSLManager : public NotificationObserver {
   // The list of currently visible SSL InfoBars.
   ObserverList<SSLInfoBar> visible_info_bars_;
 
+  // Handles registering notifications with the NotificationService.
+  NotificationRegistrar registrar_;
+
   // Certificate policies for each host.
   std::map<std::string, net::X509Certificate::Policy> cert_policy_for_host_;