[ios clean] ToolsMenu Mediator observes Webstate.

ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm

Index: ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm
diff --git a/ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm b/ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm
index 0d94cd5dc768fef62c39b811dcd1ec4ae6766b3b..87dfc2cec82a1b72bcf00440b9ff5581119a93a9 100644
--- a/ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm
+++ b/ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm
@@ -4,10 +4,14 @@
 
 #import "ios/clean/chrome/browser/ui/tools/tools_mediator.h"
 
+#include "base/memory/ptr_util.h"
 #import "ios/clean/chrome/browser/ui/tools/tools_consumer.h"
 #import "ios/clean/chrome/browser/ui/tools/tools_mediator_private.h"
 #import "ios/clean/chrome/browser/ui/tools/tools_menu_item.h"
+#import "ios/shared/chrome/browser/ui/toolbar/toolbar_test_util.h"
 #import "ios/shared/chrome/browser/ui/tools_menu/tools_menu_configuration.h"
+#import "ios/web/public/test/fakes/test_web_state.h"
+#import "ios/web/public/web_state/web_state_observer_bridge.h"
 #include "testing/gtest_mac.h"
 #include "testing/platform_test.h"
 #import "third_party/ocmock/OCMock/OCMock.h"
@@ -17,87 +21,125 @@
 #error "This file requires ARC support."
 #endif
 
+@interface TestToolsMediator : ToolsMediator<CRWWebStateObserver>

[marq (ping after 24h), 2017/05/30 09:24:54]

This doesn't seem to be necessary.

[sczs, 2017/05/31 03:05:49]

You're right. Done.

