New issue
Advanced search Search tips

Issue 880672 link

Starred by 1 user
Status: Started
Owner:
kkhorimoto@chromium.org
Cc:
gambard@chromium.org
kkhorimoto@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Bug

Blocked on:
issue 880656


Sign in to add a comment

[iOS] Extend toolbar container functionality.

Project Member Reported by kkhorimoto@chromium.org, Sep 5 
As the solution to  Issue 880656 , we're creating a toolbar container coordinator/view controller that manages the layout of the secondary toolbar.  These classes should be expanded to also handle the top toolbar.  This would involve passing an array of toolbars rather than a single value.  Ultimately, the BVC can hold references to two ToolbarContainerCoordinators that manage the top and bottom toolbars, and all FullscreenUIElement code will be handled internally through these coordinators rather than through BVC.

Comment 1 by kkhorimoto@chromium.org, Sep 12

Owner: kkhorimoto@chromium.org
Status: Started (was: Available)
Working on this this week.
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 12 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/79d590b77b9df146e9a68c6f7083b705f9f1239f

commit 79d590b77b9df146e9a68c6f7083b705f9f1239f
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Wed Sep 12 19:09:28 2018

[iOS] Create flag for toolbar container code.

This will allow us to easily switch between the legacy implementation
and the ToolbarContainerCoordinator-based implementation.

Bug: 880672
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I67e3ac1b828f34adb64edd0f4aa0a2c66ead8dde
Reviewed-on: https://chromium-review.googlesource.com/1220721
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590775}
[modify] https://crrev.com/79d590b77b9df146e9a68c6f7083b705f9f1239f/ios/chrome/browser/BUILD.gn
[modify] https://crrev.com/79d590b77b9df146e9a68c6f7083b705f9f1239f/ios/chrome/browser/about_flags.mm
[modify] https://crrev.com/79d590b77b9df146e9a68c6f7083b705f9f1239f/ios/chrome/browser/ios_chrome_flag_descriptions.cc
[modify] https://crrev.com/79d590b77b9df146e9a68c6f7083b705f9f1239f/ios/chrome/browser/ios_chrome_flag_descriptions.h
[modify] https://crrev.com/79d590b77b9df146e9a68c6f7083b705f9f1239f/ios/chrome/browser/ui/BUILD.gn
[add] https://crrev.com/79d590b77b9df146e9a68c6f7083b705f9f1239f/ios/chrome/browser/ui/toolbar_container/BUILD.gn
[add] https://crrev.com/79d590b77b9df146e9a68c6f7083b705f9f1239f/ios/chrome/browser/ui/toolbar_container/toolbar_container_features.h
[add] https://crrev.com/79d590b77b9df146e9a68c6f7083b705f9f1239f/ios/chrome/browser/ui/toolbar_container/toolbar_container_features.mm

Comment 3 by kkhorimoto@chromium.org, Oct 5

Labels: ReleaseBlock-Stable M-72
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 9 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ef60d6ac06aa02f3ca7fe180fb1124858147ecb2

commit ef60d6ac06aa02f3ca7fe180fb1124858147ecb2
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Tue Oct 09 07:13:36 2018

[iOS] Remove toolbar container custom view flag.

This flag was only needed for an emergency shutoff to mitigate risk for
an RBS fix that occurred late in the branch.  It is not needed after
that fix is merged.

Original Fix:
crrev.com/c/1259882

Original Flag CL:
crrev.com/c/1269462

