[ios clean] Use ToolsMenuConfiguration for Menu context.

[ios clean] Use ToolsMenuConfiguration for Menu context.

Screenshots:
https://drive.google.com/file/d/0Byo6-Nuda2jgaDNxMVYtWmEtUXc/view?usp=sharing
https://drive.google.com/file/d/0Byo6-Nuda2jgaXJEYmZ3RzdVUlU/view?usp=sharing

BUG=682880
Review-Url: https://codereview.chromium.org/2814963002
Cr-Commit-Position: refs/heads/master@{#465659}
Committed: https://chromium.googlesource.com/chromium/src/+/31019d751c35578034f6bf1f37467c8aa9ffff70

[sczs, 2017-04-12 00:08:03]

Description was changed from

==========
[ios clean] Use ToolsMenuConfiguration for Menu context.

BUG=682880
==========

to

==========
[ios clean] Use ToolsMenuConfiguration for Menu context.

This is still a Draft CL.

BUG=682880
==========

[sczs, 2017-04-12 00:08:03]

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

[sczs, 2017-04-12 00:59:26]

Simple Draft CL, PTAL and let me know what you think of the overall idea.

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

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:7: #import
"ios/chrome/browser/ui/tools_menu/tools_menu_configuration.h"
Realized I need to update the BUILD.gn file, but importing from
ios/chrome/browser... is OK, right?

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:106:
[[ToolsMenuConfiguration alloc] initWithDisplayView:nil];
Currently [[ToolsMenuConfiguration alloc] init] is disabled, I will refactor it
a little bit.

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:108:
toolsCoordinator.toolsMenuConfiguration = menuConfiguration;
This is my main question, should the Coordinator initializing the ToolsMenu
Coordinator configure the ToolsMenuConfiguration? 
Or should we use the UI Model so the ToolsMenu Coordinator configures its own
ToolsMenuConfiguration object?

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tools/tools_consumer.h (right):

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/tools_consumer.h:18: -
(void)canDisplayOverflowControls:(BOOL)displayOverflowControls;
Name is not final, but any suggestions are welcome.

[marq (ping after 24h), 2017-04-12 11:17:08]

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

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:7: #import
"ios/chrome/browser/ui/tools_menu/tools_menu_configuration.h"
On 2017/04/12 00:59:26, sczs wrote:
> Realized I need to update the BUILD.gn file, but importing from
> ios/chrome/browser... is OK, right? 

Currently the rules are:

/clean can import from /shared and /chrome, but the /chrome imports will need to
be cleaned up later, so they should only be done as part of the "hacking in"
process of refactoring.
/shared can't import from either /chrome or /clean
/chrome can import from /shared but not /clean

So ideally, if you can move the tools menu configuration files into shared/,
that would be better. (and in a separate CL).

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:108:
toolsCoordinator.toolsMenuConfiguration = menuConfiguration;
On 2017/04/12 00:59:26, sczs wrote:
> This is my main question, should the Coordinator initializing the ToolsMenu
> Coordinator configure the ToolsMenuConfiguration? 
> Or should we use the UI Model so the ToolsMenu Coordinator configures its own
> ToolsMenuConfiguration object? 

Conceptually ToolsMenuConfiguration is model code, so ideally the
ToolsMenuMediator just creates the instance it needs and nobody else knows
anything about it.

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right):

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:121: -
(void)canDisplayOverflowControls:(BOOL)displayOverflowControls {
This depends on device size/format, which is something I've been trying to keep
confined to the view controller layer. What do we gain by having this pushed in
from the mediator?

[sczs, 2017-04-12 15:06:08]

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

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:7: #import
"ios/chrome/browser/ui/tools_menu/tools_menu_configuration.h"
On 2017/04/12 11:17:07, marq wrote:
> On 2017/04/12 00:59:26, sczs wrote:
> > Realized I need to update the BUILD.gn file, but importing from
> > ios/chrome/browser... is OK, right? 
> 
> Currently the rules are:
> 
> /clean can import from /shared and /chrome, but the /chrome imports will need
to
> be cleaned up later, so they should only be done as part of the "hacking in"
> process of refactoring.
> /shared can't import from either /chrome or /clean
> /chrome can import from /shared but not /clean
> 
> So ideally, if you can move the tools menu configuration files into shared/,
> that would be better. (and in a separate CL).

Understood, will start working on moving that file!

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:108:
toolsCoordinator.toolsMenuConfiguration = menuConfiguration;
On 2017/04/12 11:17:07, marq wrote:
> On 2017/04/12 00:59:26, sczs wrote:
> > This is my main question, should the Coordinator initializing the ToolsMenu
> > Coordinator configure the ToolsMenuConfiguration? 
> > Or should we use the UI Model so the ToolsMenu Coordinator configures its
own
> > ToolsMenuConfiguration object? 
> 
> Conceptually ToolsMenuConfiguration is model code, so ideally the
> ToolsMenuMediator just creates the instance it needs and nobody else knows
> anything about it.

The problem is that ToolsMenuConfiguration needs to know about the current
context of the UI so we can configure it accordingly.
e.g. It needs to know if we are on the TabGrid or in "navigation/toolbar" mode,
because we will never show the overflow controls on the TabGrid ToolsMenu, even
if we are on compact width.

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right):

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:121: -
(void)canDisplayOverflowControls:(BOOL)displayOverflowControls {
On 2017/04/12 11:17:07, marq wrote:
> This depends on device size/format, which is something I've been trying to
keep
> confined to the view controller layer. What do we gain by having this pushed
in
> from the mediator?

This also depends on who is displaying the ToolsMenu, since we won't display the
overflow controls on the TabGrid ToolsMenu for example.

[marq (ping after 24h), 2017-04-14 10:01:52]

Overall LGTM

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

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:108:
toolsCoordinator.toolsMenuConfiguration = menuConfiguration;
On 2017/04/12 15:06:07, sczs wrote:
> On 2017/04/12 11:17:07, marq wrote:
> > On 2017/04/12 00:59:26, sczs wrote:
> > > This is my main question, should the Coordinator initializing the
ToolsMenu
> > > Coordinator configure the ToolsMenuConfiguration? 
> > > Or should we use the UI Model so the ToolsMenu Coordinator configures its
> own
> > > ToolsMenuConfiguration object? 
> > 
> > Conceptually ToolsMenuConfiguration is model code, so ideally the
> > ToolsMenuMediator just creates the instance it needs and nobody else knows
> > anything about it.
> 
> The problem is that ToolsMenuConfiguration needs to know about the current
> context of the UI so we can configure it accordingly.
> e.g. It needs to know if we are on the TabGrid or in "navigation/toolbar"
mode,
> because we will never show the overflow controls on the TabGrid ToolsMenu,
even
> if we are on compact width. 

Ah, OK; I missed that. Then, yes the coordinator that starts the menu
coordinator should pass in a menu config to the menu coordinator, like you are
doing here.

I'd also prefer to see (in another CL is fine) ToolsMenuConfiguration subclasses
for the switcher/toolbar cases rather than a property with conditional logic in
the class. Any time we can extract conditions that won't change at runtime into
separate implementations we should do so (I think).

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right):

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:121: -
(void)canDisplayOverflowControls:(BOOL)displayOverflowControls {
On 2017/04/12 15:06:08, sczs wrote:
> On 2017/04/12 11:17:07, marq wrote:
> > This depends on device size/format, which is something I've been trying to
> keep
> > confined to the view controller layer. What do we gain by having this pushed
> in
> > from the mediator?
> 
> This also depends on who is displaying the ToolsMenu, since we won't display
the
> overflow controls on the TabGrid ToolsMenu for example.

Indeed! My mistake.

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tools/tools_consumer.h (right):

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/tools_consumer.h:18: -
(void)canDisplayOverflowControls:(BOOL)displayOverflowControls;
On 2017/04/12 00:59:26, sczs wrote:
> Name is not final, but any suggestions are welcome. 

Total nit, but:

rather than -canDisplay ..., i'd just go with displaysOverflowControls ...

"can" implies that this might change during the lifetime of the consumer, when
it can't.

[lpromero, 2017-04-14 13:12:41]

lpromero@chromium.org changed reviewers:
- lpromero@chromium.org

[sczs, 2017-04-14 22:15:17]

Patchset #2 (id:20001) has been deleted

[sczs, 2017-04-14 22:21:07]

This is ready for review. PTAL

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

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:108:
toolsCoordinator.toolsMenuConfiguration = menuConfiguration;
On 2017/04/14 10:01:51, marq wrote:
> On 2017/04/12 15:06:07, sczs wrote:
> > On 2017/04/12 11:17:07, marq wrote:
> > > On 2017/04/12 00:59:26, sczs wrote:
> > > > This is my main question, should the Coordinator initializing the
> ToolsMenu
> > > > Coordinator configure the ToolsMenuConfiguration? 
> > > > Or should we use the UI Model so the ToolsMenu Coordinator configures
its
> > own
> > > > ToolsMenuConfiguration object? 
> > > 
> > > Conceptually ToolsMenuConfiguration is model code, so ideally the
> > > ToolsMenuMediator just creates the instance it needs and nobody else knows
> > > anything about it.
> > 
> > The problem is that ToolsMenuConfiguration needs to know about the current
> > context of the UI so we can configure it accordingly.
> > e.g. It needs to know if we are on the TabGrid or in "navigation/toolbar"
> mode,
> > because we will never show the overflow controls on the TabGrid ToolsMenu,
> even
> > if we are on compact width. 
> 
> Ah, OK; I missed that. Then, yes the coordinator that starts the menu
> coordinator should pass in a menu config to the menu coordinator, like you are
> doing here.
> 
> I'd also prefer to see (in another CL is fine) ToolsMenuConfiguration
subclasses
> for the switcher/toolbar cases rather than a property with conditional logic
in
> the class. Any time we can extract conditions that won't change at runtime
into
> separate implementations we should do so (I think).

Ack. Will merge this as an initial POC and keep iterating on how the Config is
set up.

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tools/tools_consumer.h (right):

https://codereview.chromium.org/2814963002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/tools_consumer.h:18: -
(void)canDisplayOverflowControls:(BOOL)displayOverflowControls;
On 2017/04/14 10:01:52, marq wrote:
> On 2017/04/12 00:59:26, sczs wrote:
> > Name is not final, but any suggestions are welcome. 
> 
> Total nit, but:
> 
> rather than -canDisplay ..., i'd just go with displaysOverflowControls ...
> 
> "can" implies that this might change during the lifetime of the consumer, when
> it can't.

Agree, I was thinking of can because this is not the only criteria for showing
the Menu. I'll change it since I think its more important to not give the
impression it changes over the lifetime of the consumer.

[sczs, 2017-04-14 22:38:58]

Description was changed from

==========
[ios clean] Use ToolsMenuConfiguration for Menu context.

This is still a Draft CL.

BUG=682880
==========

to

==========
[ios clean] Use ToolsMenuConfiguration for Menu context.

Screenshots:
https://drive.google.com/file/d/0Byo6-Nuda2jgaDNxMVYtWmEtUXc/view?usp=sharing
https://drive.google.com/file/d/0Byo6-Nuda2jgaXJEYmZ3RzdVUlU/view?usp=sharing

BUG=682880
==========

[lpromero, 2017-04-18 09:37:01]

lpromero@chromium.org changed reviewers:
+ lpromero@chromium.org

[lpromero, 2017-04-18 09:37:01]

lgtm

https://codereview.chromium.org/2814963002/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tools/tools_consumer.h (right):

https://codereview.chromium.org/2814963002/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tools/tools_consumer.h:17: // Sets a flag so the
consumer knows if it should display the Menu overflow
Optional nit: maybe setDisplayOverflowControls would be clearer?

[sczs, 2017-04-19 14:33:04]

https://codereview.chromium.org/2814963002/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tools/tools_consumer.h (right):

https://codereview.chromium.org/2814963002/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tools/tools_consumer.h:17: // Sets a flag so the
consumer knows if it should display the Menu overflow
On 2017/04/18 09:37:01, lpromero wrote:
> Optional nit: maybe setDisplayOverflowControls would be clearer?

I agree. Done.

[sczs, 2017-04-19 14:33:27]

The CQ bit was checked by sczs@chromium.org

[sczs, 2017-04-19 14:33:27]

The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org,
lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2814963002/#ps60001
(title: "Rebase and nit")

[commit-bot: I haz the power, 2017-04-19 14:33:51]

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-04-19 14:42:46]

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

