[ios] Adds a CommandDispatcher to Browser.

[ios] Moves CommandDispatcher to ios/shared and adds it to Browser.

BUG=698748
Review-Url: https://codereview.chromium.org/2745223005
Cr-Commit-Position: refs/heads/master@{#459434}
Committed: https://chromium.googlesource.com/chromium/src/+/0203b9af9ab794f10f9d59c4aaa01f5873695ed0

[rohitrao (ping after 24h), 2017-03-15 14:55:51]

Description was changed from

==========
[ios] Adds a CommandDispatcher to Browser.

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

to

==========
[ios] Adds a CommandDispatcher to Browser.

BUG=698748
==========

[rohitrao (ping after 24h), 2017-03-15 14:57:36]

rohitrao@chromium.org changed reviewers:
+ lpromero@chromium.org, sdefresne@chromium.org

[rohitrao (ping after 24h), 2017-03-15 14:57:36]

The code here should be uncontroversial, but this adds a dependency from
ios/clean/chrome/browser -> ios/clean/chrome/browser/ui, which is bad.

Should the Browser class move into ios/clean/chrome/browser/ui?  Is this
fundamentally a model class or a ui class?

Another option is to move the dispatcher up into ios/clean/chrome/browser, but
the dispatcher feels like it's solidly a ui class.

[lpromero, 2017-03-15 17:38:50]

On 2017/03/15 14:57:36, rohitrao wrote:
> The code here should be uncontroversial, but this adds a dependency from
> ios/clean/chrome/browser -> ios/clean/chrome/browser/ui, which is bad.
> 
> Should the Browser class move into ios/clean/chrome/browser/ui?  Is this
> fundamentally a model class or a ui class?
> 
> Another option is to move the dispatcher up into ios/clean/chrome/browser, but
> the dispatcher feels like it's solidly a ui class.

To me the dispatcher doesn't belong to the UI layer, but implements interfaces
that are defined in the UI layer.

[lpromero, 2017-03-15 17:38:55]

https://codereview.chromium.org/2745223005/diff/1/ios/clean/chrome/browser/mo...
File ios/clean/chrome/browser/model/BUILD.gn (right):

https://codereview.chromium.org/2745223005/diff/1/ios/clean/chrome/browser/mo...
ios/clean/chrome/browser/model/BUILD.gn:17:
"//ios/clean/chrome/browser/ui/commands",
Should commands live on their own, outside of ui, and be a shared dep?

[rohitrao (ping after 24h), 2017-03-15 17:41:24]

> To me the dispatcher doesn't belong to the UI layer, but implements interfaces
> that are defined in the UI layer.

I'd like to have checkdeps enforce that ios/clean/chrome/browser cannot depend
on ios/clean/chrome/browser/ui.  If the dispatcher moves out of ui/, the
commands will probably need to as well.

[sdefresne, 2017-03-15 17:49:29]

On 2017/03/15 17:41:24, rohitrao wrote:
> > To me the dispatcher doesn't belong to the UI layer, but implements
interfaces
> > that are defined in the UI layer.
> 
> I'd like to have checkdeps enforce that ios/clean/chrome/browser cannot depend
> on ios/clean/chrome/browser/ui.  If the dispatcher moves out of ui/, the
> commands will probably need to as well.

Why does commands depends on command dispatcher?

IIRC the design, command dispatcher will not implement any interface (as
discussed in the CL adding command dispatcher). So, for me that means the
command dispatcher does not depend on the commands themselves. They may depend
on the dispatcher, but i/c/c/b/ui can depend on i/c/c/b so, I think it would be
okay to move command dispatcher outside of ui/.

[rohitrao (ping after 24h), 2017-03-15 17:52:43]

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

[rohitrao (ping after 24h), 2017-03-15 17:52:43]

The dispatcher is playing a role similar to the responder chain, which makes it
a "UI" class in my opinion.

Which do people prefer out of the two options?
1) Moving the dispatcher to c/browser
2) Moving Browser to c/b/ui

[lpromero, 2017-03-15 17:57:08]

On 2017/03/15 17:52:43, rohitrao wrote:
> The dispatcher is playing a role similar to the responder chain, which makes
it
> a "UI" class in my opinion.
> 
> Which do people prefer out of the two options?
> 1) Moving the dispatcher to c/browser
> 2) Moving Browser to c/b/ui

Doesn't Browser depend on BrowserState, WebState, etc?
I'd vote 1.

[edchin, 2017-03-15 18:05:53]

On 2017/03/15 17:57:08, lpromero wrote:
> On 2017/03/15 17:52:43, rohitrao wrote:
> > The dispatcher is playing a role similar to the responder chain, which makes
> it
> > a "UI" class in my opinion.
> > 
> > Which do people prefer out of the two options?
> > 1) Moving the dispatcher to c/browser
> > 2) Moving Browser to c/b/ui
> 
> Doesn't Browser depend on BrowserState, WebState, etc?
> I'd vote 1.

I vote 1 as well.

[sdefresne, 2017-03-15 18:08:22]

On 2017/03/15 17:57:08, lpromero wrote:
> On 2017/03/15 17:52:43, rohitrao wrote:
> > The dispatcher is playing a role similar to the responder chain, which makes
> it
> > a "UI" class in my opinion.
> > 
> > Which do people prefer out of the two options?
> > 1) Moving the dispatcher to c/browser
> > 2) Moving Browser to c/b/ui
> 
> Doesn't Browser depend on BrowserState, WebState, etc?
> I'd vote 1.

Mediators will also depends on model object and will probably live in ui/ so
there is some precedent for having code that depends on
BrowserState/WebState/... to live in ui/ (or there will be).