Bug: 880672
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: Ife2d1ce4eb60a0c0cb405c0f8e599215984ee45c
Reviewed-on: https://chromium-review.googlesource.com/c/1269615
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597827}
[modify] https://crrev.com/ef60d6ac06aa02f3ca7fe180fb1124858147ecb2/ios/chrome/browser/about_flags.mm
[modify] https://crrev.com/ef60d6ac06aa02f3ca7fe180fb1124858147ecb2/ios/chrome/browser/ios_chrome_flag_descriptions.cc
[modify] https://crrev.com/ef60d6ac06aa02f3ca7fe180fb1124858147ecb2/ios/chrome/browser/ios_chrome_flag_descriptions.h
[modify] https://crrev.com/ef60d6ac06aa02f3ca7fe180fb1124858147ecb2/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/ef60d6ac06aa02f3ca7fe180fb1124858147ecb2/ios/chrome/browser/ui/toolbar_container/toolbar_container_features.h
[modify] https://crrev.com/ef60d6ac06aa02f3ca7fe180fb1124858147ecb2/ios/chrome/browser/ui/toolbar_container/toolbar_container_features.mm
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 16 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aa055e655117238554d89ad4d411e54bfa6028bc

commit aa055e655117238554d89ad4d411e54bfa6028bc
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Tue Oct 16 00:08:58 2018

[iOS] Creates ToolbarContainerCoordinator and its view controller.

This coordinator manages a stack of optionally collapsible toolbars.
It manages laying out these toolbars in a top-to-bottom or bottom-to-
top order and interpolating the toolbars' heights based on the
fullscreen progress.

Ultimately, toolbar layout management will be moved from BVC to this
coordinator.

Bug: 880672
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I2b143895d30ce1ec68f58141d7364f1d32ef3f39
Reviewed-on: https://chromium-review.googlesource.com/c/1220722
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599790}
[modify] https://crrev.com/aa055e655117238554d89ad4d411e54bfa6028bc/ios/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/aa055e655117238554d89ad4d411e54bfa6028bc/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/aa055e655117238554d89ad4d411e54bfa6028bc/ios/chrome/browser/ui/toolbar_container/BUILD.gn
[add] https://crrev.com/aa055e655117238554d89ad4d411e54bfa6028bc/ios/chrome/browser/ui/toolbar_container/collapsing_toolbar_height_constraint.h
[add] https://crrev.com/aa055e655117238554d89ad4d411e54bfa6028bc/ios/chrome/browser/ui/toolbar_container/collapsing_toolbar_height_constraint.mm
[add] https://crrev.com/aa055e655117238554d89ad4d411e54bfa6028bc/ios/chrome/browser/ui/toolbar_container/collapsing_toolbar_height_constraint_unittest.mm
[add] https://crrev.com/aa055e655117238554d89ad4d411e54bfa6028bc/ios/chrome/browser/ui/toolbar_container/toolbar_collapsing.h
[add] https://crrev.com/aa055e655117238554d89ad4d411e54bfa6028bc/ios/chrome/browser/ui/toolbar_container/toolbar_container_coordinator.h
[add] https://crrev.com/aa055e655117238554d89ad4d411e54bfa6028bc/ios/chrome/browser/ui/toolbar_container/toolbar_container_coordinator.mm
[add] https://crrev.com/aa055e655117238554d89ad4d411e54bfa6028bc/ios/chrome/browser/ui/toolbar_container/toolbar_container_view.h
[add] https://crrev.com/aa055e655117238554d89ad4d411e54bfa6028bc/ios/chrome/browser/ui/toolbar_container/toolbar_container_view.mm
[add] https://crrev.com/aa055e655117238554d89ad4d411e54bfa6028bc/ios/chrome/browser/ui/toolbar_container/toolbar_container_view_controller.h
[add] https://crrev.com/aa055e655117238554d89ad4d411e54bfa6028bc/ios/chrome/browser/ui/toolbar_container/toolbar_container_view_controller.mm
[add] https://crrev.com/aa055e655117238554d89ad4d411e54bfa6028bc/ios/chrome/browser/ui/toolbar_container/toolbar_container_view_controller_unittest.mm
[modify] https://crrev.com/aa055e655117238554d89ad4d411e54bfa6028bc/ios/chrome/test/BUILD.gn
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 16 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/43b75585c11e9aee0bc40f3a8a26fb70c4216c2d