+@end
+
+@implementation TestToolsMediator
+@end
+
 namespace {
 
 class ToolsMediatorTest : public PlatformTest {
+ public:
+  ToolsMediatorTest() {
+    std::unique_ptr<ToolbarTestNavigationManager> navigation_manager =

[marq (ping after 24h), 2017/05/30 09:24:55]

Use a C++ initializer list here?

[sczs, 2017/05/31 03:05:50]

Done, though I'm not sure if this is what you meant.

[marq (ping after 24h), 2017/05/31 10:40:32]

It is, but I meant for all of the member variables.

[sczs, 2017/05/31 23:19:26]

Done.

[sdefresne, 2017/07/02 19:01:54]

This was incorrect, those were not member variables but local variables.

+        base::MakeUnique<ToolbarTestNavigationManager>();
+    navigation_manager_ = navigation_manager.get();
+    test_web_state_.SetNavigationManager(std::move(navigation_manager));
+    configuration_ = OCMClassMock([ToolsMenuConfiguration class]);

[marq (ping after 24h), 2017/05/30 09:24:55]

I'd prefer not to use a mock for ToolsMenuConfiguration, since it's really just
a read-only data object. We don't need to verify that any calls are made against
it.

[sczs, 2017/05/31 03:05:49]

Done.

+    consumer_ = OCMProtocolMock(@protocol(ToolsConsumer));
+  }
+
  protected:
-  ToolsMediator* mediator_;
+  TestToolsMediator* mediator_;
+  id consumer_;
+  id configuration_;
+  ToolbarTestWebState test_web_state_;
+  ToolbarTestNavigationManager* navigation_manager_;
 };
 
 TEST_F(ToolsMediatorTest, TestShowOverFlowControls) {
-  id consumer = OCMProtocolMock(@protocol(ToolsConsumer));
-  id configuration = OCMClassMock([ToolsMenuConfiguration class]);
-  OCMStub([configuration isInTabSwitcher]).andReturn(YES);
-
-  mediator_ = [[ToolsMediator alloc] initWithConsumer:consumer
-                                     andConfiguration:configuration];
+  OCMStub([configuration_ isInTabSwitcher]).andReturn(YES);
+  mediator_ = [[TestToolsMediator alloc] initWithConsumer:consumer_
+                                         andConfiguration:configuration_];
 
-  [[consumer verify] setToolsMenuItems:[OCMArg any]];
-  [[consumer verify] setDisplayOverflowControls:NO];
+  [[consumer_ verify] setToolsMenuItems:[OCMArg any]];
+  [[consumer_ verify] setDisplayOverflowControls:NO];
 }
 
 TEST_F(ToolsMediatorTest, TestHideOverFlowControls) {
-  id consumer = OCMProtocolMock(@protocol(ToolsConsumer));
-  id configuration = OCMClassMock([ToolsMenuConfiguration class]);
-  OCMStub([configuration isInTabSwitcher]).andReturn(NO);
+  OCMStub([configuration_ isInTabSwitcher]).andReturn(NO);
 
-  mediator_ = [[ToolsMediator alloc] initWithConsumer:consumer
-                                     andConfiguration:configuration];
+  mediator_ = [[TestToolsMediator alloc] initWithConsumer:consumer_

[marq (ping after 24h), 2017/05/30 09:24:55]

Any reason to not initialize mediator_ in the test initializer?

[sczs, 2017/05/31 03:05:51]

Since the configuration is not exposed as a property it can only be set when
initializing the mediator.

[marq (ping after 24h), 2017/05/31 10:40:32]

And calling the initializer triggers updates to the consumer. No change for this
CL, but I think I prefer the pattern where we set the consumer through a
property. There's some additional cost in managing state, but it means that
initialization and action are separated.

[sczs, 2017/05/31 23:19:26]

Acknowledged.

+                                         andConfiguration:configuration_];
 
-  [[consumer verify] setToolsMenuItems:[OCMArg any]];
-  [[consumer verify] setDisplayOverflowControls:YES];
+  [[consumer_ verify] setToolsMenuItems:[OCMArg any]];
+  [[consumer_ verify] setDisplayOverflowControls:YES];
 }
 
 TEST_F(ToolsMediatorTest, TestMenuItemsForNonTabSwitcherNonIncognito) {
-  id consumer = OCMProtocolMock(@protocol(ToolsConsumer));
-  id configuration = OCMClassMock([ToolsMenuConfiguration class]);
-  OCMStub([configuration isInTabSwitcher]).andReturn(NO);
-  OCMStub([configuration isInIncognito]).andReturn(NO);
+  OCMStub([configuration_ isInTabSwitcher]).andReturn(NO);
+  OCMStub([configuration_ isInIncognito]).andReturn(NO);
 
-  mediator_ = [[ToolsMediator alloc] initWithConsumer:consumer
-                                     andConfiguration:configuration];
+  mediator_ = [[TestToolsMediator alloc] initWithConsumer:consumer_
+                                         andConfiguration:configuration_];
 
   EXPECT_EQ(7ul, [mediator_.menuItemsArray count]);
 }
 
 TEST_F(ToolsMediatorTest, TestMenuItemsForNonTabSwitcherIncognito) {
-  id consumer = OCMProtocolMock(@protocol(ToolsConsumer));
-  id configuration = OCMClassMock([ToolsMenuConfiguration class]);
-  OCMStub([configuration isInTabSwitcher]).andReturn(NO);
-  OCMStub([configuration isInIncognito]).andReturn(YES);
+  OCMStub([configuration_ isInTabSwitcher]).andReturn(NO);
+  OCMStub([configuration_ isInIncognito]).andReturn(YES);
 
-  mediator_ = [[ToolsMediator alloc] initWithConsumer:consumer
-                                     andConfiguration:configuration];
+  mediator_ = [[TestToolsMediator alloc] initWithConsumer:consumer_
+                                         andConfiguration:configuration_];
 
   EXPECT_EQ(7ul, [mediator_.menuItemsArray count]);
 }
 
-TEST_F(ToolsMediatorTest, TestMenuItemsForTabSwitcherNonIncognito) {
-  id consumer = OCMProtocolMock(@protocol(ToolsConsumer));
-  id configuration = OCMClassMock([ToolsMenuConfiguration class]);
-  OCMStub([configuration isInTabSwitcher]).andReturn(YES);
-  OCMStub([configuration isInIncognito]).andReturn(YES);
+TEST_F(ToolsMediatorTest, TestMenuItemsForTabSwitcherIncognito) {
+  OCMStub([configuration_ isInTabSwitcher]).andReturn(YES);
+  OCMStub([configuration_ isInIncognito]).andReturn(YES);
 
-  mediator_ = [[ToolsMediator alloc] initWithConsumer:consumer
-                                     andConfiguration:configuration];
+  mediator_ = [[TestToolsMediator alloc] initWithConsumer:consumer_
+                                         andConfiguration:configuration_];
 
   ToolsMenuItem* closeAllTabsItem = mediator_.menuItemsArray[2];
   EXPECT_NSEQ(@"Close All Incognito Tabs", closeAllTabsItem.title);
   EXPECT_EQ(5ul, [mediator_.menuItemsArray count]);
 }
 
-TEST_F(ToolsMediatorTest, TestMenuItemsForTabSwitcherIncognito) {
-  id consumer = OCMProtocolMock(@protocol(ToolsConsumer));
-  id configuration = OCMClassMock([ToolsMenuConfiguration class]);
-  OCMStub([configuration isInTabSwitcher]).andReturn(YES);
-  OCMStub([configuration isInIncognito]).andReturn(NO);
+TEST_F(ToolsMediatorTest, TestMenuItemsForTabSwitcherNonIncognito) {
+  OCMStub([configuration_ isInTabSwitcher]).andReturn(YES);
+  OCMStub([configuration_ isInIncognito]).andReturn(NO);
 
-  mediator_ = [[ToolsMediator alloc] initWithConsumer:consumer
-                                     andConfiguration:configuration];
+  mediator_ = [[TestToolsMediator alloc] initWithConsumer:consumer_
+                                         andConfiguration:configuration_];
 
   ToolsMenuItem* closeAllTabsItem = mediator_.menuItemsArray[2];
   EXPECT_NSEQ(@"Close All Tabs", closeAllTabsItem.title);
   EXPECT_EQ(5ul, [mediator_.menuItemsArray count]);
 }
 
+TEST_F(ToolsMediatorTest, TestDontUpdateConsumerLoadingState) {
+  OCMStub([configuration_ isInTabSwitcher]).andReturn(NO);

[marq (ping after 24h), 2017/05/30 09:24:55]

Should you also test these cases when -isInTabSwitcher is YES?

[sczs, 2017/05/31 03:05:53]

You're right, it should always reject! Since no Webstate is supposed to be set
then. I will also add this Check on the Mediator code.
Done.

+  mediator_ = [[TestToolsMediator alloc] initWithConsumer:consumer_
+                                         andConfiguration:configuration_];
+
+  [[consumer_ reject] setIsLoading:YES];
+  [[consumer_ reject] setIsLoading:NO];
+}
+
+TEST_F(ToolsMediatorTest, TestUpdateConsumerLoadingState) {
+  OCMStub([configuration_ isInTabSwitcher]).andReturn(NO);
+  mediator_ = [[TestToolsMediator alloc] initWithConsumer:consumer_
+                                         andConfiguration:configuration_];
+  mediator_.webState = &test_web_state_;
+  test_web_state_.SetLoading(false);
+  [[consumer_ verify] setIsLoading:NO];
+  test_web_state_.SetLoading(true);
+  [[consumer_ verify] setIsLoading:YES];
+}
+
+TEST_F(ToolsMediatorTest, TestUpdateConsumerLoadingStateInverse) {
+  OCMStub([configuration_ isInTabSwitcher]).andReturn(NO);
+  mediator_ = [[TestToolsMediator alloc] initWithConsumer:consumer_
+                                         andConfiguration:configuration_];
+  mediator_.webState = &test_web_state_;
+  test_web_state_.SetLoading(true);
+  [[consumer_ verify] setIsLoading:YES];
+  test_web_state_.SetLoading(false);
+  [[consumer_ verify] setIsLoading:NO];
+}
+
 }  // namespace