[ios clean] Creates ToolbarWebState Mediator

[ios clean] Creates ToolbarWebState Mediator

BUG=683793
Review-Url: https://codereview.chromium.org/2708013002
Cr-Commit-Position: refs/heads/master@{#452059}
Committed: https://chromium.googlesource.com/chromium/src/+/fe033cc08fe0572cd3ee971800db6482c7838563

[sczs, 2017-02-21 03:05:43]

Patchset #1 (id:1) has been deleted

[sczs, 2017-02-21 03:29:08]

sczs@chromium.org changed reviewers:
+ edchin@chromium.org, lpromero@chromium.org, marq@chromium.org

[sczs, 2017-02-21 03:29:09]

PTAL

Added a Mediator for Toolbar, I'm following similar pattern/naming as the
WebContents mediator, that way it'll be easier to change anything in the future
if needed.

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_mediator.h (right):

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_mediator.h:20: @interface
ToolbarWebStateMediator : NSObject<CRWWebStateObserver>
Should we create a Mediator base class if this pattern is going to be used more
widely?

[edchin, 2017-02-21 07:56:12]

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/BUILD.gn (right):

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/BUILD.gn:30: "toolbar_web_state_consumer.h",
I don't love that the name has a reference to "web_state".

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right):

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:43:
self.mediator.webState = self.webState;
The webState is used only as a pass-through to the mediator. It does not appear
to have any other function in this class. Can we remove it as a property, and
only have this setter instead?

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_consumer.h (right):

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_consumer.h:10: //
ToolbarWebStateConsumer sets the current appeareance of the Toolbar based on
nit: spelling. appearance.

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_mediator.mm (right):

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_mediator.mm:34:
[self.consumer setCurrentPageText:base::SysUTF8ToNSString(pageURL.spec())];
The consumer is a UI object therefore this call needs to be on the main thread.
I wonder if we can be certain that this callback is in the main thread?

[marq (ping after 24h), 2017-02-21 12:04:25]

LGTM modulo naming changes and Ed's comments.

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_consumer.h (right):

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_consumer.h:12: @protocol
ToolbarWebStateConsumer
The general model I'd like to adopt is that the view controller defines a
coordinator protocol for all of the properties it needs to be provided with.
Those shouldn't be connected to any particular data source. The mediator's job
is to take one or more data sources and combine information and events from them
into updates for the consumer.

With that in mind, this should just be 'ToolbarConsumer', and the mediator will
just be 'ToolbarMediator'. The mediator will happen to accept a webState as its
input, but that's essentially an implementation detail.

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_mediator.h (right):

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_mediator.h:20: @interface
ToolbarWebStateMediator : NSObject<CRWWebStateObserver>
On 2017/02/21 03:29:09, sczs wrote:
> Should we create a Mediator base class if this pattern is going to be used
more
> widely? 

There's no specific functionality of a Mediator (yet) that's worth encapsulating
in a base class, although it is good to keep an eye out for.

Please move the web_state_observer_bridge dependency and CRWWebStateObserver
protocol adoption into the .mm; they aren't part of the public API.

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_mediator.mm (right):

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_mediator.mm:34:
[self.consumer setCurrentPageText:base::SysUTF8ToNSString(pageURL.spec())];
On 2017/02/21 07:56:12, edchin wrote:
> The consumer is a UI object therefore this call needs to be on the main
thread.
> I wonder if we can be certain that this callback is in the main thread?

The CRWWebStateObserver and web::WebStateObserver headers should document that;
at the moment they don't. I'm pretty sure those observer methods are only called
on the main thread, but ask Eugene.

[sczs, 2017-02-21 17:28:28]

Thanks for the feedback, PTAL

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/BUILD.gn (right):

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/BUILD.gn:30: "toolbar_web_state_consumer.h",
On 2017/02/21 07:56:11, edchin wrote:
> I don't love that the name has a reference to "web_state".

