[ios clean] Consumer interface for WebContents

This CL abstracts out WebContentsViewController's WebState dependency by creating a
mediator and a consumer protocol.  

WebContentsConsumer is a "push" interface for a provider object (here the mediator) to 
inform a consumer (here the view controller) about changes it should reflect in its UI.

Review-Url: https://codereview.chromium.org/2675763002
Cr-Commit-Position: refs/heads/master@{#448629}
Committed: https://chromium.googlesource.com/chromium/src/+/92ee2d62dea54502e0f2ab2b0874620c4bed8c69

[marq (ping after 24h), 2017-02-02 18:01:36]

Separate files.

[marq (ping after 24h), 2017-02-02 18:09:45]

Description was changed from

==========
gewerken


formatted.

BUG=
==========

to

==========
This CL abstracts out WebContentsViewController's WebState dependency by
creating a
mediator and a consumer protocol.  

WebContentsConsumer is a "push" interface for a provider object (here the
mediator) to 
inform a consumer (here the view controller) about changes it should reflect in
its UI.
==========

[marq (ping after 24h), 2017-02-02 18:09:45]

marq@chromium.org changed reviewers:
+ edchin@chromium.org

[marq (ping after 24h), 2017-02-02 18:10:19]

Ed -- First pass at a "push" view controller data source as we discussed.

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

Overall, I love this approach. 

I have two mild concerns:

1) Is it necessary to introduce the new terminology: "Consumer"? Why does the
idiomatic term "Delegate" not fit the needs here?

2) Does the UI layer need to be involved in this "Consumer" contract? Consider
an alternative setup:
Perhaps the coordinator could register itself as the consumer. And the
ViewController can expose custom methods that update the relevant UI elements.
For example, a favicon setter can be exposed by the viewController without
knowing that the data comes from webContents.

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm (right):

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:26: if
(!self.webState)
Can we be consistent in the use of property vs. instance variable accessors?

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm
(right):

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm:42:
[self updateContentView];
It's better balanced if you [self.view addSubview:self.contentView] here, since
you remove above.

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm:49:
self.contentView.translatesAutoresizingMaskIntoConstraints = NO;
Using autoresizing masks would be clearer and more concise for situations like
this where the constraints are simply attaching to the parent edges.

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm (right):

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm:39: _webState =
webState;
Consistent use of property accessor.

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm:57: // better way in
the future.
It is not clear to me why we should set webUsageEnabled to false. Can you
explain this comment?

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

> 1) Is it necessary to introduce the new terminology: "Consumer"? Why does the
> idiomatic term "Delegate" not fit the needs here?

The naming is the thing I'm least decided on, and I'm aware of (and concerned
about)
terminological explosion. I'll discuss this more, but in brief I don't want to
call this
'Delegate' because (a) this isn't a delegation relationship, and (b) there are
UIKit 
conventions about protocols named 'Delegate' that this doesn't (and shouldn't)
follow.

> 2) Does the UI layer need to be involved in this "Consumer" contract? Consider
an 
> alternative setup:
> Perhaps the coordinator could register itself as the consumer. And the 
> ViewController can expose custom methods that update the relevant UI elements.
> For example, a favicon setter can be exposed by the viewController without
> knowing that the data comes from webContents.

My gut feeling is that that couples the view controller too tightly to the
coordinator. 

Here are two litmus tests:

- Writing a test for the coordinator is harder, because it has to mock the view
controller's
UI update methods. 
- Suppose the coordinator wants to (based on an experimental flag) show a VC
with a different
UI. Then the logic for consumer->UI mapping for both cases has to be in the
coordinator.

The pattern in this CL, however, means that
- The coordinator's responsibilities are limited to creating the VC and mediator
objects and 
connecting them to each other, which is easy to right change-resilient tests
for.
- The coordinator and the mediator logic doesn't need to change as long as the
VC being
shown conforms to the consumer protocol. And if a new VC requires additional
consumer methods,
they can be added to the protocol without having any impact on the old VC.

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm (right):

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:26: if
(!self.webState)
On 2017/02/02 22:17:52, edchin wrote:
> Can we be consistent in the use of property vs. instance variable accessors? 

