Tied up BlimpNavigationController to NavigationFeature

blimp/client/core/contents/blimp_contents_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_CONTENTS_IMPL_H_
 #define BLIMP_CLIENT_CORE_CONTENTS_BLIMP_CONTENTS_IMPL_H_
 
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "blimp/client/core/contents/blimp_navigation_controller_delegate.h"
@@ skipping 22 matching lines @@
  public:
   BlimpContentsImpl();
   ~BlimpContentsImpl() override;
 
 #if defined(OS_ANDROID)
   base::android::ScopedJavaLocalRef<jobject> GetJavaBlimpContentsImpl();
   BlimpContentsImplAndroid* GetBlimpContentsImplAndroid();
 #endif  // defined(OS_ANDROID)
 
   // BlimpContents implementation.
   BlimpNavigationControllerImpl& GetNavigationController() override;

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

This should be a pointer, not a reference - reference return types are almost
always const.

[David Trainor- moved to gerrit, 2016/07/28 16:43:24]

We were following the pattern used by WebContents and WebContentsImpl in
Chromium.  Although after looking, it seems like the IOS versions from WebState
return a pointer and say 'cannot be null.'

I was under the impression that a reference is a clean way to state that this
object can never be null, but I'm fine with us changing it to a pointer if you
feel strongly.  I'm not sure if the Chromium style guide prefers one way over
another, but Shakti should look!

[nyquist, 2016/07/28 19:23:06]

Yeah, when I added this I was following the exact same pattern as for the public
content API with the added bonus that it clearly shows it's never null.
Otherwise you have to look at the base class documentation in the header to see
if it can be null or not.

I don't feel strongly, but I didn't quite get what the benefit of what returning
a pointer here would give us.

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

The convention that Wez and I have become very accustomed to is that pointers
imply immutability and a potentially long lifetime (you can depend on the
pointer living as long as the comment implies) whereas references imply
constness and a potentially shorter lifetime.

ISTM that the style guide has implicitly declared a preference for pointered
return values by specifying that mutable parameters must be passed in via
pointers. Pointer parameters are also susceptible to being passed
surprise-NULLs, but we accept that risk--mitigated with DCHECK--in exchange for
the readability benefit of strongly associating mutability with pointers.

   void AddObserver(BlimpContentsObserver* observer) override;
   void RemoveObserver(BlimpContentsObserver* observer) override;
 
   // BlimpNavigationControllerDelegate implementation.
-  void NotifyURLLoaded(const GURL& url) override;
+  void OnNavigationStateChanged() override;
 
  private:
   // Handles the back/forward list and loading URLs.
   BlimpNavigationControllerImpl navigation_controller_;
 
   // A list of all the observers of this BlimpContentsImpl.
   base::ObserverList<BlimpContentsObserver> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(BlimpContentsImpl);
 };
 
 }  // namespace client
 }  // namespace blimp
 
 #endif  // BLIMP_CLIENT_CORE_CONTENTS_BLIMP_CONTENTS_IMPL_H_