Simple strip container that can reveal/hide tab strip.

Simple strip container that can reveal/hide tab strip.

The simple strip container is WIP and this CL has only
minimal functionality. This CL also showcases this
strip container in Showcase.

BUG=none.

Committed: https://crrev.com/8359e507d5430b69e498e1e190fa7b9c55974711
Cr-Commit-Position: refs/heads/master@{#440853}

[edchin, 2016-12-21 16:31:35]

Description was changed from

==========
Simple strip container that can reveal/hide tab strip.

BUG=none.
==========

to

==========
Simple strip container that can reveal/hide tab strip.

This CL also showcases this strip container in
Showcase.

BUG=none.
==========

[edchin, 2016-12-21 16:33:48]

Description was changed from

==========
Simple strip container that can reveal/hide tab strip.

This CL also showcases this strip container in
Showcase.

BUG=none.
==========

to

==========
Simple strip container that can reveal/hide tab strip.

The simple strip container is WIP and this CL has only
minimal functionality. This CL also showcases this 
strip container in Showcase. 

BUG=none.
==========

[edchin, 2016-12-21 16:43:04]

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

[edchin, 2016-12-21 16:43:05]

edchin@chromium.org changed required reviewers:
+ marq@chromium.org

[edchin, 2016-12-21 16:43:05]

PTAL. This is just the first strip container CL. I wanted to have a very basic
first iteration.

[marq (ping after 24h), 2016-12-21 17:26:15]

Great start, just a couple of tweaks.

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/strip/strip_container_view_controller.h (right):

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/strip/strip_container_view_controller.h:17: // Subclasses
manage the specific layout of these view controllers.
Is the sentence about subclasses true in this case?

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/strip/strip_container_view_controller.mm (right):

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/strip/strip_container_view_controller.mm:25:
@property(nonatomic, weak) UIViewController* topmostViewController;
I don't think you need this if we aren't going to vary the strip position (which
we aren't).

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/strip/strip_container_view_controller.mm:46: -
(void)didAddContentViewController;
-didAddStripViewController, right? Or is this error in the tab container as
well?

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/strip/strip_container_view_controller.mm:223: // Action
for UIResponder chain that toggles visibility of tab strip.
Nit: all actions are by definition using the responder chain. "Action to toggle
visibility of the tab strip".

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/strip/strip_container_view_controller.mm:224: -
(void)toggleTabStrip:(id)sender {
Add a ui/actions/tab_strip_actions.h file with a TabStripActions protocol for
this.

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/strip/strip_container_view_controller.mm:227: [self.view
setNeedsUpdateConstraints];
Is this needed? No constraints have been added or removed. Changing the constant
of an active constraint should mark the constrained views as dirty and trigger a
layout.

https://codereview.chromium.org/2594023002/diff/40001/ios/showcase/strip/sc_s...
File ios/showcase/strip/sc_strip_coordinator.mm (right):

https://codereview.chromium.org/2594023002/diff/40001/ios/showcase/strip/sc_s...
ios/showcase/strip/sc_strip_coordinator.mm:23:
blackViewController.view.backgroundColor = [UIColor blackColor];
Newline after this to separate the two view controller setups.

[edchin, 2016-12-22 10:17:17]

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/strip/strip_container_view_controller.h (right):

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/strip/strip_container_view_controller.h:17: // Subclasses
manage the specific layout of these view controllers.
On 2016/12/21 17:26:14, marq wrote:
> Is the sentence about subclasses true in this case?

Removed.

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/strip/strip_container_view_controller.mm (right):

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/strip/strip_container_view_controller.mm:25:
@property(nonatomic, weak) UIViewController* topmostViewController;
On 2016/12/21 17:26:15, marq wrote:
> I don't think you need this if we aren't going to vary the strip position
(which
> we aren't).

This container implementation accounts for when there is no tab strip view. So
doesn't it need to change the status bar based on that? In fact, perhaps I need
to implement it so that the status bar changes whenever one toggles the tab
strip.

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/strip/strip_container_view_controller.mm:46: -
(void)didAddContentViewController;
On 2016/12/21 17:26:15, marq wrote:
> -didAddStripViewController, right? Or is this error in the tab container as
> well?

Error in tab container as well. Will update that in a separate CL.

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/strip/strip_container_view_controller.mm:223: // Action
for UIResponder chain that toggles visibility of tab strip.
On 2016/12/21 17:26:15, marq wrote:
> Nit: all actions are by definition using the responder chain. "Action to
toggle
> visibility of the tab strip".

Done.

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/strip/strip_container_view_controller.mm:224: -
(void)toggleTabStrip:(id)sender {
On 2016/12/21 17:26:15, marq wrote:
> Add a ui/actions/tab_strip_actions.h file with a TabStripActions protocol for
> this.

Done.

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/strip/strip_container_view_controller.mm:227: [self.view
setNeedsUpdateConstraints];
On 2016/12/21 17:26:15, marq wrote:
> Is this needed? No constraints have been added or removed. Changing the
constant
> of an active constraint should mark the constrained views as dirty and trigger
a
> layout.

Done.

[edchin, 2016-12-22 10:37:45]

https://codereview.chromium.org/2594023002/diff/40001/ios/showcase/strip/sc_s...
File ios/showcase/strip/sc_strip_coordinator.mm (right):

https://codereview.chromium.org/2594023002/diff/40001/ios/showcase/strip/sc_s...
ios/showcase/strip/sc_strip_coordinator.mm:23:
blackViewController.view.backgroundColor = [UIColor blackColor];
On 2016/12/21 17:26:15, marq wrote:
> Newline after this to separate the two view controller setups.

Done.

https://codereview.chromium.org/2594023002/diff/80001/ios/showcase/strip/sc_s...
File ios/showcase/strip/sc_strip_coordinator.mm (right):

https://codereview.chromium.org/2594023002/diff/80001/ios/showcase/strip/sc_s...
ios/showcase/strip/sc_strip_coordinator.mm:28:
action:@selector(toggleTabStrip:)];
This is highly susceptible to mistyping and that will not be caught by the
compiler. If there is a typo, no one in the responder chain will catch the
action, and there will be no complaints even at runtime. Can we think of any way
to guard against this?

[marq (ping after 24h), 2016-12-22 11:33:07]

LGTM!

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/strip/strip_container_view_controller.mm (right):

https://codereview.chromium.org/2594023002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/strip/strip_container_view_controller.mm:25:
@property(nonatomic, weak) UIViewController* topmostViewController;
On 2016/12/22 10:17:17, edchin wrote:
> On 2016/12/21 17:26:15, marq wrote:
> > I don't think you need this if we aren't going to vary the strip position
> (which
> > we aren't).
> 
> This container implementation accounts for when there is no tab strip view. So
> doesn't it need to change the status bar based on that? In fact, perhaps I
need
> to implement it so that the status bar changes whenever one toggles the tab
> strip. 

You are of course correct.

https://codereview.chromium.org/2594023002/diff/80001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/actions/tab_strip_actions.h (right):

https://codereview.chromium.org/2594023002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/actions/tab_strip_actions.h:8: #import
<Foundation/Foundation.h>
Add new architecture banner.

https://codereview.chromium.org/2594023002/diff/80001/ios/showcase/strip/sc_s...
File ios/showcase/strip/sc_strip_coordinator.mm (right):

https://codereview.chromium.org/2594023002/diff/80001/ios/showcase/strip/sc_s...
ios/showcase/strip/sc_strip_coordinator.mm:28:
action:@selector(toggleTabStrip:)];
On 2016/12/22 10:37:45, edchin wrote:
> This is highly susceptible to mistyping and that will not be caught by the
> compiler. If there is a typo, no one in the responder chain will catch the
> action, and there will be no complaints even at runtime. Can we think of any
way
> to guard against this? 

The app delegate is the last step in the responder chain, and we can have it
respond to all methods with a CHECK; that will catch it at runtime. We can use
macros instead of @selector when setting actions for compile-time checks.

[edchin, 2016-12-22 11:42:14]

https://codereview.chromium.org/2594023002/diff/80001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/actions/tab_strip_actions.h (right):

https://codereview.chromium.org/2594023002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/actions/tab_strip_actions.h:8: #import
<Foundation/Foundation.h>
On 2016/12/22 11:33:07, marq wrote:
> Add new architecture banner.

Done.

https://codereview.chromium.org/2594023002/diff/80001/ios/showcase/strip/sc_s...
File ios/showcase/strip/sc_strip_coordinator.mm (right):

https://codereview.chromium.org/2594023002/diff/80001/ios/showcase/strip/sc_s...
ios/showcase/strip/sc_strip_coordinator.mm:28:
action:@selector(toggleTabStrip:)];
On 2016/12/22 11:33:07, marq wrote:
> On 2016/12/22 10:37:45, edchin wrote:
> > This is highly susceptible to mistyping and that will not be caught by the
> > compiler. If there is a typo, no one in the responder chain will catch the
> > action, and there will be no complaints even at runtime. Can we think of any
> way
> > to guard against this? 
> 
> The app delegate is the last step in the responder chain, and we can have it
> respond to all methods with a CHECK; that will catch it at runtime. We can use
> macros instead of @selector when setting actions for compile-time checks.

Acknowledged.

[edchin, 2016-12-27 21:47:18]

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

[commit-bot: I haz the power, 2016-12-27 21:47:24]

Dry run: 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, 2016-12-27 22:37:17]

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

[commit-bot: I haz the power, 2016-12-27 22:37:18]

Dry run: This issue passed the CQ dry run.

[marq (ping after 24h), 2016-12-28 09:36:45]

On 2016/12/22 11:42:14, edchin wrote:
>
https://codereview.chromium.org/2594023002/diff/80001/ios/chrome/browser/ui/a...
> File ios/chrome/browser/ui/actions/tab_strip_actions.h (right):
> 
>
https://codereview.chromium.org/2594023002/diff/80001/ios/chrome/browser/ui/a...
> ios/chrome/browser/ui/actions/tab_strip_actions.h:8: #import
> <Foundation/Foundation.h>
> On 2016/12/22 11:33:07, marq wrote:
> > Add new architecture banner.
> 
> Done.
> 
>
https://codereview.chromium.org/2594023002/diff/80001/ios/showcase/strip/sc_s...
> File ios/showcase/strip/sc_strip_coordinator.mm (right):
> 
>
https://codereview.chromium.org/2594023002/diff/80001/ios/showcase/strip/sc_s...
> ios/showcase/strip/sc_strip_coordinator.mm:28:
> action:@selector(toggleTabStrip:)];
> On 2016/12/22 11:33:07, marq wrote:
> > On 2016/12/22 10:37:45, edchin wrote:
> > > This is highly susceptible to mistyping and that will not be caught by the
> > > compiler. If there is a typo, no one in the responder chain will catch the
> > > action, and there will be no complaints even at runtime. Can we think of
any
> > way
> > > to guard against this? 
> > 
> > The app delegate is the last step in the responder chain, and we can have it
> > respond to all methods with a CHECK; that will catch it at runtime. We can
use
> > macros instead of @selector when setting actions for compile-time checks.
> 
> Acknowledged.

In a hilarious twist, everything I said in that comment was incorrect. 

The application *object* (not delegate) is the last step in the responder chain.
We may or may not want to jam unhandled-action-checking into a UIApplication
subclass.

We can't really use macros instead of @selector in any way that's not
unpleasant, but we can (and have) turn on the -Wundeclared-selector warning
which will catch random selector typos at compile-time.

[edchin, 2016-12-28 16:38:54]

On 2016/12/28 09:36:45, marq wrote:
> We can't really use macros instead of @selector in any way that's not
> unpleasant, but we can (and have) turn on the -Wundeclared-selector warning
> which will catch random selector typos at compile-time.

Incidentally, I had the wrong import which meant that the selector was not
declared properly.
But the newly enabled undeclared-selector warning caught the problem.

[edchin, 2016-12-28 16:39:03]

The CQ bit was checked by edchin@chromium.org

[edchin, 2016-12-28 16:39:03]

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/2594023002/#ps120001
(title: "Update import.")

[commit-bot: I haz the power, 2016-12-28 16:39:18]

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, 2016-12-28 16:43:28]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482943143315870,
"parent_rev": "12d704a3cd34134ced87a86c7c4377c6ac9b854c", "commit_rev":
"e48eab87411ae9fed13d5efb9f4d7b8d39a08206"}

