Refactor creation of SadTabView into a tab helper object

ios/chrome/browser/web/tab_helper_delegate.h

Index: ios/chrome/browser/web/tab_helper_delegate.h
diff --git a/ios/chrome/browser/web/tab_helper_delegate.h b/ios/chrome/browser/web/tab_helper_delegate.h
new file mode 100644
index 0000000000000000000000000000000000000000..81f8715b227e6dee14f0b16fde65027e0331fd85
--- /dev/null
+++ b/ios/chrome/browser/web/tab_helper_delegate.h
@@ -0,0 +1,27 @@
+// Copyright 2017 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 tab_helper_delegate_h
+#define tab_helper_delegate_h
+
+#import <Foundation/Foundation.h>
+
+NS_ASSUME_NONNULL_BEGIN
+
+// Tab Helper IDs
+
+extern NSString* const SadTabTabHelperID;
+
+@protocol TabHelperDelegate<NSObject>

[kkhorimoto, 2017/04/10 22:31:10]

Also, for C++ objects, we should use C++ delegates (see WebStateDelegate for an
example).

[kkhorimoto, 2017/04/10 22:31:10]

Can you add comments elaborating the purpose of this delegate?  I'm not sure we
need a generalized delegate for all TabHelpers (like the naming here implies). 
From the implementation, it looks like this is specific to not creating
SadTabViews for tabs that are not currently active;  maybe we should create a
delegate class specifically for SadTabHelper?  "TabHelper" is a general pattern
we use, but it isn't an actual class; I'm hesitant about creating a
TabHelperDelegate since there isn't a specific class whose functionality we're
delegating.

[PL, 2017/04/11 01:35:22]

Done, thanks! Changing this as discussed offline: Continue using a delegate but
abandon an attempt to make a generic tab helper delegate pattern here.

+
+// Returns whether the helper should be active and providing 'help' to the
+// delegate.
+// tabHelperID is a mechanism for identifying the specific tabHelper identity
+- (BOOL)tabHelperShouldBeActive:(NSString*)tabHelperID;

[kkhorimoto, 2017/04/10 22:44:14]

Another reason I'm not a big fan of this is that "Active" doesn't really have a
clear definition for TabHelpers in general.  Even for SadTabHelper, it's a
little ambiguous because the helper is still actively observing the WSO
callbacks, but only updates the view hierarchy if this returns YES.  I think a
better solution would be a delegate specific to SadTabHelper with something like
ShouldShowSadTabForWebState().  Another option (if we wanted to avoid adding C++
delegate classes, which are kinda clunky) would be to add additional
functionality to SadTabHelper.  You could probably implement the same behavior
pretty easily by using WebStateListObserver to track whether the SadTabHelper's
WebState is currently active, then decide whether to display the SadTabView
using observed state.

[Eugene But (OOO till 7-30), 2017/04/10 22:56:28]

Do you want to rename this delegate to SadTabTabHelperDelegate and rename this
method to?:
- (BOOL)isTabCurrentlyDisplayedForTabHelper:(SadTabTabHelper*)tabHelper;

The reasons are:
1.) delegate method should take caller as a first argument
2.) It is hard to understand how |tabHelperShouldBeActive:| should be
implemented.
3.) You don't need TabHelperID with proposed approach

[PL, 2017/04/11 01:35:22]

Done, thanks! Changing this as discussed offline: Continue using a delegate but
abandon an attempt to make a generic tab helper delegate pattern here.

+
+@end
+
+NS_ASSUME_NONNULL_END
+
+#endif /* tab_helper_delegate_h */