commit 43b75585c11e9aee0bc40f3a8a26fb70c4216c2d
Author: Olivier Robin <olivierrobin@chromium.org>
Date: Tue Oct 16 11:26:38 2018

Disable ToolbarContainerViewControllerTest tests on device

Disable BottomToTopExpanded and BottomToTopCollapsed

TBR: marq, kkhorimoto
Bug: 880672
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: If4c71557240e8589b52d6fe53491d661a5ff3a26
Reviewed-on: https://chromium-review.googlesource.com/c/1283029
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Commit-Queue: Olivier Robin <olivierrobin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599946}
[modify] https://crrev.com/43b75585c11e9aee0bc40f3a8a26fb70c4216c2d/ios/chrome/browser/ui/toolbar_container/toolbar_container_view_controller_unittest.mm
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 16 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a770ff653cb93438d6dd75f8cadee0be2eb23b83

commit a770ff653cb93438d6dd75f8cadee0be2eb23b83
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Tue Oct 16 14:17:44 2018

[iOS] Add toolbar_container/OWNERS file.

The owners are kkhorimoto@ (original author), and gambard@
(reviewer).

Bug: 880672
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I2d099c944b12e77c04106dc6de6de428824f1272
Reviewed-on: https://chromium-review.googlesource.com/c/1220720
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Peter Lee <pkl@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Commit-Queue: Peter Lee <pkl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599970}
[add] https://crrev.com/a770ff653cb93438d6dd75f8cadee0be2eb23b83/ios/chrome/browser/ui/toolbar_container/OWNERS
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 17 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4fffe197a2d9a55f6db616ce9cf3b2744effc59d

commit 4fffe197a2d9a55f6db616ce9cf3b2744effc59d
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Wed Oct 17 02:25:57 2018

[iOS] Use ToolbarContainerCoordinator for secondary toolbar.

This CL adds code to use a ToolbarContainerCoordinator to manage the
layout of the secondary toolbar.  This moves some fullscreen logic from
BVC and into the toolbar container view controller.  The added code is
behind the kToolbarContainerEnabled flag.

Bug: 880672
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: Icd2e3b48bb0947b871bd4855a83a78c1a58af0e4
Reviewed-on: https://chromium-review.googlesource.com/c/1222915
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600251}
[modify] https://crrev.com/4fffe197a2d9a55f6db616ce9cf3b2744effc59d/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/4fffe197a2d9a55f6db616ce9cf3b2744effc59d/ios/chrome/browser/ui/toolbar/BUILD.gn
[modify] https://crrev.com/4fffe197a2d9a55f6db616ce9cf3b2744effc59d/ios/chrome/browser/ui/toolbar/secondary_toolbar_view.mm
[modify] https://crrev.com/4fffe197a2d9a55f6db616ce9cf3b2744effc59d/ios/chrome/browser/ui/toolbar/secondary_toolbar_view_controller.mm
[modify] https://crrev.com/4fffe197a2d9a55f6db616ce9cf3b2744effc59d/ios/chrome/browser/ui/toolbar_container/BUILD.gn
[modify] https://crrev.com/4fffe197a2d9a55f6db616ce9cf3b2744effc59d/ios/chrome/browser/ui/toolbar_container/toolbar_collapsing.h
[modify] https://crrev.com/4fffe197a2d9a55f6db616ce9cf3b2744effc59d/ios/chrome/browser/ui/toolbar_container/toolbar_container_coordinator.h
[modify] https://crrev.com/4fffe197a2d9a55f6db616ce9cf3b2744effc59d/ios/chrome/browser/ui/toolbar_container/toolbar_container_coordinator.mm
[modify] https://crrev.com/4fffe197a2d9a55f6db616ce9cf3b2744effc59d/ios/chrome/browser/ui/toolbar_container/toolbar_container_view_controller.mm
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 18 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/343f58e4df9cefa7be327d3fd8419d7276f1d837

commit 343f58e4df9cefa7be327d3fd8419d7276f1d837
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Thu Oct 18 21:04:33 2018

