Tied up BlimpNavigationController to NavigationFeature

blimp/client/core/contents/fake_navigation_feature.h

Index: blimp/client/core/contents/fake_navigation_feature.h
diff --git a/blimp/client/core/contents/fake_navigation_feature.h b/blimp/client/core/contents/fake_navigation_feature.h
new file mode 100644
index 0000000000000000000000000000000000000000..146497b1e45aa948b5d66313a0ae44c63cbf9eda
--- /dev/null
+++ b/blimp/client/core/contents/fake_navigation_feature.h
@@ -0,0 +1,37 @@
+// 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_FAKE_NAVIGATION_FEATURE_H_
+#define BLIMP_CLIENT_CORE_CONTENTS_FAKE_NAVIGATION_FEATURE_H_
+
+#include <string>
+
+#include "base/memory/weak_ptr.h"
+#include "blimp/client/core/contents/navigation_feature.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace blimp {
+namespace client {
+
+class FakeNavigationFeature : public NavigationFeature {

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

s/Fake/Mock/

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

The NavigateToUrlText method emulates that URL has been loaded and notifies the
delegate. So I think calling Fake is more appropriate?

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

We don't need to emulate that behavior, because it's not really providing better
coverage of product code. We just want to ensure that NavigateToUrlText is
called. Invoking the delegate with a fake method is only really exercising
GetDelegate(), which you can test through more direct means.

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

hmm.. Emulating this method provides a nice way to verify the call path all the
way from BlimpContents, BlimpNavigationController,
BlimpNavigationControllerDelegate, BlimpContentsObserver. For instance, we could
verify if we are loading the correct URL and check the observers etc. In future,
we might also need to verify end-to-end from Java.

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

Yeah, but right now a lot of the functionality of this Fake that is being tested
is... test code. What's the point of testing that?

My gut feeling is that we'll probably want to avoid the use of mocks in
end-to-end testing anyhow, since we'll want as little "fakeness" as possible?

We can demote FindDelegate() to class-private afterward.

+ public:
+  FakeNavigationFeature();
+  ~FakeNavigationFeature();
+
+  MOCK_METHOD1(Reload, void(int tab_id));
+  MOCK_METHOD1(GoBack, void(int tab_id));
+  MOCK_METHOD1(GoForward, void(int tab_id));
+
+  void NavigateToUrlText(int tab_id, const std::string& url_text) override;
+
+ private:
+  void NotifyDelegateURLLoaded(int tab_id, const std::string& url_text);
+
+  base::WeakPtrFactory<FakeNavigationFeature> weak_ptr_factory_;

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

Is this ever used? I don't see a test case where FakeNavigationFeature is
deleted.

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

DISALLOW_COPY_AND_ASSIGNED

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

Done.

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

Yes, this is used in the C++ file to post a callback.

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

WeakPtr isn't useful though. Just use base::Unretained().

You might want to read weak_ptr.h to get an understanding of why WeakPtr is
useful, since this has come up elsewhere in this code review.

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

Done. Thanks!

+};
+
+}  // namespace client
+}  // namespace blimp
+
+#endif  // BLIMP_CLIENT_CORE_CONTENTS_FAKE_NAVIGATION_FEATURE_H_