Done.

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right):

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:43:
self.mediator.webState = self.webState;
On 2017/02/21 07:56:11, edchin wrote:
> The webState is used only as a pass-through to the mediator. It does not
appear
> to have any other function in this class. Can we remove it as a property, and
> only have this setter instead?

It will be used to pass the WebState to the ToolsMenu Coordinator, I know its
not currently used but its a small change and we know for sure that it will be
used. I'd like to leave it if its OK with you.

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_consumer.h (right):

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_consumer.h:10: //
ToolbarWebStateConsumer sets the current appeareance of the Toolbar based on
On 2017/02/21 07:56:11, edchin wrote:
> nit: spelling. appearance.

Done.

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_consumer.h:12: @protocol
ToolbarWebStateConsumer
On 2017/02/21 12:04:24, marq wrote:
> The general model I'd like to adopt is that the view controller defines a
> coordinator protocol for all of the properties it needs to be provided with.
> Those shouldn't be connected to any particular data source. The mediator's job
> is to take one or more data sources and combine information and events from
them
> into updates for the consumer.
> 
> With that in mind, this should just be 'ToolbarConsumer', and the mediator
will
> just be 'ToolbarMediator'. The mediator will happen to accept a webState as
its
> input, but that's essentially an implementation detail. 

Done.

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_mediator.h (right):

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_mediator.h:20: @interface
ToolbarWebStateMediator : NSObject<CRWWebStateObserver>
On 2017/02/21 12:04:24, marq wrote:
> On 2017/02/21 03:29:09, sczs wrote:
> > Should we create a Mediator base class if this pattern is going to be used
> more
> > widely? 
> 
> There's no specific functionality of a Mediator (yet) that's worth
encapsulating
> in a base class, although it is good to keep an eye out for.
> 
> Please move the web_state_observer_bridge dependency and CRWWebStateObserver
> protocol adoption into the .mm; they aren't part of the public API.

Done.

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_mediator.mm (right):

https://codereview.chromium.org/2708013002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_web_state_mediator.mm:34:
[self.consumer setCurrentPageText:base::SysUTF8ToNSString(pageURL.spec())];
On 2017/02/21 12:04:25, marq wrote:
> On 2017/02/21 07:56:12, edchin wrote:
> > The consumer is a UI object therefore this call needs to be on the main
> thread.
> > I wonder if we can be certain that this callback is in the main thread?
> 
> The CRWWebStateObserver and web::WebStateObserver headers should document
that;
> at the moment they don't. I'm pretty sure those observer methods are only
called
> on the main thread, but ask Eugene.

