Tied up BlimpNavigationController to NavigationFeature

blimp/client/core/contents/blimp_navigation_controller_impl.h

Index: blimp/client/core/contents/blimp_navigation_controller_impl.h
diff --git a/blimp/client/core/contents/blimp_navigation_controller_impl.h b/blimp/client/core/contents/blimp_navigation_controller_impl.h
index 6ae6fa036877b5371d2c5b36afd7bd74fbd63eb3..9579dd735873d5be94e20ea59350701144f67957 100644
--- a/blimp/client/core/contents/blimp_navigation_controller_impl.h
+++ b/blimp/client/core/contents/blimp_navigation_controller_impl.h
@@ -7,9 +7,12 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "blimp/client/core/contents/navigation_feature.h"
 #include "blimp/client/public/contents/blimp_navigation_controller.h"
 #include "url/gurl.h"
 
+class SkBitmap;
+
 namespace blimp {
 namespace client {
 
@@ -21,18 +24,40 @@ class BlimpNavigationControllerDelegate;
 // The BlimpNavigationControllerImpl is the implementation part of
 // BlimpNavigationController. It maintains the back-forward list for a
 // BlimpContentsImpl and manages all navigation within that list.
-class BlimpNavigationControllerImpl : public BlimpNavigationController {
+class BlimpNavigationControllerImpl
+    : public BlimpNavigationController,
+      public NavigationFeature::NavigationFeatureDelegate {
  public:
+  // TODO(shaktisahu): Provide NavigationFeature* in the constructor.

[Kevin M, 2016/07/27 23:35:17]

Obsolete comment?

[shaktisahu, 2016/07/28 19:32:51]

Done.

   explicit BlimpNavigationControllerImpl(

[Kevin M, 2016/07/29 21:05:32]

"explicit" not used for >1 param ctor

[shaktisahu, 2016/07/29 22:57:30]

Done.

-      BlimpNavigationControllerDelegate* delegate);
+      BlimpNavigationControllerDelegate* delegate,
+      NavigationFeature* feature);

[Kevin M, 2016/07/27 23:35:17]

Document the lifetime requirements of |feature| ? Does the need for WeakPtr
still exist?

[shaktisahu, 2016/07/28 19:32:51]

Added comments in the variables section. Regarding the WeakPtr, please clarify?
If you are talking about the test class, please see my comments there.

[Kevin M, 2016/07/29 21:05:32]

I meant, is it possible that the NavigationController might get navigation
events from the UI after the feature is deleted? If no, then disregard.

[shaktisahu, 2016/07/29 22:57:30]

Possibly not. The |feature| lives in BlimpClientSession and is managed on main
thread.

   ~BlimpNavigationControllerImpl() override;
 
   // BlimpNavigationController implementation.

[Kevin M, 2016/07/27 23:35:17]

Overrides go below non-overrides.

[shaktisahu, 2016/07/28 19:32:51]

Since the only other non-override method is a forTesting method, I think it
should be deprioritized and go below the other methods? Let me know what you
think.

[Kevin M, 2016/07/29 21:05:32]

Even though it might be a less-important method, the style guide is pretty
categorical about the declaration order - static methods > class methods >
overrides.

[shaktisahu, 2016/07/29 22:57:30]

Done.

   void LoadURL(const GURL& url) override;
+  void Reload() override;
+  bool CanGoBack() const override;
+  bool CanGoForward() const override;
+  void GoBack() override;
+  void GoForward() override;
+
   const GURL& GetURL() override;
 
+  // For testing only.

[Kevin M, 2016/07/27 23:35:17]

Is this method necessary given that the ctor takes a NavigationFeature*?

[shaktisahu, 2016/07/28 19:32:51]

Yes. It is used in blimp_contents_impl_unittest.cc to set a
FakeNavigationFeature to the navigation controller obtained from the
BlimpContents.

[Kevin M, 2016/07/29 21:05:32]

Right, but the constructor already takes |feature|, so why do we need a testing
setter?

[shaktisahu, 2016/07/29 22:57:30]

Because this object is created inside BlimpContentsImpl constructor which takes
no argument and there is no way to pass this |feature|.

[Kevin M, 2016/08/01 23:41:57]

That's true right now, but BlimpContentsImpl can easily provide an alternate
constructor that takes a NavigationController*. The default no-args ctor can
then delegate to that ctor.

[shaktisahu, 2016/08/02 17:47:52]

Sure, that's a good idea. In that case, we are providing an extra constructor
that originally we didn't want to provide to the users. Not strongly feeling
about it though. Let me know if you want me to make this change. There are lots
of examples following "**ForTesting" pattern. So I decided to use.

+  void SetNavigationFeatureForTesting(NavigationFeature* feature);
+
  private:
-  void NotifyDelegateURLLoaded(const GURL& url);
+  // NavigationFeatureDelegate implementation.
+  void OnUrlChanged(int tab_id, const GURL& url) override;
+  void OnFaviconChanged(int tab_id, const SkBitmap& favicon) override;
+  void OnTitleChanged(int tab_id, const std::string& title) override;
+  void OnLoadingChanged(int tab_id, bool loading) override;
+  void OnPageLoadStatusUpdate(int tab_id, bool completed) override;
+
+  // The |navigation_feature_| is responsible for sending and receiving the
+  // navigation state to the server over the proto channel.
+  NavigationFeature* navigation_feature_;
 
   // The delegate contains functionality required for the navigation controller
   // to function correctly. It is also invoked on relevant state changes of the
@@ -42,9 +67,6 @@ class BlimpNavigationControllerImpl : public BlimpNavigationController {
   // The currently active URL.
   GURL current_url_;
 
-  // TODO(shaktisahu): Remove this after integration with NavigationFeature.
-  base::WeakPtrFactory<BlimpNavigationControllerImpl> weak_ptr_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(BlimpNavigationControllerImpl);
 };