[commit-bot: I haz the power, 2017-04-19 14:42:47]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sczs, 2017-04-19 15:36:46]

The CQ bit was checked by sczs@chromium.org

[sczs, 2017-04-19 15:36:47]

The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org,
lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2814963002/#ps80001
(title: "Update unittest")

[commit-bot: I haz the power, 2017-04-19 15:37:13]

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-04-19 15:49:21]

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

[commit-bot: I haz the power, 2017-04-19 15:49:22]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[sczs, 2017-04-19 16:26:11]

Patchset #4 (id:80001) has been deleted

[sczs, 2017-04-19 16:26:32]

The CQ bit was checked by sczs@chromium.org

[sczs, 2017-04-19 16:26:32]

The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org,
lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2814963002/#ps100001
(title: "Update Unittest")

[commit-bot: I haz the power, 2017-04-19 16:26:51]

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-04-19 17:40:25]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492619192449540,
"parent_rev": "a936ad274d8d2f8967fb39dd8150488e0383ab8f", "commit_rev":
"31019d751c35578034f6bf1f37467c8aa9ffff70"}

[commit-bot: I haz the power, 2017-04-19 17:40:36]

Description was changed from

==========
[ios clean] Use ToolsMenuConfiguration for Menu context.

Screenshots:
https://drive.google.com/file/d/0Byo6-Nuda2jgaDNxMVYtWmEtUXc/view?usp=sharing
https://drive.google.com/file/d/0Byo6-Nuda2jgaXJEYmZ3RzdVUlU/view?usp=sharing

BUG=682880
==========

to

==========
[ios clean] Use ToolsMenuConfiguration for Menu context.

Screenshots:
https://drive.google.com/file/d/0Byo6-Nuda2jgaDNxMVYtWmEtUXc/view?usp=sharing
https://drive.google.com/file/d/0Byo6-Nuda2jgaXJEYmZ3RzdVUlU/view?usp=sharing

BUG=682880

Review-Url: https://codereview.chromium.org/2814963002
Cr-Commit-Position: refs/heads/master@{#465659}
Committed:
https://chromium.googlesource.com/chromium/src/+/31019d751c35578034f6bf1f3746...
==========

[commit-bot: I haz the power, 2017-04-19 17:40:37]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/31019d751c35578034f6bf1f3746...