[ios clean] Toolbar displays total number of tabs.

ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.mm

Index: ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.mm
diff --git a/ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.mm b/ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.mm
index 540da63e4d7c0bc3d2883347d517199e26dc6959..fe1b6d45bb3b10a44a88707677e7cd89d272001c 100644
--- a/ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.mm
+++ b/ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.mm
@@ -5,7 +5,10 @@
 #import "ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.h"
 
 #include "base/memory/ptr_util.h"
+#include "base/scoped_observer.h"
 #include "base/strings/sys_string_conversions.h"
+#import "ios/chrome/browser/web_state_list/web_state_list.h"
+#import "ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h"
 #import "ios/clean/chrome/browser/ui/toolbar/toolbar_consumer.h"
 #import "ios/web/public/navigation_manager.h"
 #include "ios/web/public/web_state/web_state.h"
@@ -15,19 +18,29 @@
 #error "This file requires ARC support."
 #endif
 
-@interface ToolbarMediator ()<CRWWebStateObserver>
+@interface ToolbarMediator ()<CRWWebStateObserver, WebStateListObserving>
 @end
 
 @implementation ToolbarMediator {
   std::unique_ptr<web::WebStateObserverBridge> _webStateObserver;
+  std::unique_ptr<WebStateListObserverBridge> _webStateListObserver;
+  std::unique_ptr<ScopedObserver<WebStateList, WebStateListObserverBridge>>
+      _scopedWebStateListObserver;
 }
 
 @synthesize consumer = _consumer;
 @synthesize webState = _webState;
+@synthesize webStateList = _webStateList;
 
 - (void)dealloc {
+  [self disconnect];
+}
+
+#pragma mark - Public
+
+- (void)disconnect {
+  self.webStateList = nullptr;
   _webStateObserver.reset();
-  _webState = nullptr;
 }
 
 #pragma mark - CRWWebStateObserver
@@ -52,6 +65,20 @@
   [self.consumer setLoadingProgress:progress];
 }
 
+#pragma mark - WebStateListObserver
+
+- (void)webStateList:(WebStateList*)webStateList
+    didInsertWebState:(web::WebState*)webState
+              atIndex:(int)index {
+  [self.consumer setNumberOfTabs:_webStateList->count()];
+}
+
+- (void)webStateList:(WebStateList*)webStateList
+    didDetachWebState:(web::WebState*)webState
+              atIndex:(int)index {
+  [self.consumer setNumberOfTabs:_webStateList->count()];
+}
+
 #pragma mark - Setters
 
 - (void)setWebState:(web::WebState*)webState {
@@ -68,6 +95,23 @@
   if (self.webState) {
     [self updateConsumer];
   }
+  if (self.webStateList) {
+    [self.consumer setNumberOfTabs:_webStateList->count()];
+  }
+}
+
+- (void)setWebStateList:(WebStateList*)webStateList {
+  _webStateList = webStateList;
+  _webStateListObserver = base::MakeUnique<WebStateListObserverBridge>(self);
+  _scopedWebStateListObserver = base::MakeUnique<
+      ScopedObserver<WebStateList, WebStateListObserverBridge>>(
+      _webStateListObserver.get());
+  if (_webStateList) {
+    _scopedWebStateListObserver->Add(_webStateList);
+    if (self.consumer) {
+      [self.consumer setNumberOfTabs:_webStateList->count()];

[edchin, 2017/05/27 16:25:06]

If the webStateList is set to nil, wouldn't you want to set the tab count to
zero, instead of leaving it at some existing number? Consider
DCHECK(webStateList). I don't think we have this pattern yet, but I feel it is
better than convoluted logic that checks for nullability. 

[marq (ping after 24h), 2017/05/29 11:04:36]

Yes, I think it's correct to declare that setting a nullptr WSL here is a
programming error, and enforce that with a DCHECK at the start of the method. 

(Consider: if the WSL is empty, what toolbar is this mediator talking to?)

[sczs, 2017/05/30 00:18:21]

The main problem is that we are explicitly calling self.webStateList = nullptr
on disconnect. Without this we get the following observer_list failed check
during testing:

FATAL:observer_list.h(338)] Check failed: ObserverListBase<ObserverType>::size()
== 0U (1 vs. 0)
(If I try to set the backing variable to nullptr I still get this same check
fail)

I think Ed and Rohit got to the bottom of this, and that's why TabGridMediator
also checks for a nil webStateList

The second issue is that TabCoordinator unit tests are currently not
initializing a WebStateList, so they would also trigger a DCHECK(webStateList).
We also need to update those. 

For these reasons I would recommend leaving this as is and we can revisit this
issue for both this mediator and TabGridMediator on a separate CL. I will add a
TODO for now.

If you don't agree with this approach please let me know, I'm open to any ideas
:) 

+    }
+  }
 }
 
 #pragma mark - Helper methods