Tied up BlimpNavigationController to NavigationFeature

blimp/client/core/contents/blimp_navigation_controller_impl.h

 // Copyright 2016 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.
 
 #ifndef BLIMP_CLIENT_CORE_CONTENTS_BLIMP_NAVIGATION_CONTROLLER_IMPL_H_
 #define BLIMP_CLIENT_CORE_CONTENTS_BLIMP_NAVIGATION_CONTROLLER_IMPL_H_
 
 #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 {
 
 class BlimpContentsObserver;
 class BlimpNavigationControllerDelegate;
 
 // BlimpContentsImpl is the implementation of the core class in

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

BlimpContentsImpl isn't defined here; is this comment applicable?

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

Removed the comment since it doesn't convey more than the super class comments.

 // //blimp/client/core, the BlimpContents.
 // 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
   // navigation controller.
   BlimpNavigationControllerDelegate* delegate_;
 
   // 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);
 };
 
 }  // namespace client
 }  // namespace blimp
 
 #endif  // BLIMP_CLIENT_CORE_CONTENTS_BLIMP_NAVIGATION_CONTROLLER_IMPL_H_