Track in which frames navigation errors occurred and don't send further navigation events for them

chrome/browser/extensions/extension_webnavigation_api.h

 // Copyright (c) 2010 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.
 
 // Defines the Chrome Extensions WebNavigation API functions for observing and
 // intercepting navigation events, as specified in
 // chrome/common/extensions/api/extension_api.json.
 
 #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_WEBNAVIGATION_API_H_
 #define CHROME_BROWSER_EXTENSIONS_EXTENSION_WEBNAVIGATION_API_H_
 #pragma once
 
 #include <map>
 
 #include "base/singleton.h"
 #include "chrome/browser/extensions/extension_function.h"
 #include "chrome/common/notification_observer.h"
 #include "chrome/common/notification_registrar.h"
 #include "googleurl/src/gurl.h"
 
 class NavigationController;
 class ProvisionalLoadDetails;
 class TabContents;
 
+// Tracks which frames are in an error state, and no navigation events should
+// be sent for.
+class FrameNavigationState : public NotificationObserver {

[yzshen, 2010/10/27 17:43:38]

1) The design says: For a specific *navigation*, there is no further events
fired after onErrorOccurred.
That doesn't mean that onErrorOccurred is guaranteed to be the last event from a
frame. Instead, it means that onErrorOccurred must be followed by an
onBeforeNavigate (the first event for a new navigation).
Does that make sense?
I think the implementation probably doesn't work for the following scenario:
onErrorOccurred in a subframe; a new navigation starts in that subframe.

2) Although this class is registered as a notification observer to listen to
TAB_CONTENTS_DESTROYED, it still relies on FRAME_PROVISIONAL_LOAD_START (via
TrackFrame).

To me, it seems better to keep FrameNavigationState simple (not register it as a
NotificationObserver), and let ExtensionWebNavigationEventRouter do the job of
listening to notifications.

[jochen (gone - plz use gerrit), 2010/10/27 18:01:14]

I think it works for that case :)

I'll add a unit test to verify
sure, why not

+ public:
+  FrameNavigationState();
+  ~FrameNavigationState();

[yzshen, 2010/10/27 17:43:38]

Please consider to add "virtual" for the destructor. (Although it works fine
without "virtual", I think it is more clear to have it.)

[jochen (gone - plz use gerrit), 2010/10/27 18:01:14]

why? It'll just make the destructor call more expensive

[yzshen, 2010/10/27 18:52:29]

Since ~NotificationObserver is declared as "virtual",
~FrameNavigationState is already virtual, no matter you add the "virtual"
keyword or not.

Moreover, it is required by the C++ style guide:
Make your destructor virtual if necessary. If your class has virtual methods,
its destructor should be virtual.
...
When redefining an inherited virtual function, explicitly declare it virtual in
the declaration of the derived class. Rationale: If virtual is omitted, the
reader has to check all ancestors of the class in question to determine if the
function is virtual or not. 

+
+  // True if navigation events for the given frame can be sent.
+  bool CanSendEvents(long long frame_id) const;
+
+  // Starts the track a frame given by its |frame_id| showing the URL |url| in

[yzshen, 2010/10/27 17:43:38]

Do you mean "Starts to track"?

+  // a |tab_contents|.
+  void TrackFrame(long long frame_id,
+                  const GURL& url,
+                  bool is_main_frame,
+                  const TabContents* tab_contents);
+
+  // Marks a frame as in an error state.
+  void ErrorOccurredInFrame(long long frame_id);
+
+ private:
+  typedef std::multimap<const TabContents*, long long> TabContentsToFrameIdMap;
+  typedef std::map<long long, bool> FrameIdToErrorStateMap;
+
+  // NotificationObserver implementation.
+  virtual void Observe(NotificationType type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details);
+
+  // Remove state associated with this tab contents and all of its frames.

[yzshen, 2010/10/27 17:43:38]

Remove -> Removes

+  void RemoveTabContentsState(const TabContents* tab_contents);
+
+  // Tracks which frames belong to a given tab contents object.
+  TabContentsToFrameIdMap tab_contents_map_;
+
+  // Tracks which frames are in an error state. Maps to true, if an error has
+  // occurred in a given frame.
+  FrameIdToErrorStateMap frame_state_map_;
+
+  // Used for tracking registrations to tab contents notifications.
+  NotificationRegistrar registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(FrameNavigationState);
+};
+
 // Observes navigation notifications and routes them as events to the extension
 // system.
 class ExtensionWebNavigationEventRouter : public NotificationObserver {
  public:
   // Single instance of the event router.
   static ExtensionWebNavigationEventRouter* GetInstance();
 
   void Init();
 
  private:
@@ skipping 23 matching lines @@
   // the details of such an event and constructs a suitable JSON formatted
   // extension event from it.
   void FailProvisionalLoadWithError(NavigationController* controller,
                                     ProvisionalLoadDetails* details);
 
   // This method dispatches events to the extension message service.
   void DispatchEvent(Profile* context,
                      const char* event_name,
                      const std::string& json_args);
 
+  // Tracks the state of the frames we are sending events for.
+  FrameNavigationState navigation_state_;
+
   // Used for tracking registrations to navigation notifications.
   NotificationRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionWebNavigationEventRouter);
 };
 
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_WEBNAVIGATION_API_H_