I think I have been. Note that this is the setter method for webState, so
assigning to self.webState will become a recursive call to [self setWebState].

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm
(right):

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm:42:
[self updateContentView];
On 2017/02/02 22:17:52, edchin wrote:
> It's better balanced if you [self.view addSubview:self.contentView] here,
since
> you remove above. 

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.

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm:49:
self.contentView.translatesAutoresizingMaskIntoConstraints = NO;
On 2017/02/02 22:17:53, edchin wrote:
> Using autoresizing masks would be clearer and more concise for situations like
> this where the constraints are simply attaching to the parent edges.

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).

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm (right):

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm:39: _webState =
webState;
On 2017/02/02 22:17:53, edchin wrote:
> Consistent use of property accessor. 

Again, this is the setter method; I have to use the ivar directly.

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm:57: // better way in
the future.
On 2017/02/02 22:17:53, edchin wrote:
> It is not clear to me why we should set webUsageEnabled to false. Can you
> explain this comment?

webUsageEnabled means that the WebState is attached to a WKWebView instance. It
must be set to true before accessing the webState's view via GetView(), and
should be set to false when the web view isn't being displayed so  we can free
memory (among other things).

I'll expand the comment.

[marq (ping after 24h), 2017-02-03 15:00:01]

Review feedback, added tests.

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

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm (right):

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:26: if
(!self.webState)
On 2017/02/03 09:48:10, marq wrote:
> On 2017/02/02 22:17:52, edchin wrote:
> > Can we be consistent in the use of property vs. instance variable accessors?

> 
> I think I have been. Note that this is the setter method for webState, so
> assigning to self.webState will become a recursive call to [self setWebState].

You're right. I should have seen that this is a property setter.

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:34:
_webState->GetNavigationManager()->LoadURLWithParams(params);
Perhaps this one should actually be a property accessor.

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm
(right):

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm:42:
[self updateContentView];
On 2017/02/03 09:48:10, marq wrote:
> On 2017/02/02 22:17:52, edchin wrote:
> > It's better balanced if you [self.view addSubview:self.contentView] here,
> since
> > you remove above. 
> 
> 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.

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.

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm (right):

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm:39: _webState =
webState;
On 2017/02/03 09:48:10, marq wrote:
> On 2017/02/02 22:17:53, edchin wrote:
> > Consistent use of property accessor. 
> 
> Again, this is the setter method; I have to use the ivar directly.

You're right. I should have seen that this is the setter override.

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm:57: // better way in
the future.
On 2017/02/03 09:48:10, marq wrote:
> On 2017/02/02 22:17:53, edchin wrote:
> > It is not clear to me why we should set webUsageEnabled to false. Can you
> > explain this comment?
> 
> webUsageEnabled means that the WebState is attached to a WKWebView instance.
It
> must be set to true before accessing the webState's view via GetView(), and
> should be set to false when the web view isn't being displayed so  we can free
> memory (among other things).
> 
> I'll expand the comment.

Acknowledged.

[edchin, 2017-02-06 20:55:33]

LGTM.

> 'Delegate' because (a) this isn't a delegation relationship, and (b) there are
> UIKit 
> conventions about protocols named 'Delegate' that this doesn't (and shouldn't)
> follow.

ACK.


> My gut feeling is that that couples the view controller too tightly to the
> coordinator. 
> 
> Here are two litmus tests:
> 
> - Writing a test for the coordinator is harder, because it has to mock the
view
> controller's
> UI update methods. 

ACK. Makes sense to me.

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

Rebase, minor fix.

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

Thanks for the review!

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm (right):

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:34:
_webState->GetNavigationManager()->LoadURLWithParams(params);
On 2017/02/03 15:33:38, edchin wrote:
> Perhaps this one should actually be a property accessor.

Yes! Bad copy/paste from me.

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm
(right):

https://codereview.chromium.org/2675763002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm:42:
[self updateContentView];
On 2017/02/03 15:33:38, edchin wrote:
> On 2017/02/03 09:48:10, marq wrote:
> > On 2017/02/02 22:17:52, edchin wrote:
> > > It's better balanced if you [self.view addSubview:self.contentView] here,
> > since
> > > you remove above. 
> > 
> > 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.
> 
> 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.

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.

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

