New issue
Advanced search Search tips

Issue 798418 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Task



Sign in to add a comment

Split the LegacyToolbarCoordinator into different sub-protocols

Project Member Reported by gambard@chromium.org, Jan 2 2018

Issue description

For now the LegacyToolbarCoordinator is implementing all the methods for the toolbar. As we are moving to an adaptive toolbar, the coordinator should be split into multiple protocols.

The idea is to separate all the methods only relevant to the primary toolbar to one protocol. Other methods should also moves to a protocol which will be implemented by the two toolbars.

This can allow BVC to have different properties, linking to the same object for now, but which would allow to see more clearly where the toolbar is used.
As we still have some legacy code, a legacy coordinator should be created and be removed (with all its call when cleaning the old toolbar implementation.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 4 2018

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

commit 83207456d4dfab26c85c014c1e7db381d9e27b53
Author: Gauthier Ambard <gambard@chromium.org>
Date: Thu Jan 04 09:16:10 2018

Create a PrimaryToolbar protocol

This CL creates an AbstractPrimaryToolbar protocol and moves some of
the methods from the LegacyToolbarCoordinator to it.

It also split the uses of the toolbar coordinator in the BVC in three:
- The PrimaryToolbarCoordinator, for the calls only related to the
   primary toolbar.
- The LegacyToolbarCoordinator, for the calls which will be deleted
   once the old toolbar is removed.
- The _toolbarCoordinator (current) for the calls which might be going
   to both toolbars or undecided yet.

This separation makes things clearer in BVC. It will allow an easier
transition to the adaptive toolbar as the responsibilities will be
cleaner.

It also moves the OmniboxFocuser and SideSwipeToolbarInteracting
protocols to the toolbar/public folder as they are used by the
AbstractPrimaryToolbar.

Bug:  798418 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I18822be75c483af7f1b810b66c450f521f720b37
Reviewed-on: https://chromium-review.googlesource.com/846993
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526943}
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/app/BUILD.gn
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/content_suggestions/BUILD.gn
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/content_suggestions/content_suggestions_header_view_controller.mm
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/key_commands_provider.h
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/ntp/new_tab_page_toolbar_controller.mm
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/side_swipe/BUILD.gn
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/side_swipe/card_side_swipe_view.mm
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/side_swipe/side_swipe_controller.mm
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/toolbar/clean/BUILD.gn
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/toolbar/clean/toolbar_coordinator.h
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/toolbar/clean/toolbar_coordinator.mm
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/toolbar/clean/toolbar_view_controller.mm
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/toolbar/legacy_toolbar_coordinator.h
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/toolbar/legacy_toolbar_coordinator.mm
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/toolbar/public/BUILD.gn
[rename] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/toolbar/public/omnibox_focuser.h
[add] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/toolbar/public/primary_toolbar_coordinator.h
[rename] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/toolbar/public/side_swipe_toolbar_interacting.h
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/toolbar/web_toolbar_controller.h
[modify] https://crrev.com/83207456d4dfab26c85c014c1e7db381d9e27b53/ios/chrome/browser/ui/toolbar/web_toolbar_delegate.h

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 4 2018

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

commit 569dd0ccbd64a2311aaf6df9f90cec997ddd30f0
Author: Gauthier Ambard <gambard@chromium.org>
Date: Thu Jan 04 15:55:08 2018

Move LegacyToolbar method to a specific protocol

This CL splits the use of the legacy method of the
LegacyToolbarCoordinator and regroup them in a specific protocol.
It allows a clearer interface description of the toolbar.

Bug:  798418 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ibde8c9e2a819be056de801ff6560922b22ec59d3
Reviewed-on: https://chromium-review.googlesource.com/848855
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526991}
[modify] https://crrev.com/569dd0ccbd64a2311aaf6df9f90cec997ddd30f0/ios/chrome/browser/ui/browser_view_controller_unittest.mm
[modify] https://crrev.com/569dd0ccbd64a2311aaf6df9f90cec997ddd30f0/ios/chrome/browser/ui/toolbar/legacy_toolbar_coordinator.h
[modify] https://crrev.com/569dd0ccbd64a2311aaf6df9f90cec997ddd30f0/ios/chrome/browser/ui/toolbar/legacy_toolbar_coordinator.mm
[modify] https://crrev.com/569dd0ccbd64a2311aaf6df9f90cec997ddd30f0/ios/chrome/browser/ui/toolbar/public/BUILD.gn
[add] https://crrev.com/569dd0ccbd64a2311aaf6df9f90cec997ddd30f0/ios/chrome/browser/ui/toolbar/public/legacy_toolbar_coordinator.h
[modify] https://crrev.com/569dd0ccbd64a2311aaf6df9f90cec997ddd30f0/ios/chrome/browser/ui/toolbar/toolbar_adapter.mm
[modify] https://crrev.com/569dd0ccbd64a2311aaf6df9f90cec997ddd30f0/ios/chrome/browser/ui/toolbar/web_toolbar_controller.mm

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 5 2018

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

