[ios clean] Adds ToolsMenu to Showcase.

[ios clean] Adds ToolsMenu to Showcase.

ToolsMenu wasn't working in showcase since it now depends on a model. This CL
fixes that issue.

It also removes the tools_actions file since it was not needed anymore.

Screenshot CompactWidth:
https://drive.google.com/open?id=0Byo6-Nuda2jgTVB1VUVsUUJNeFE

Screenshot RegularWidth:
https://drive.google.com/open?id=0Byo6-Nuda2jgOUhXYU0ycTRFdTA

BUG=682880
Review-Url: https://codereview.chromium.org/2944703002
Cr-Commit-Position: refs/heads/master@{#481162}
Committed: https://chromium.googlesource.com/chromium/src/+/16cafbab6bef1dce2175df58a54028effdacd074

[sczs, 2017-06-16 20:44:21]

Description was changed from

==========
[ios clean] Adds ToolsMenu to Showcase.

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

to

==========
[ios clean] Adds ToolsMenu to Showcase.

ToolsMenu wasn't working in showcase since it now depends on a model. This CL
fixes that issue.

Screenshot:
https://drive.google.com/open?id=0Byo6-Nuda2jgTVB1VUVsUUJNeFE

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

[sczs, 2017-06-16 20:44:21]

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

[sczs, 2017-06-16 20:46:34]

PTAL, it should be quick

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

https://codereview.chromium.org/2944703002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:240:
[self.closeMenuButton.topAnchor
Missing constraint that Showcase helped me notice :P

[lpromero, 2017-06-19 09:38:49]

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

https://codereview.chromium.org/2944703002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:240:
[self.closeMenuButton.topAnchor
On 2017/06/16 20:46:34, sczs wrote:
> Missing constraint that Showcase helped me notice :P  

Awesome!

https://codereview.chromium.org/2944703002/diff/1/ios/showcase/tools_menu/BUI...
File ios/showcase/tools_menu/BUILD.gn (right):

https://codereview.chromium.org/2944703002/diff/1/ios/showcase/tools_menu/BUI...
ios/showcase/tools_menu/BUILD.gn:13:
"//ios/clean/chrome/browser/ui/tools:tools",
We shouldn't add this target as a dependency in Showcase. This is to get
itemsModelList. Even if it has model in its name, should it be moved to the
tools_ui target?
Otherwise, in Showcase, we need to mock it, maybe even with bogus values.

https://codereview.chromium.org/2944703002/diff/1/ios/showcase/tools_menu/sc_...
File ios/showcase/tools_menu/sc_tools_coordinator.mm (right):

https://codereview.chromium.org/2944703002/diff/1/ios/showcase/tools_menu/sc_...
ios/showcase/tools_menu/sc_tools_coordinator.mm:53: // The Overflow controls
will only be displayed on CompactWidth SizeClasses.
Can you add a screenshot for non compact?

https://codereview.chromium.org/2944703002/diff/1/ios/showcase/tools_menu/sc_...
ios/showcase/tools_menu/sc_tools_coordinator.mm:57: // need to set the
navigation translucency to NO so it doesn't cover the
Remove "we need to". It avoids "we" and should spare one line!

[sczs, 2017-06-19 19:41:34]

Description was changed from

==========
[ios clean] Adds ToolsMenu to Showcase.

ToolsMenu wasn't working in showcase since it now depends on a model. This CL
fixes that issue.

Screenshot:
https://drive.google.com/open?id=0Byo6-Nuda2jgTVB1VUVsUUJNeFE

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

to

==========
[ios clean] Adds ToolsMenu to Showcase.

ToolsMenu wasn't working in showcase since it now depends on a model. This CL
fixes that issue.

It also removes the tools_actions file since it was not needed anymore.

Screenshot:
https://drive.google.com/open?id=0Byo6-Nuda2jgTVB1VUVsUUJNeFE

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

[sczs, 2017-06-19 19:43:01]

Description was changed from

==========
[ios clean] Adds ToolsMenu to Showcase.

ToolsMenu wasn't working in showcase since it now depends on a model. This CL
fixes that issue.

It also removes the tools_actions file since it was not needed anymore.

Screenshot:
https://drive.google.com/open?id=0Byo6-Nuda2jgTVB1VUVsUUJNeFE

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

to

==========
[ios clean] Adds ToolsMenu to Showcase.

ToolsMenu wasn't working in showcase since it now depends on a model. This CL
fixes that issue.

It also removes the tools_actions file since it was not needed anymore.

Screenshot CompactWidth:
https://drive.google.com/open?id=0Byo6-Nuda2jgTVB1VUVsUUJNeFE

Screenshot RegularWidth:
https://drive.google.com/open?id=0Byo6-Nuda2jgOUhXYU0ycTRFdTA

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

[sczs, 2017-06-19 19:44:38]

Thanks for the comments. PTAL
Also realized I could do some extra clean up by deleting tools_action.h

https://codereview.chromium.org/2944703002/diff/1/ios/showcase/tools_menu/BUI...
File ios/showcase/tools_menu/BUILD.gn (right):

https://codereview.chromium.org/2944703002/diff/1/ios/showcase/tools_menu/BUI...
ios/showcase/tools_menu/BUILD.gn:13:
"//ios/clean/chrome/browser/ui/tools:tools",
On 2017/06/19 09:38:49, lpromero wrote:
> We shouldn't add this target as a dependency in Showcase. This is to get
> itemsModelList. Even if it has model in its name, should it be moved to the
> tools_ui target?
> Otherwise, in Showcase, we need to mock it, maybe even with bogus values.

I've created a new target for the model, I think it makes sense and it solves
our problem. I guess I could also rename tools to tools_controller, I think it
could be a nice "standard" to have across the app. WDYT?

https://codereview.chromium.org/2944703002/diff/1/ios/showcase/tools_menu/sc_...
File ios/showcase/tools_menu/sc_tools_coordinator.mm (right):

https://codereview.chromium.org/2944703002/diff/1/ios/showcase/tools_menu/sc_...
ios/showcase/tools_menu/sc_tools_coordinator.mm:53: // The Overflow controls
will only be displayed on CompactWidth SizeClasses.
On 2017/06/19 09:38:49, lpromero wrote:
> Can you add a screenshot for non compact?

Done.

https://codereview.chromium.org/2944703002/diff/1/ios/showcase/tools_menu/sc_...
ios/showcase/tools_menu/sc_tools_coordinator.mm:57: // need to set the
navigation translucency to NO so it doesn't cover the
On 2017/06/19 09:38:49, lpromero wrote:
> Remove "we need to". It avoids "we" and should spare one line!

Aaaghhh, my bad! Thanks for pointing it out.

[lpromero, 2017-06-20 08:50:23]

LGMT, but I think just keeping tools and tools_ui would be way better for
consistency and clarity. Having a target called _model but being actually in the
UI layer would be confusing.

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

https://codereview.chromium.org/2944703002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tools/BUILD.gn:31: source_set("tools_model") {
Why not just fold it into tools_ui? It's called model, but it's considered part
of the UI layer in your case.

[sczs, 2017-06-21 01:31:43]

Thanks Louis! Seems good to me, I was thinking that having a nice separation of
MVC targets could work out nice. But we wouldn't have consistency, specially
now. If later on the idea makes more sense I'll propose it again. 

P.S. You had a typo on your previous "LGTM", so I will need one again :P

[lpromero, 2017-06-21 08:31:25]

Duh, sorry :( I reviewed to spare one day of back and forth but
flaied^Wfalied^Wfailed…

I am making an automatic text substitution for that one now.

LGTM!

[lpromero, 2017-06-21 08:31:29]

The CQ bit was checked by lpromero@chromium.org

[commit-bot: I haz the power, 2017-06-21 08:31: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-06-21 10:03:26]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498033889646270,
"parent_rev": "64e2012a9e0278ff735674d16db1c44caefc91ae", "commit_rev":
"16cafbab6bef1dce2175df58a54028effdacd074"}

[commit-bot: I haz the power, 2017-06-21 10:03:39]

Description was changed from

==========
[ios clean] Adds ToolsMenu to Showcase.

ToolsMenu wasn't working in showcase since it now depends on a model. This CL
fixes that issue.

It also removes the tools_actions file since it was not needed anymore.

Screenshot CompactWidth:
https://drive.google.com/open?id=0Byo6-Nuda2jgTVB1VUVsUUJNeFE

Screenshot RegularWidth:
https://drive.google.com/open?id=0Byo6-Nuda2jgOUhXYU0ycTRFdTA

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

to

==========
[ios clean] Adds ToolsMenu to Showcase.

ToolsMenu wasn't working in showcase since it now depends on a model. This CL
fixes that issue.

It also removes the tools_actions file since it was not needed anymore.

Screenshot CompactWidth:
https://drive.google.com/open?id=0Byo6-Nuda2jgTVB1VUVsUUJNeFE

Screenshot RegularWidth:
https://drive.google.com/open?id=0Byo6-Nuda2jgOUhXYU0ycTRFdTA

BUG=682880

Review-Url: https://codereview.chromium.org/2944703002
Cr-Commit-Position: refs/heads/master@{#481162}
Committed:
https://chromium.googlesource.com/chromium/src/+/16cafbab6bef1dce2175df58a540...
==========

[commit-bot: I haz the power, 2017-06-21 10:03:40]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/16cafbab6bef1dce2175df58a540...

[sczs, 2017-06-21 15:51:45]

On 2017/06/21 08:31:25, lpromero wrote:
> Duh, sorry :( I reviewed to spare one day of back and forth but
> flaied^Wfalied^Wfailed…
> 
> I am making an automatic text substitution for that one now.
> 
> LGTM!

No worries :) thanks for taking a look and sending to the CQ