[ios clean] Dismisses ToolsMenu on Layout changes.

[ios] Dismisses ToolsMenu on Layout changes.

Re-adding this now that we don't depend on the UI responderChain for dismissing 
the ToolsMenu.

BUG=682880
Review-Url: https://codereview.chromium.org/2869903002
Cr-Commit-Position: refs/heads/master@{#470961}
Committed: https://chromium.googlesource.com/chromium/src/+/59e55ca446fab6e48a6da7687044b3998e319052

[sczs, 2017-05-08 18:41:03]

Description was changed from

==========
[ios] Dismisses ToolsMenu on Layout changes.

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

to

==========
[ios] Dismisses ToolsMenu on Layout changes.

Re-adding this now that we don't depend on the UI responderChain for dismissing 
the ToolsMenu.

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

[sczs, 2017-05-08 18:41:03]

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

[sczs, 2017-05-08 18:45:21]

Small CL, please take a quick look.

[marq (ping after 24h), 2017-05-09 12:13:27]

https://codereview.chromium.org/2869903002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm (right):

https://codereview.chromium.org/2869903002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:90: // (e.g.
Size changes, rotation changes, etc.)
This is probably fine, but note that the conditions you list are not the
conditions for what triggers a layout. If any change is made to the view
hierarchy -- resizing a subview, for example -- that may also cause this to
trigger.

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

https://codereview.chromium.org/2869903002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:102:
[self.dispatcher closeToolsMenu];
I'm pretty sure this isn't what we want. What if setCanGoForward:NO is called by
the mediator while the menu is showing? That will cause the forward button to
hide, which will trigger a layout pass, which will dismiss the tools menu.

[sczs, 2017-05-10 02:25:46]

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

[sczs, 2017-05-10 02:25:47]

Addressed the comments, PTAL.

https://codereview.chromium.org/2869903002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm (right):

https://codereview.chromium.org/2869903002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:90: // (e.g.
Size changes, rotation changes, etc.)
On 2017/05/09 12:13:26, marq (ping after 24h) wrote:
> This is probably fine, but note that the conditions you list are not the
> conditions for what triggers a layout. If any change is made to the view
> hierarchy -- resizing a subview, for example -- that may also cause this to
> trigger. 

Yes, you're right! I was stating what are we trying to achieve, or why are we
doing this. My own comment was showing me that something wasn't right. I've now
updated this.

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

https://codereview.chromium.org/2869903002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:102:
[self.dispatcher closeToolsMenu];
On 2017/05/09 12:13:26, marq (ping after 24h) wrote:
> I'm pretty sure this isn't what we want. What if setCanGoForward:NO is called
by
> the mediator while the menu is showing? That will cause the forward button to
> hide, which will trigger a layout pass, which will dismiss the tools menu.

Completely right, thanks for pointing this out. I tried a couple of things but I
think checking if the frame changed is our best bet all things considered.

I also tried setting a flag for Toolbar Button visibility changes, but any new
component would still trigger the dismissal. I think the approach I chose is the
most maintainable.

[marq (ping after 24h), 2017-05-10 10:57:02]

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

https://codereview.chromium.org/2869903002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:106: if
(!CGRectEqualToRect(self.currentFrame, self.view.frame)) {
UIViewController (via the UIContentContainer protocol which it conforms to) has
a method specifically for catching changes to its view size:
-viewWillTransitionToSize:withTransitionCoordinator:. I think that's what you
should use for this.

[sczs, 2017-05-10 15:32:32]

Feedback addressed. PTAL

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

https://codereview.chromium.org/2869903002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:106: if
(!CGRectEqualToRect(self.currentFrame, self.view.frame)) {
On 2017/05/10 10:57:02, marq (ping after 24h) wrote:
> UIViewController (via the UIContentContainer protocol which it conforms to)
has
> a method specifically for catching changes to its view size:
> -viewWillTransitionToSize:withTransitionCoordinator:. I think that's what you
> should use for this. 

I agree, its much better.
Done.

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

lgtm

[edchin, 2017-05-11 15:07:34]

lgtm

[sczs, 2017-05-11 15:08:50]

The CQ bit was checked by sczs@chromium.org

[commit-bot: I haz the power, 2017-05-11 15:09:22]

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-05-11 15:22:54]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494515330142980,
"parent_rev": "6ed35784e883877394fb71901f3579749d8c05a9", "commit_rev":
"59e55ca446fab6e48a6da7687044b3998e319052"}

[commit-bot: I haz the power, 2017-05-11 15:23:04]

Description was changed from

==========
[ios] Dismisses ToolsMenu on Layout changes.

Re-adding this now that we don't depend on the UI responderChain for dismissing 
the ToolsMenu.

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

to

==========
[ios] Dismisses ToolsMenu on Layout changes.

Re-adding this now that we don't depend on the UI responderChain for dismissing 
the ToolsMenu.

BUG= 682880

Review-Url: https://codereview.chromium.org/2869903002
Cr-Commit-Position: refs/heads/master@{#470961}
Committed:
https://chromium.googlesource.com/chromium/src/+/59e55ca446fab6e48a6da7687044...
==========

[commit-bot: I haz the power, 2017-05-11 15:23:05]

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