New issue
Advanced search Search tips

Issue 804533 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Task
Q1

Blocked on:
issue 804531

Blocking:
issue 804534
issue 804542



Sign in to add a comment

Have MainCoordinator and MainPresentingViewController use the tab grid as the tab switcher.

Project Member Reported by marq@chromium.org, Jan 22 2018

Issue description

Have MainCoordinator and MainPresentingViewController use the tab grid as the tab switcher.
 

Comment 1 by marq@chromium.org, Jan 23 2018

Components: UI>Browser
Labels: MS-Tab-Grid Pri-2
Owner: marq@chromium.org
Status: Available (was: Unconfirmed)

Comment 2 by marq@chromium.org, Jan 23 2018

Labels: S-See-Open-Tabs

Comment 3 by marq@chromium.org, Jan 23 2018

Labels: medium

Comment 4 by marq@chromium.org, Jan 23 2018

Blockedon: 804531

Comment 5 by marq@chromium.org, Jan 23 2018

Labels: Q1

Comment 6 by marq@chromium.org, Jan 25 2018

Status: Assigned (was: Available)

Comment 7 by marq@chromium.org, Feb 12 2018

Status: Started (was: Assigned)
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 13 2018

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

commit 184863a9eec5587b23dc1c7702c7e6e421f85f2d
Author: Mark Cogan <marq@google.com>
Date: Tue Feb 13 22:04:21 2018

[iOS] Allow TabSwitcher implementors to not be UIViewControllers

This CL changes all uses of UIViewController<TabSwitcher> to simply be id<TabSwitcher>, and
changes the |view| method in TabSwitcher to be a |viewController| method instead.

Call sites that access the |view| method now use |viewController.view|.

Code that directly treated a TabSwitcher implementor as a view controller now does so through
the ViewController method.

By making this change, it isn't necessary that an object that implements TabSwitcher is responsible
for both model-layer (i.e., new tab creation) and UI-layer (being a view controller) tasks.

Bug:  804533 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I8371b034b6932f0e584fb4167ec2203d819c8e3b
Reviewed-on: https://chromium-review.googlesource.com/915603
Commit-Queue: Mark Cogan <marq@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536499}
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/app/main_controller_private.h
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/main/BUILD.gn
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/main/main_containing_view_controller.mm
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/main/main_containing_view_controller_unittest.mm
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/main/main_presenting_view_controller.h
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/main/main_presenting_view_controller.mm
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/main/main_presenting_view_controller_unittest.mm
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/main/main_view_controller_test.h
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/main/main_view_controller_test.mm
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/main/transitions/bvc_container_to_tab_switcher_animator.h
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/main/transitions/bvc_container_to_tab_switcher_animator.mm
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/main/transitions/tab_switcher_to_bvc_container_animator.h
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/main/transitions/tab_switcher_to_bvc_container_animator.mm
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/main/view_controller_swapping.h
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/stack_view/stack_view_controller.h
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/stack_view/stack_view_controller.mm
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/tab_switcher/tab_switcher.h
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/browser/ui/tab_switcher/tab_switcher_controller.mm
[modify] https://crrev.com/184863a9eec5587b23dc1c7702c7e6e421f85f2d/ios/chrome/test/app/chrome_test_util.mm

Project Member

Comment 9 by bugdroid1@chromium.org, Feb 15 2018

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

commit b66e111357bece445f2917a82d0c29a85c288d06
Author: Mark Cogan <marq@google.com>
Date: Thu Feb 15 00:06:22 2018

[iOS] Allow ViewControllerSwapping implementors to not be UIViewControllers.

This CL changes all use of UIViewController<ViewControllerSwapping> to
simply be id<ViewControllerSwapping>.

A -viewController property is added to ViewControllerSwapping so that
calling code can access the view controller doing the swapping (there is one
call site for this at the moment).

Since MainController's MainViewController property is no longer necessarily
a view controller, it's also renamed to 'viewControllerSwapper'. (As a side
effect, this further reduces the coupling of MainController to the UI layer).

This will allow the tab grid to move the view controller swapping logic into
its coordinator, where it logically belongs, instead of coupling this to its
view controller.

Bug:  804533 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I9821aa84aa17d61f1eef34376039714510fd3e1d
Reviewed-on: https://chromium-review.googlesource.com/920081
Reviewed-by: edchin <edchin@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Commit-Queue: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536898}
[modify] https://crrev.com/b66e111357bece445f2917a82d0c29a85c288d06/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/b66e111357bece445f2917a82d0c29a85c288d06/ios/chrome/browser/ui/main/main_containing_view_controller.mm
[modify] https://crrev.com/b66e111357bece445f2917a82d0c29a85c288d06/ios/chrome/browser/ui/main/main_presenting_view_controller.mm
[modify] https://crrev.com/b66e111357bece445f2917a82d0c29a85c288d06/ios/chrome/browser/ui/main/view_controller_swapping.h

Comment 10 by marq@chromium.org, Feb 15 2018

NextAction: 2018-02-28
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 16 2018

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

commit 481431db5646dca8a53dd35563a67a7c3f8fb81f
Author: Mark Cogan <marq@google.com>
Date: Fri Feb 16 17:52:32 2018

[iOS] Preliminary interfaces for TabGrid.

This CL wires the Tab Grid into MainController.

The TabGridCoordinator is the MainCoordinator in this case (it creates and assigns the
root view controller for the app).

The TabGridViewController adopts the ViewControllerSwapping
protocol to show a tab switcher or a BVC. Since the TabGridViewController is itself the
view controller for the tab switcher when the tab grid is enabled, showing the tab switcher
is a no-op if a BVC isn't shown. The other presentation logic is lifted from
MainPresentingViewController.