I see the command dispatcher as NSNotificationCenter, so not really ui/ but I
don't really mind whether Browser live in i/c/c/b or i/c/c/b/ui. It kind of
represent the state for a group of tab and the associated ui (tab strip), so it
is an hybrid, part ui-part model. Desktop code has Browser and BrowserWindow
where BrowserWindow is the real UI and Browser is more the model (though they
both live in c/b/ui).

[lpromero, 2017-03-15 18:17:27]

On 2017/03/15 18:08:22, sdefresne wrote:
> Mediators will also depends on model object and will probably live in ui/ so
> there is some precedent for having code that depends on
> BrowserState/WebState/... to live in ui/ (or there will be).

I might be conflating things. UI layer and ui/. Coordinators live in ui/ but are
not in the UI layer.
UI layer is composed of the sources in the xxx_ui targets. I would oppose
dispatcher, mediators and Browser to be added to these.
But I see the case of having them in ui/.
Mediators and dispatcher are on equal footing with coordinators, so I would
agree to have them in ui/. Browser is a bit different but I understand the point
now.

I think I am changing my ming and voting for 2 :p

[rohitrao (ping after 24h), 2017-03-15 18:19:43]

Our directory structure is a bit weird, because I think it's important to mirror
desktop.

Controllers and Views live in browser/ui/.  Models go in browser/.

[rohitrao (ping after 24h), 2017-03-15 18:20:49]

And you could argue that the dispatcher is neither Controller nor Model nor
View, but some sort of UI-related infrastructure.  Or non-UI-related
infrastructure =)

[marq (ping after 24h), 2017-03-16 08:35:56]

On 2017/03/15 18:20:49, rohitrao wrote:
> And you could argue that the dispatcher is neither Controller nor Model nor
> View, but some sort of UI-related infrastructure.  Or non-UI-related
> infrastructure =)

I see CommandDispatcher as a peer to BrowserCoordinator; it's an infrastructural
class. The major difference is that it doesn't get subclassed.

On the other hand, if we were implementing UIViewController (the base class), we
would probably put it in ui/.

I think what makes sense is for all of these classes to move into ui/ :

BrowserCoordinator
Browser
CommandDispatcher

(I don't know if they need to be in subdirectories or not; do we want to have a
file whose path is chrome/browser/ui/browser/browser.mm?)

[rohitrao (ping after 24h), 2017-03-23 19:51:25]

The CQ bit was checked by rohitrao@chromium.org to run a CQ dry run

[rohitrao (ping after 24h), 2017-03-23 19:51:49]

Description was changed from

==========
[ios] Adds a CommandDispatcher to Browser.

BUG=698748
==========

to

==========
[ios] Moves CommandDispatcher to ios/shared and adds it to Browser.

BUG=698748
==========

[commit-bot: I haz the power, 2017-03-23 19:52:03]

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

[rohitrao (ping after 24h), 2017-03-23 19:52:30]

rohitrao@chromium.org changed reviewers:
+ sczs@chromium.org

[rohitrao (ping after 24h), 2017-03-23 19:52:30]

Rebased this on top of the Browser move and moved CommandDispatcher to
ios/shared as well.

[edchin, 2017-03-23 20:03:37]

lgtm.

[lpromero, 2017-03-23 21:08:12]

lgtm

[lpromero, 2017-03-23 21:08:12]

lgtm

lgtm

[commit-bot: I haz the power, 2017-03-23 21:08:30]

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

[commit-bot: I haz the power, 2017-03-23 21:08:31]

Dry run: This issue passed the CQ dry run.

[sczs, 2017-03-24 01:22:26]

lgtm 
This is great since my dispatcher related CL needs this :)

[sdefresne, 2017-03-24 09:10:19]

lgtm with a comment

https://codereview.chromium.org/2745223005/diff/20001/ios/shared/chrome/brows...
File ios/shared/chrome/browser/ui/browser_list/browser.h (right):

https://codereview.chromium.org/2745223005/diff/20001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser.h:36: __strong
CommandDispatcher* dispatcher_;
We discussed having TabModel getting it's WebStateList from a Browser object in
the old architecture. This means that TabModel will need to know about Browser
(or the class creating the TabModel) which is not yet building with ARC.

I think using base::scoped_nsobject<> here will allow this without requiring
converting TabModel to ARC.

[rohitrao (ping after 24h), 2017-03-24 15:37:27]

The CQ bit was checked by rohitrao@chromium.org

[commit-bot: I haz the power, 2017-03-24 15:38:15]

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-03-24 15:43:53]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490369847339250,
"parent_rev": "f942ba0c142618d08df7eaab7619d61758cc8826", "commit_rev":
"8caebe1df247ab48f16c10c4a75793b31c7a5c0f"}

[commit-bot: I haz the power, 2017-03-24 15:49:37]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490369847339250,
"parent_rev": "774ad62e31004872f04c0a9041b1349062b92557", "commit_rev":
"0203b9af9ab794f10f9d59c4aaa01f5873695ed0"}

[commit-bot: I haz the power, 2017-03-24 15:50:21]

Description was changed from

==========
[ios] Moves CommandDispatcher to ios/shared and adds it to Browser.

BUG=698748
==========

to

==========
[ios] Moves CommandDispatcher to ios/shared and adds it to Browser.

BUG=698748

Review-Url: https://codereview.chromium.org/2745223005
Cr-Commit-Position: refs/heads/master@{#459434}
Committed:
https://chromium.googlesource.com/chromium/src/+/0203b9af9ab794f10f9d59c4aaa0...
==========

[commit-bot: I haz the power, 2017-03-24 15:50:22]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/0203b9af9ab794f10f9d59c4aaa0...