[ios clean] Consumer interface for WebContents

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

 // Copyright 2016 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.
 
 // ======                        New Architecture                         =====
 // =         This code is only used in the new iOS Chrome architecture.       =
 // ============================================================================
 
 #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;
-
-- (instancetype)initWithWebState:(web::WebState*)webState {
-  if ((self = [super initWithNibName:nil bundle:nil])) {
-    DCHECK(webState);
-    _webState = webState;
-  }
-  return self;
-}
+@synthesize contentView = _contentView;
 
 - (void)viewDidLoad {
   self.view.backgroundColor = [UIColor colorWithWhite:0.75 alpha:1.0];
 
-  UIView* webContentsView = self.webState->GetView();
-  webContentsView.translatesAutoresizingMaskIntoConstraints = NO;
-  [self.view addSubview:webContentsView];
-  [NSLayoutConstraint activateConstraints:@[
-    [webContentsView.topAnchor constraintEqualToAnchor:self.view.topAnchor],
-    [webContentsView.bottomAnchor
-        constraintEqualToAnchor:self.view.bottomAnchor],
-    [webContentsView.leadingAnchor
-        constraintEqualToAnchor:self.view.leadingAnchor],
-    [webContentsView.trailingAnchor
-        constraintEqualToAnchor:self.view.trailingAnchor],
-  ]];
+  if (!self.contentView) {
+    self.contentView = [[UIView alloc] init];
+  }
 
-  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);
+  [self updateContentView];
+}
+
+#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.

   }
 }
 
+#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:@[
+    [self.contentView.topAnchor constraintEqualToAnchor:self.view.topAnchor],
+    [self.contentView.bottomAnchor
+        constraintEqualToAnchor:self.view.bottomAnchor],
+    [self.contentView.leadingAnchor
+        constraintEqualToAnchor:self.view.leadingAnchor],
+    [self.contentView.trailingAnchor
+        constraintEqualToAnchor:self.view.trailingAnchor],
+  ]];
+}
+
 @end