[ios] Use web_state_list in web_contents_mediator.

ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm

Index: ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm
diff --git a/ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm b/ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm
index e1cc86fd5df5b7d34c83fab96f2dbd79c6751fc7..7ccdf3a8f9a088a0338c1041c0ef46728e5e09a8 100644
--- a/ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm
+++ b/ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm
@@ -6,6 +6,9 @@
 
 #include "base/memory/ptr_util.h"
 #import "ios/clean/chrome/browser/ui/web_contents/web_contents_consumer.h"
+#include "ios/shared/chrome/browser/tabs/fake_web_state_list_delegate.h"
+#include "ios/shared/chrome/browser/tabs/web_state_list.h"
+#import "ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h"
 #import "ios/web/public/test/fakes/test_navigation_manager.h"
 #import "ios/web/public/test/fakes/test_web_state.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -16,6 +19,9 @@
 #error "This file requires ARC support."
 #endif
 
+@interface WebContentsMediator (Test)<WebStateListObserving>
+@end
+
 @interface StubContentsConsumer : NSObject<WebContentsConsumer>
 @property(nonatomic, weak) UIView* contentView;
 @end
@@ -48,64 +54,105 @@ class StubNavigationManager : public web::TestNavigationManager {
 };
 
 class WebContentsMediatorTest : public PlatformTest {
- public:
-  WebContentsMediatorTest() {
+ protected:
+  void SetUp() override {
+    SetUpWebStateList();
+    SetUpWebState();
+  }
+
+  void SetUpWebStateList() {
+    auto navigation_manager = base::MakeUnique<StubNavigationManager>();
+    navigation_manager->SetItemCount(0);
+    auto web_state = base::MakeUnique<web::TestWebState>();
+    web_state->SetView([[UIView alloc] init]);
+    web_state->SetNavigationManager(std::move(navigation_manager));
+    web_state_list_ = base::MakeUnique<WebStateList>(
+        &web_state_list_delegate_, WebStateList::WebStateOwned);
+    web_state_list_->InsertWebState(0, web_state.release());
+    web_state_list_->ActivateWebStateAt(0);

[lpromero, 2017/04/03 09:48:36]

Your web_state_list_ doesn’t include web_state_. Is it an issue?

[edchin, 2017/04/04 19:50:12]

This setup was not very readable. I've attempted to make it better. PTAL.

+  }
+
+  void SetUpWebState() {
     auto navigation_manager = base::MakeUnique<StubNavigationManager>();
     navigation_manager_ = navigation_manager.get();
     navigation_manager_->SetItemCount(0);
     web_state_.SetNavigationManager(std::move(navigation_manager));
   }
 
- protected:
+  std::unique_ptr<WebStateList> web_state_list_;
+  FakeWebStateListDelegate web_state_list_delegate_;
   StubNavigationManager* navigation_manager_;
   web::TestWebState web_state_;
+  web::TestWebState old_web_state_;
 };
 
-TEST_F(WebContentsMediatorTest, TestSetWebUsageEnabled) {
+// Test that both the old and new active web states have WebUsageEnabled
+// updated.
+TEST_F(WebContentsMediatorTest, TestWebUsageEnabled) {
   WebContentsMediator* mediator = [[WebContentsMediator alloc] init];
+  mediator.webStateList = web_state_list_.get();

[lpromero, 2017/04/03 09:48:36]

The web state list is not used in the method you test. Is it a requirement to
have a web state list set here?

[edchin, 2017/04/04 19:50:12]

This testing approach was not very readable. I've attempted to make it better.
PTAL.

+  web_state_.SetWebUsageEnabled(false);
+  old_web_state_.SetWebUsageEnabled(true);
+  [mediator webStateList:web_state_list_.get()

[lpromero, 2017/04/03 09:48:36]

Idem, you could pass nullptr here, right?

[rohitrao (ping after 24h), 2017/04/03 23:48:51]

Philosophical question: should we be "faking" WSL observer methods like this, or
should we just call ActivateWebStateAt() and let the real observer machinery do
the work?

This method doesn't need to be in the public API of the mediator, so we could
remove it if we tested the call indirectly.  You could also imagine having a
mediator doing something like DCHECK_EQ(new_web_state,
list->GetActiveWebState()) and then failing because the state of the given WSL
doesn't match what the observer method's arguments say is happening.

[edchin, 2017/04/04 19:50:12]

Both you and marq@ brought up this philosophical point. I agree and have 
changed the approach so as not to "fake" the observer methods. Now it
uses the real observer machinery. 

[edchin, 2017/04/04 19:50:12]

This testing approach was not very readable. I've attempted to make it better.
PTAL. 

+      didChangeActiveWebState:&web_state_
+                  oldWebState:&old_web_state_
+                      atIndex:0
+                   userAction:NO];
+  EXPECT_TRUE(web_state_.IsWebUsageEnabled());
+  EXPECT_FALSE(old_web_state_.IsWebUsageEnabled());
+  mediator.webStateList = nullptr;
+}
 
-  mediator.webState = &web_state_;
-  // Setting the webState should set webUsageEnabled.
-  EXPECT_EQ(true, web_state_.IsWebUsageEnabled());
-  // Expect that with zero navigation items, a url will be loaded.
-  EXPECT_EQ(true, navigation_manager_->GetHasLoadedUrl());
-
-  mediator.webState = nullptr;
-  // The previous webState should now have web usage disabled.
-  EXPECT_EQ(false, web_state_.IsWebUsageEnabled());
+// Test that a url is loaded if the new active web state has zero navigation

[lpromero, 2017/04/03 09:48:36]

URL

[edchin, 2017/04/04 19:50:12]

Done. Found two instances.

+// items.
+TEST_F(WebContentsMediatorTest, TestURLHasLoaded) {
+  WebContentsMediator* mediator = [[WebContentsMediator alloc] init];
+  navigation_manager_->SetItemCount(0);
+  [mediator webStateList:web_state_list_.get()
+      didChangeActiveWebState:&web_state_
+                  oldWebState:&old_web_state_
+                      atIndex:0
+                   userAction:NO];
+  EXPECT_TRUE(navigation_manager_->GetHasLoadedUrl());
 }
 
+// Test that a url is not loaded if the new active web state has some navigation
+// items.
 TEST_F(WebContentsMediatorTest, TestNoLoadURL) {
   WebContentsMediator* mediator = [[WebContentsMediator alloc] init];
-
   navigation_manager_->SetItemCount(2);
-
-  mediator.webState = &web_state_;
-  // Expect that with nonzero navigation items, no url will be loaded.
-  EXPECT_EQ(false, navigation_manager_->GetHasLoadedUrl());
+  [mediator webStateList:web_state_list_.get()
+      didChangeActiveWebState:&web_state_
+                  oldWebState:&old_web_state_
+                      atIndex:0
+                   userAction:NO];
+  EXPECT_FALSE(navigation_manager_->GetHasLoadedUrl());
 }
 
-TEST_F(WebContentsMediatorTest, TestSetWebStateFirst) {
+// Test that the consumer is updated immediately once both consumer and
+// webStateList are set. This test sets webStateList first.
+TEST_F(WebContentsMediatorTest, TestConsumerViewIsSetWebStateListFirst) {
   WebContentsMediator* mediator = [[WebContentsMediator alloc] init];
   StubContentsConsumer* consumer = [[StubContentsConsumer alloc] init];
-
-  mediator.webState = &web_state_;
+  web::WebState* web_state = web_state_list_->GetActiveWebState();
+  mediator.webStateList = web_state_list_.get();
+  EXPECT_NE(web_state->GetView(), consumer.contentView);
   mediator.consumer = consumer;
-
-  // Setting the consumer after the web state should still have the consumer
-  // called.
-  EXPECT_EQ(web_state_.GetView(), consumer.contentView);
+  EXPECT_EQ(web_state->GetView(), consumer.contentView);
+  mediator.webStateList = nullptr;
 }
 
-TEST_F(WebContentsMediatorTest, TestSetConsumerFirst) {
+// Test that the consumer is updated immediately once both consumer and
+// webStateList are set. This test sets consumer first.
+TEST_F(WebContentsMediatorTest, TestConsumerViewIsSetConsumerFirst) {
   WebContentsMediator* mediator = [[WebContentsMediator alloc] init];
   StubContentsConsumer* consumer = [[StubContentsConsumer alloc] init];
-
+  web::WebState* web_state = web_state_list_->GetActiveWebState();
   mediator.consumer = consumer;
-  mediator.webState = &web_state_;
-
-  // Setting the web_state after the consumer should trigger a call to the
-  // consumer.
-  EXPECT_EQ(web_state_.GetView(), consumer.contentView);
-}
+  EXPECT_NE(web_state->GetView(), consumer.contentView);
+  mediator.webStateList = web_state_list_.get();
+  EXPECT_EQ(web_state->GetView(), consumer.contentView);
+  mediator.webStateList = nullptr;
 }
+
+}  // namespace