Asked Eugene and he told me that they all should be called on the main thread
(unless there's a bug of course). I'll send a quick CL later to update those
headers.

[edchin, 2017-02-21 18:39:09]

https://codereview.chromium.org/2708013002/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.h (right):

https://codereview.chromium.org/2708013002/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.h:21:
@property(nonatomic, assign) web::WebState* webState;
If this webState is required, consider a custom initializer.

https://codereview.chromium.org/2708013002/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right):

https://codereview.chromium.org/2708013002/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:43:
self.mediator.webState = self.webState;
Properly handle when the webState is nil or changes after the coordinator is
started.

https://codereview.chromium.org/2708013002/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:49:
self.mediator.consumer = self.viewController;
Create the Mediator here and set it's webState and consumer all at once.

[sczs, 2017-02-21 19:04:10]

Hey Ed, I think you bring valid points but I'm not sure if we should address
them on this CL. 
We should have a meeting/discussion and decide on a standard way of handling
coordinators and its mediators, and I'll be more than happy to update all of
them.

https://codereview.chromium.org/2708013002/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.h (right):

https://codereview.chromium.org/2708013002/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.h:21:
@property(nonatomic, assign) web::WebState* webState;
On 2017/02/21 18:39:08, edchin wrote:
> If this webState is required, consider a custom initializer. 

I was thinking the exact same thing! But I'm not sure what's our policy on
custom initializers for Coordinators, and since Mark didn't create one for web I
thought we should keep things similar.

https://codereview.chromium.org/2708013002/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right):

https://codereview.chromium.org/2708013002/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:43:
self.mediator.webState = self.webState;
On 2017/02/21 18:39:08, edchin wrote:
> Properly handle when the webState is nil or changes after the coordinator is
> started.

Since this is a "parallel" coordinator I think it would be a good idea to leave
this as similar to:
https://codereview.chromium.org/2675763002/diff/80001/ios/clean/chrome/browse...
It would be weird to handle things differently and initialize them differently
from TabContainer. Also, since mediators are still on a Placeholder sorts of
stage, making any future changes would be easier to implement in both
Coordinators/Mediators.

[chromium-reviews, 2017-02-21 19:09:08]

Sounds good to me. LGTM.

On Tue, Feb 21, 2017, 11:04 AM <sczs@chromium.org> wrote:

> Hey Ed, I think you bring valid points but I'm not sure if we should
> address
> them on this CL.
> We should have a meeting/discussion and decide on a standard way of
> handling
> coordinators and its mediators, and I'll be more than happy to update all
> of
> them.
>
>
>
>
https://codereview.chromium.org/2708013002/diff/40001/ios/clean/chrome/browse...
> File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.h (right):
>
>
>
https://codereview.chromium.org/2708013002/diff/40001/ios/clean/chrome/browse...
> ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.h:21:
> @property(nonatomic, assign) web::WebState* webState;
> On 2017/02/21 18:39:08, edchin wrote:
> > If this webState is required, consider a custom initializer.
>
> I was thinking the exact same thing! But I'm not sure what's our policy
> on custom initializers for Coordinators, and since Mark didn't create
> one for web I thought we should keep things similar.
>
>
>
>
https://codereview.chromium.org/2708013002/diff/40001/ios/clean/chrome/browse...
> File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right):
>
>
>
https://codereview.chromium.org/2708013002/diff/40001/ios/clean/chrome/browse...
> ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:43:
> self.mediator.webState = self.webState;
> On 2017/02/21 18:39:08, edchin wrote:
> > Properly handle when the webState is nil or changes after the
> coordinator is
> > started.
>
> Since this is a "parallel" coordinator I think it would be a good idea
> to leave this as similar to:
>
>
https://codereview.chromium.org/2675763002/diff/80001/ios/clean/chrome/browse...
> It would be weird to handle things differently and initialize them
> differently from TabContainer. Also, since mediators are still on a
> Placeholder sorts of stage, making any future changes would be easier to
> implement in both Coordinators/Mediators.
>
> https://codereview.chromium.org/2708013002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sczs, 2017-02-21 19:10:11]

The CQ bit was checked by sczs@chromium.org

[sczs, 2017-02-21 19:10:12]

The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2708013002/#ps40001
(title: "Renaming feedback")

[commit-bot: I haz the power, 2017-02-21 19:10:53]

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-21 21:14:29]

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

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

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[sczs, 2017-02-22 15:21:20]

The CQ bit was checked by sczs@chromium.org

[commit-bot: I haz the power, 2017-02-22 15:21:43]

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-22 15:26:23]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487776880696270,
"parent_rev": "4df731b9954e2973ae8e206185f3de4556a806be", "commit_rev":
"fe033cc08fe0572cd3ee971800db6482c7838563"}

[commit-bot: I haz the power, 2017-02-22 15:26:59]

Description was changed from

==========
[ios clean] Creates ToolbarWebState Mediator

BUG=683793
==========

to

==========
[ios clean] Creates ToolbarWebState Mediator

BUG=683793

Review-Url: https://codereview.chromium.org/2708013002
Cr-Commit-Position: refs/heads/master@{#452059}
Committed:
https://chromium.googlesource.com/chromium/src/+/fe033cc08fe0572cd3ee971800db...
==========

[commit-bot: I haz the power, 2017-02-22 15:27:01]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/fe033cc08fe0572cd3ee971800db...