The CQ bit was checked by marq@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from edchin@chromium.org
Link to the patchset: https://codereview.chromium.org/2675763002/#ps60001
(title: "Rebase, minor fix.")

[commit-bot: I haz the power, 2017-02-07 14:33:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 14:33:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 14:33:32]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[marq (ping after 24h), 2017-02-07 14:35:47]

marq@chromium.org changed reviewers:
+ sdefresne@chromium.org

[marq (ping after 24h), 2017-02-07 14:35:47]

+sdefresne for committer review.

[sdefresne, 2017-02-07 14:44:53]

lgtm with nits

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/BUILD.gn (right):

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/BUILD.gn:5: source_set("web_contents")
{
(no action required) this is unfortunate name due to
//content/public/browser/web_contents.h ...

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.h (right):

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.h:20: // The
webState whose properties this object mediates. This can change during
s/webState/WebState/

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm (right):

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:26: if
(!self.webState)
For consistency, can you add braces around this "if" body? Or remove the braces
for the other simple "if"s?

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm
(right):

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm:34:
return;
ditto, can you add braces or remove them from the other "if" for consistency?

[marq (ping after 24h), 2017-02-07 15:01:38]

Feedback.

[marq (ping after 24h), 2017-02-07 15:01:39]

The CQ bit was checked by marq@chromium.org

[marq (ping after 24h), 2017-02-07 15:01:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, edchin@chromium.org
Link to the patchset: https://codereview.chromium.org/2675763002/#ps80001
(title: "Feedback.")

[commit-bot: I haz the power, 2017-02-07 15:02:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marq (ping after 24h), 2017-02-07 15:08:10]

Thanks for the review!

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/BUILD.gn (right):

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/BUILD.gn:5: source_set("web_contents")
{
On 2017/02/07 14:44:53, sdefresne wrote:
> (no action required) this is unfortunate name due to
> //content/public/browser/web_contents.h ...

Duly noted; it might become 'web container'.

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.h (right):

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.h:20: // The
webState whose properties this object mediates. This can change during
On 2017/02/07 14:44:53, sdefresne wrote:
> s/webState/WebState/

Done.

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm (right):

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:26: if
(!self.webState)
On 2017/02/07 14:44:53, sdefresne wrote:
> For consistency, can you add braces around this "if" body? Or remove the
braces
> for the other simple "if"s?

Done.

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm
(right):

https://codereview.chromium.org/2675763002/diff/60001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_view_controller.mm:34:
return;
On 2017/02/07 14:44:53, sdefresne wrote:
> ditto, can you add braces or remove them from the other "if" for consistency?

Done.

[sdefresne, 2017-02-07 15:13:16]

lgtm

[commit-bot: I haz the power, 2017-02-07 15:55:27]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486479699602800,
"parent_rev": "44f97665488a7d70481a4dc189e3b2dffa40f28f", "commit_rev":
"92ee2d62dea54502e0f2ab2b0874620c4bed8c69"}

[commit-bot: I haz the power, 2017-02-07 15:56:02]

Description was changed from

==========
This CL abstracts out WebContentsViewController's WebState dependency by
creating a
mediator and a consumer protocol.  

WebContentsConsumer is a "push" interface for a provider object (here the
mediator) to 
inform a consumer (here the view controller) about changes it should reflect in
its UI.
==========

to

==========
This CL abstracts out WebContentsViewController's WebState dependency by
creating a
mediator and a consumer protocol.  

WebContentsConsumer is a "push" interface for a provider object (here the
mediator) to 
inform a consumer (here the view controller) about changes it should reflect in
its UI.

Review-Url: https://codereview.chromium.org/2675763002
Cr-Commit-Position: refs/heads/master@{#448629}
Committed:
https://chromium.googlesource.com/chromium/src/+/92ee2d62dea54502e0f2ab2b0874...
==========

[commit-bot: I haz the power, 2017-02-07 15:56:03]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/92ee2d62dea54502e0f2ab2b0874...