[commit-bot: I haz the power, 2016-12-28 16:43:55]

Description was changed from

==========
Simple strip container that can reveal/hide tab strip.

The simple strip container is WIP and this CL has only
minimal functionality. This CL also showcases this 
strip container in Showcase. 

BUG=none.
==========

to

==========
Simple strip container that can reveal/hide tab strip.

The simple strip container is WIP and this CL has only
minimal functionality. This CL also showcases this 
strip container in Showcase. 

BUG=none.

Review-Url: https://codereview.chromium.org/2594023002
==========

[commit-bot: I haz the power, 2016-12-28 16:43:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2017-01-02 15:49:08]

Description was changed from

==========
Simple strip container that can reveal/hide tab strip.

The simple strip container is WIP and this CL has only
minimal functionality. This CL also showcases this 
strip container in Showcase. 

BUG=none.

Review-Url: https://codereview.chromium.org/2594023002
==========

to

==========
Simple strip container that can reveal/hide tab strip.

The simple strip container is WIP and this CL has only
minimal functionality. This CL also showcases this
strip container in Showcase.

BUG=none.

Committed: https://crrev.com/8359e507d5430b69e498e1e190fa7b9c55974711
Cr-Commit-Position: refs/heads/master@{#440853}
==========

[commit-bot: I haz the power, 2017-01-02 15:49:09]

Patchset 7 (id:??) landed as
https://crrev.com/8359e507d5430b69e498e1e190fa7b9c55974711
Cr-Commit-Position: refs/heads/master@{#440853}