[ios clean] Consumer interface for WebContents

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

Index: ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm
diff --git a/ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm b/ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm
index 1b9c41dcf22333f16988936e56360275a7e75abb..834f5211d76babfc2827ab7c77e44cb8ae1cc00a 100644
--- a/ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm
+++ b/ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm
@@ -8,49 +8,55 @@
 
 #import "ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.h"
 
-#import "ios/web/public/navigation_manager.h"
-#include "ios/web/public/web_state/web_state.h"
-#include "ui/base/page_transition_types.h"
-#include "url/gurl.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
 #endif
 
+@interface WebContentsViewController ()
+@property(nonatomic, strong) UIView* contentView;
+@end
+
 @implementation WebContentsViewController
 
-@synthesize webState = _webState;
+@synthesize contentView = _contentView;
 
-- (instancetype)initWithWebState:(web::WebState*)webState {
-  if ((self = [super initWithNibName:nil bundle:nil])) {
-    DCHECK(webState);
-    _webState = webState;
+- (void)viewDidLoad {
+  self.view.backgroundColor = [UIColor colorWithWhite:0.75 alpha:1.0];
+
+  if (!self.contentView) {
+    self.contentView = [[UIView alloc] init];
   }
-  return self;
+
+  [self updateContentView];
 }
 
-- (void)viewDidLoad {
-  self.view.backgroundColor = [UIColor colorWithWhite:0.75 alpha:1.0];
+#pragma mark - WebContentsConsumer
+
+- (void)contentViewDidChange:(UIView*)contentView {
+  if (contentView == self.contentView)
+    return;
+  [self.contentView removeFromSuperview];
+  self.contentView = contentView;
+  if (self.viewIfLoaded) {
+    [self updateContentView];

[edchin, 2017/02/02 22:17:52]

It's better balanced if you [self.view addSubview:self.contentView] here, since
you remove above. 

[marq (ping after 24h), 2017/02/03 09:48:10]

I'll spell out why I did it this way in comments. This has to handle two edge
cases:

- -contentViewDidChange: is called for the second or subsequent time, meaning
the previous content view needs to be removed from the view hierarchy.
- -contentViewDidChange: is called before the view controller's view has been
created, meaning we want to wait until the view is loaded to add it to the view
hierarchy.

[edchin, 2017/02/03 15:33:38]

My suggestion is to simply pull a line out from within a method. Removing the
previous content view
should stay the same. And the line will still be after the check that the view
is loaded.

[marq (ping after 24h), 2017/02/07 14:33:11]

We still need to add the subview in -updateContentView to handle the case where
-viewDidLoad is called after -contentViewDidChange, which means either having an
addSubview: that's a no-op some of the time. I'm going to leave it as-is for now
but keep an eye on complexity creep as this gets more involved, to make sure
things stay clear.

+  }
+}
 
-  UIView* webContentsView = self.webState->GetView();
-  webContentsView.translatesAutoresizingMaskIntoConstraints = NO;
-  [self.view addSubview:webContentsView];
+#pragma mark - Private methods
+
+- (void)updateContentView {
+  self.contentView.translatesAutoresizingMaskIntoConstraints = NO;

[edchin, 2017/02/02 22:17:53]

Using autoresizing masks would be clearer and more concise for situations like
this where the constraints are simply attaching to the parent edges.

[marq (ping after 24h), 2017/02/03 09:48:10]

I agree, I've been using constraints for everything as a matter of principle but
since UIKit's just going to generate constraints from the mask anyway, it's
mostly not worth it.

(The one case where I would use constraints in a case like this is when I
expected to be adding more subviews that interact with those constraints later).

+  [self.view addSubview:self.contentView];
   [NSLayoutConstraint activateConstraints:@[
-    [webContentsView.topAnchor constraintEqualToAnchor:self.view.topAnchor],
-    [webContentsView.bottomAnchor
+    [self.contentView.topAnchor constraintEqualToAnchor:self.view.topAnchor],
+    [self.contentView.bottomAnchor
         constraintEqualToAnchor:self.view.bottomAnchor],
-    [webContentsView.leadingAnchor
+    [self.contentView.leadingAnchor
         constraintEqualToAnchor:self.view.leadingAnchor],
-    [webContentsView.trailingAnchor
+    [self.contentView.trailingAnchor
         constraintEqualToAnchor:self.view.trailingAnchor],
   ]];
-
-  if (!self.webState->GetNavigationManager()->GetItemCount()) {
-    web::NavigationManager::WebLoadParams params(
-        GURL("https://dev.chromium.org/"));
-    params.transition_type = ui::PAGE_TRANSITION_TYPED;
-    _webState->GetNavigationManager()->LoadURLWithParams(params);
-  }
 }
 
 @end