[iOS] Fix toolbar container progress logic.

The previous implementation of ToolbarContainer scaled
each toolbar's interpolation progress at the same rate,
resulting in accordion-like behavior where all toolbars
were expanded and collapsed at the same time.  The desired
effect is to have telescopic behavior, where the toolbars
are extended one at a time.  This is needed on iPad, where
a scroll event should first reduce the height of the tab
strip to 0.0 before reducing the height of the primary
toolbar.

This CL adds logic to convert the [0.0, 1.0] fullscreen
progress range for the overall stack to individual progress
values for each toolbar, such that the last toolbar is
expanded first, and the previous toolbars don't start
expanding until all subsequent ones are fully expanded.
The range of stack-level progress values corresponding to
a particular toolbar is proportional to that toolbar's
height delta relative to the height delta of the overall
stack.

In addition, this CL also introduces HeightRange, a simple
container object that can be used to hold max and min heights
and perform interpolation calculations.

Bug: 880672,  895766 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I1205ec75ac3e9a0e57dc19d1ebcc1f1740f8aeac
Reviewed-on: https://chromium-review.googlesource.com/c/1231933
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600903}
[modify] https://crrev.com/343f58e4df9cefa7be327d3fd8419d7276f1d837/ios/chrome/browser/ui/toolbar_container/BUILD.gn
[modify] https://crrev.com/343f58e4df9cefa7be327d3fd8419d7276f1d837/ios/chrome/browser/ui/toolbar_container/collapsing_toolbar_height_constraint.h
[modify] https://crrev.com/343f58e4df9cefa7be327d3fd8419d7276f1d837/ios/chrome/browser/ui/toolbar_container/collapsing_toolbar_height_constraint.mm
[add] https://crrev.com/343f58e4df9cefa7be327d3fd8419d7276f1d837/ios/chrome/browser/ui/toolbar_container/collapsing_toolbar_height_constraint_delegate.h
[modify] https://crrev.com/343f58e4df9cefa7be327d3fd8419d7276f1d837/ios/chrome/browser/ui/toolbar_container/collapsing_toolbar_height_constraint_unittest.mm
[modify] https://crrev.com/343f58e4df9cefa7be327d3fd8419d7276f1d837/ios/chrome/browser/ui/toolbar_container/toolbar_container_coordinator.mm
[modify] https://crrev.com/343f58e4df9cefa7be327d3fd8419d7276f1d837/ios/chrome/browser/ui/toolbar_container/toolbar_container_view_controller.h
[modify] https://crrev.com/343f58e4df9cefa7be327d3fd8419d7276f1d837/ios/chrome/browser/ui/toolbar_container/toolbar_container_view_controller.mm
[modify] https://crrev.com/343f58e4df9cefa7be327d3fd8419d7276f1d837/ios/chrome/browser/ui/toolbar_container/toolbar_container_view_controller_unittest.mm
[add] https://crrev.com/343f58e4df9cefa7be327d3fd8419d7276f1d837/ios/chrome/browser/ui/toolbar_container/toolbar_height_range.h
[add] https://crrev.com/343f58e4df9cefa7be327d3fd8419d7276f1d837/ios/chrome/browser/ui/toolbar_container/toolbar_height_range.mm
[add] https://crrev.com/343f58e4df9cefa7be327d3fd8419d7276f1d837/ios/chrome/browser/ui/toolbar_container/toolbar_height_range_unittest.mm

Comment 10 by kkhorimoto@chromium.org, Oct 19 

Issue 813149 has been merged into this issue.

Comment 11 by kkhorimoto@chromium.org, Oct 19 

 Issue 806437  has been merged into this issue.

Comment 12 by kariahda@chromium.org, Dec 10 

Why was this marked RBS for 72? Does it link to a certain feature for 72?

Comment 13 by kkhorimoto@google.com, Dec 10

Labels: -ReleaseBlock-Stable
This should not block the release.
►

Sign in to add a comment