PlzNavigate: add cancel navigation logic for uncommitted requests

content/browser/frame_host/navigation_request.h

Index: content/browser/frame_host/navigation_request.h
diff --git a/content/browser/frame_host/navigation_request.h b/content/browser/frame_host/navigation_request.h
index 6cc1cd2fe98b6bcaf4a44e74ae2e4f66ffeb3eca..c9f7fdbaa2776fbdff8ae586cdd183354c53fd79 100644
--- a/content/browser/frame_host/navigation_request.h
+++ b/content/browser/frame_host/navigation_request.h
@@ -8,6 +8,7 @@
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
 #include "content/browser/frame_host/navigation_request_info.h"
+#include "content/common/content_export.h"
 
 namespace content {
 class ResourceRequestBody;
@@ -20,21 +21,59 @@ class ResourceRequestBody;
 // the navigation following its refactoring.
 class NavigationRequest {
  public:
+
+  // Implementors of NavigationRequest::Observer will be notified of important
+  // events happening in the life-cycle of the NavigationRequest instance they
+  // register for.
+  class Observer {

[clamy, 2014/08/28 12:17:01]

The CONTENT_EXPORT should be placed before the class declaration rather than in
front of the methods.

[(Do not use) nasko, 2014/08/28 15:55:05]

This class doesn't seem to follow very well how other observers works. I don't
know of any case we have where we hardcode only one observer. Also, if it is
only used for testing, it should be clearly labeled as such.

+   public:
+    CONTENT_EXPORT Observer();

[clamy, 2014/08/28 12:17:01]

The constructors and the destructor should be protected.

Also the empty constructor is not used anywhere, so I think it should be
removed.

[carlosk, 2014/08/29 15:54:43]

Done.

+
+    CONTENT_EXPORT explicit Observer(NavigationRequest* nv);
+
+    CONTENT_EXPORT virtual ~Observer();
+
+    virtual void RequestCanceled(NavigationRequest* nv) = 0;
+
+    virtual void RequestDestroyed(NavigationRequest* nv) = 0;
+
+    // NOTE: This method is automatically called by NavigationRequest when an
+    // observer is set to it. Users should not need to do this manually.
+    void set_navigation_request(NavigationRequest* nv) {

[clamy, 2014/08/28 12:17:01]

With further reading of the code, it is apparently possible to make
NavigationRequest a friend of this class. In that case this setter should be
removed.

+      navigation_request_ = nv; }
+
+   private:
+     NavigationRequest* navigation_request_;
+  };
+
   NavigationRequest(const NavigationRequestInfo& info, int64 frame_node_id);
 
   ~NavigationRequest();
 
-  const NavigationRequestInfo& info() const { return info_; }
-  int64 frame_node_id() const { return frame_node_id_; }
-
   // Called on the UI thread by the RenderFrameHostManager which owns the
   // NavigationRequest. After calling this function, |body| can no longer be
   // manipulated on the UI thread.
   void BeginNavigation(scoped_refptr<ResourceRequestBody> body);
 
+  // Called on the UI thread by the RenderFrameHostManager which owns the
+  // NavigationRequest whenever this navigation request should be canceled.
+  void CancelNavigation();
+
+  // Used to register and unregister a single observer for this instance. This
+  // call automatically sets the navigation request instance into the observer.
+  void SetSingleObserver(NavigationRequest::Observer* observer);
+
+  const NavigationRequestInfo& info() const { return info_; }
+
+  int64 frame_node_id() const { return frame_node_id_; }
+
+  int64 navigation_request_id() const { return navigation_request_id_; }
+
  private:
+  const int64 navigation_request_id_;
   const NavigationRequestInfo info_;
   const int64 frame_node_id_;
+  Observer* single_observer_;

[clamy, 2014/08/28 12:17:01]

I really don't like this single observer pattern. The other observer classes in
the code follow a different pattern in which you can have any number of
observers, stored in an ObserverList (see base/observer_list.h), with two
functions AddObserver and RemoveObserver. I think the NavigationRequestObserver
should follow this pattern as well, because other people will not expect that
adding an observer to the class will remove the previous one.

The ObserverList also comes with a FOR_EACH_OBSERVER macro that should be used
to call the appropriate functions on each observer.

 
   DISALLOW_COPY_AND_ASSIGN(NavigationRequest);
 };