To make that a bit cleaner, this CL factors bvc_container_view_controller into its own file.

The new TabGridAdaptor object, created and owned by the TabGridCoordinator, is the object
that exposes the TabSwitcher protocol. Future CLs will add hooks for it into the tab grid
controller and coordinator, which will be opaque to MainController.

As another small cleanup, the 'tabSwitcherController' ivar in MainController is renamed to
'tabSwitcher', since it's no longer necessarily a UIViewController. MainCoordinator also
separates its view controller from its view controller swapper, since in the tab grid
coordinator subclass those aren't the same thing.

The TabGridCoordinator unit tests are largely cloned from MainPresentingViewController's
unit tests, since their APIs are (at this stage) essentially the same.

Bug:  804533 ,  804531 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ia39101fc384100ca9c1ea9c1f7b08bc7e08b70e0
Reviewed-on: https://chromium-review.googlesource.com/917671
Commit-Queue: Mark Cogan <marq@chromium.org>
Reviewed-by: edchin <edchin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537346}
[modify] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/app/application_delegate/url_opener_unittest.mm
[modify] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/app/main_controller_private.h
[modify] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/browser/ui/main/BUILD.gn
[add] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/browser/ui/main/bvc_container_view_controller.h
[add] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/browser/ui/main/bvc_container_view_controller.mm
[modify] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/browser/ui/main/main_coordinator.h
[modify] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/browser/ui/main/main_coordinator.mm
[modify] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/browser/ui/main/main_presenting_view_controller.mm
[modify] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/browser/ui/tab_grid/BUILD.gn
[add] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/browser/ui/tab_grid/tab_grid_adaptor.h
[add] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/browser/ui/tab_grid/tab_grid_adaptor.mm
[modify] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/browser/ui/tab_grid/tab_grid_coordinator.h
[modify] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm
[add] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/browser/ui/tab_grid/tab_grid_coordinator_unittest.mm
[modify] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/browser/ui/tab_grid/tab_grid_view_controller.h
[modify] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm
[modify] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/browser/ui/tab_switcher/tab_switcher.h
[modify] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/test/BUILD.gn
[modify] https://crrev.com/481431db5646dca8a53dd35563a67a7c3f8fb81f/ios/chrome/test/app/stack_view_test_util.mm

Project Member

Comment 12 by bugdroid1@chromium.org, Feb 28 2018

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

commit 43dbc4ac4fc1efe56460282f05c99524419d4685
Author: Mark Cogan <marq@google.com>
Date: Wed Feb 28 01:59:52 2018

[iOS] add further TabSwitcher support to TabGrid.

This CL makes all the implementation of the TabSwitcher protocol in TabGridAdaptor complete,
providing non-stub methods where required.

This includes:
  - Adding a dispatcher to the TabGridCoordinator, matching how this is handled in the existing
    tab switcher implementations. For now the BrowserCommands methods handled in the coordinator
    are stubs.
  - Adding code in the adaptor to create and open a new tab. The code paths for this from
    MainController are from external events, generally (see testing note below).
  - Adding code in the coordinator's ViewControllerPresengting method to call the TabSwitcher's
    delegate method when the presentation of a tab completes.

I also cleaned up pragma comments in MainController (grouping all of the TabSwitcherDelegate
methods before the helpers) and corrected a method name in a comment in tab_switcher.h

I tested manually in simulator by launching Chrome (with the tab grid flag enabled), entering the
tab grid, then backgrounding Chrome. Then I 3D-touched the Chrome icon to get the various quick
action options, and tested each of them (they all open tabs using this code path).

My expectation is that that unit and egtests will (still) not pass with the tab grid flag enabled.
Fixes for that will follow in another CL (that's probably where the BrowserCommand methods will
get filled in).

Bug:  804533 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I3427075225ab10c33aaa13d3f3e01c033e30cd12
Reviewed-on: https://chromium-review.googlesource.com/940195
Commit-Queue: Mark Cogan <marq@chromium.org>
Reviewed-by: edchin <edchin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539649}
[modify] https://crrev.com/43dbc4ac4fc1efe56460282f05c99524419d4685/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/43dbc4ac4fc1efe56460282f05c99524419d4685/ios/chrome/browser/ui/tab_grid/BUILD.gn
[modify] https://crrev.com/43dbc4ac4fc1efe56460282f05c99524419d4685/ios/chrome/browser/ui/tab_grid/tab_grid_adaptor.h
[modify] https://crrev.com/43dbc4ac4fc1efe56460282f05c99524419d4685/ios/chrome/browser/ui/tab_grid/tab_grid_adaptor.mm
[modify] https://crrev.com/43dbc4ac4fc1efe56460282f05c99524419d4685/ios/chrome/browser/ui/tab_grid/tab_grid_coordinator.h
[modify] https://crrev.com/43dbc4ac4fc1efe56460282f05c99524419d4685/ios/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm
[modify] https://crrev.com/43dbc4ac4fc1efe56460282f05c99524419d4685/ios/chrome/browser/ui/tab_grid/tab_grid_coordinator_unittest.mm
[modify] https://crrev.com/43dbc4ac4fc1efe56460282f05c99524419d4685/ios/chrome/browser/ui/tab_switcher/tab_switcher.h

Comment 13 by marq@chromium.org, Feb 28 2018

Status: Fixed (was: Started)
The NextAction date has arrived: 2018-02-28

Comment 15 by cmasso@google.com, Feb 28 2018

NextAction: ----

Sign in to add a comment