commit c4d04f1a1104cd47b76c5031b3240a45062dfce1
Author: Gauthier Ambard <gambard@chromium.org>
Date: Fri Jan 05 07:53:39 2018

Use the LegacyToolbarCoordinator protocol in BVC

The LegacyToolbarCoordinator class is conforming to the
LegacyToolbarCoordinator protocol.
This CL is changing the type of the legacyToolbar property of the BVC
to conform to this protocol.
It allows an easier separation of the different usages of the toolbar.

Bug:  798418 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I87ca91a3d6854f1c5d08d2b9d03b1bb4bd0d197a
Reviewed-on: https://chromium-review.googlesource.com/850413
Reviewed-by: Mark Cogan <marq@chromium.org>
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527239}
[modify] https://crrev.com/c4d04f1a1104cd47b76c5031b3240a45062dfce1/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/c4d04f1a1104cd47b76c5031b3240a45062dfce1/ios/chrome/browser/ui/toolbar/legacy_toolbar_coordinator.h
[modify] https://crrev.com/c4d04f1a1104cd47b76c5031b3240a45062dfce1/ios/chrome/browser/ui/toolbar/public/primary_toolbar_coordinator.h

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 11 2018

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

commit 948048cd18a5ee8de7eea167e2d93844442b2f5f
Author: Gauthier Ambard <gambard@chromium.org>
Date: Thu Jan 11 12:38:26 2018

Create interface for toolbar coordinators

This CL creates a ToolbarCoordinatorAdaptor, allowing other objects to
communicate with both toolbars without having to take into account the
real number of toolbars (one or two).
It also adds the ToolsMenu as a child coordinator of this adaptor.
Having it as a child coordinator of the adaptor allows the coordinator
to have the ToolsMenuCommands to be dispatched. If we have one child
coordinator in each toolbar, the dispatcher wouldn't know to which
coordinator to dispatch the method calls.

Bug:  798418 ,  800330 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I780eb9a5446824824f8af2cd068115f4714121ca
Reviewed-on: https://chromium-review.googlesource.com/861626
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528606}
[modify] https://crrev.com/948048cd18a5ee8de7eea167e2d93844442b2f5f/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/948048cd18a5ee8de7eea167e2d93844442b2f5f/ios/chrome/browser/ui/toolbar/adaptive/BUILD.gn
[modify] https://crrev.com/948048cd18a5ee8de7eea167e2d93844442b2f5f/ios/chrome/browser/ui/toolbar/adaptive/primary_toolbar_coordinator.h
[modify] https://crrev.com/948048cd18a5ee8de7eea167e2d93844442b2f5f/ios/chrome/browser/ui/toolbar/adaptive/primary_toolbar_coordinator.mm
[add] https://crrev.com/948048cd18a5ee8de7eea167e2d93844442b2f5f/ios/chrome/browser/ui/toolbar/adaptive/toolbar_coordinator_adaptor.h
[add] https://crrev.com/948048cd18a5ee8de7eea167e2d93844442b2f5f/ios/chrome/browser/ui/toolbar/adaptive/toolbar_coordinator_adaptor.mm
[modify] https://crrev.com/948048cd18a5ee8de7eea167e2d93844442b2f5f/ios/chrome/browser/ui/toolbar/legacy_toolbar_coordinator.h
[modify] https://crrev.com/948048cd18a5ee8de7eea167e2d93844442b2f5f/ios/chrome/browser/ui/toolbar/public/BUILD.gn
[modify] https://crrev.com/948048cd18a5ee8de7eea167e2d93844442b2f5f/ios/chrome/browser/ui/toolbar/public/primary_toolbar_coordinator.h
[add] https://crrev.com/948048cd18a5ee8de7eea167e2d93844442b2f5f/ios/chrome/browser/ui/toolbar/public/toolbar_coordinating.h

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 16 2018

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

commit 5e6d17edba14f59ec437e4fcbe4a624494ce1c7d
Author: Gauthier Ambard <gambard@chromium.org>
Date: Tue Jan 16 08:08:10 2018

Moves the tabHistoryPositioner to legacy

This CL moves the tabHistoryPositioner protocol from the
PrimaryToolbarCoordinator to the LegacyToolbarCoordinator protocol.
The new implementation of the toolbar is using named layout guide
to position the tab history popup.

Bug:  798418 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ic7388eedaeb50971839f31b7d6d31a726070d960
Reviewed-on: https://chromium-review.googlesource.com/865899
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529378}
[modify] https://crrev.com/5e6d17edba14f59ec437e4fcbe4a624494ce1c7d/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/5e6d17edba14f59ec437e4fcbe4a624494ce1c7d/ios/chrome/browser/ui/toolbar/adaptive/primary_toolbar_coordinator.mm
[modify] https://crrev.com/5e6d17edba14f59ec437e4fcbe4a624494ce1c7d/ios/chrome/browser/ui/toolbar/public/legacy_toolbar_coordinator.h
[modify] https://crrev.com/5e6d17edba14f59ec437e4fcbe4a624494ce1c7d/ios/chrome/browser/ui/toolbar/public/primary_toolbar_coordinator.h

Status: Fixed (was: Assigned)

Sign in to add a comment